10000 perf(p2p): Only update send monitor once per batch packet msg send by ValarDragon · Pull Request #3382 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

perf(p2p): Only update send monitor once per batch packet msg send #3382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jul 3, 2024
Merged

perf(p2p): Only update send monitor once per batch packet msg send #3382

merged 8 commits into from
Jul 3, 2024

Conversation

ValarDragon
Copy link
Contributor
@ValarDragon ValarDragon commented Jul 1, 2024 •
edited by cason
Loading

Small optimization to outbound packet gossip, I expect this to be a 1-2% speedup to outbound packet gossip as is right now. Will test on mainnet soon

This is safe as outbound packet gossip is single threaded per peer as is right now. Technically makes the send monitor marginally less real time, but this is irrelevant as the send monitor works on 20ms sliding windows anyway

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@ValarDragon ValarDragon requested review from a team as code owners July 1, 2024 11:23
@ValarDragon ValarDragon changed the title Only update send monitor once per batch packet msg send perf(p2p): Only update send monitor once per batch packet msg send Jul 1, 2024
melekes
melekes approved these changes Jul 1, 2024
View reviewed changes
Copy link
Contributor
@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ValarDragon ❤️

cason
cason reviewed Jul 1, 2024
View reviewed changes
Copy link
Contributor
@cason cason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. By I wondering if we are preserving the same behavior as before, in particular in terms of the error handling.

In short, if we return at line 533 we are not updating the monitor at all, even with the bytes sent in previously and successful calls. The same is valid for line 529, isn't it?

The solution I suggest is to update the sendMonitor as part of a defer clause, it is done in any case.

cason
cason reviewed Jul 1, 2024
View reviewed changes
cason
cason reviewed Jul 1, 2024
View reviewed changes
ValarDragon and others added 2 commits July 1, 2024 21:24
@ValarDragon @melekes
Apply suggestions from code review
ed30301 
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@ValarDragon
Apply @cason's fix
466b36e
@ValarDragon
Copy link
Contributor Author

Great catch, fixed!

cason
cason approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor
@cason cason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good.

There is a mandatory fix, line 542 to be removed, otherwise the behavior will be wrong.

I think we would need to have some test units for this rate limiting implementation...

p2p/conn/connection.go Outdated
if err != nil {
c.Logger.Error("Failed to write PacketMsg", "err", err)
c.stopForError(err)
return true
return n, true
}
// TODO: Change this to only do one update for the entire bawtch.
Copy l 10000 ink
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// TODO: Change this to only do one update for the entire bawtch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO solved, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still applies for the flush monitor

@cason cason enabled auto-merge July 3, 2024 07:12
@cason cason added this pull request to the merge queue Jul 3, 2024
Merged via the queue into main with commit 20d8630 Jul 3, 2024
40 checks passed
@cason cason deleted the dev/single_sendmonitor_update branch July 3, 2024 10:21
@ValarDragon ValarDragon added the backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x label Jul 3, 2024
mergify bot pushed a commit that referenced this pull request Jul 3, 2024
@ValarDragon @mergify
perf(p2p): Only update send monitor once per batch packet msg send (#…
8000
d733310 
…3382)

Small optimization to outbound packet gossip, I expect this to be a 1-2%
speedup to outbound packet gossip as is right now. Will test on mainnet
soon

This is safe as outbound packet gossip is single threaded per peer as is
right now. Technically makes the send monitor marginally less real time,
but this is irrelevant as the send monitor works on 20ms sliding windows
anyway

---

#### PR checklist

- [ ] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Co-authored-by: Daniel <daniel.cason@informal.systems>
(cherry picked from commit 20d8630)

# Conflicts:
#	.changelog/v0.38.8/improvements/3382-single-send-monitor-per-packet.md
@mergify mergify bot mentioned this pull request Jul 3, 2024
Merged
4 tasks
ValarDragon added a commit to osmosis-labs/cometbft that referenced this pull request Jul 3, 2024
@ValarDragon @melekes @cason
perf(p2p): Only update send monitor once per batch packet msg send (c…
68397ea 
…ometbft#3382)

Small optimization to outbound packet gossip, I expect this to be a 1-2%
speedup to outbound packet gossip as is right now. Will test on mainnet
soon

This is safe as outbound packet gossip is single threaded per peer as is
right now. Technically makes the send monitor marginally less real time,
but this is irrelevant as the send monitor works on 20ms sliding windows
anyway

---

- [ ] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Co-authored-by: Daniel <daniel.cason@informal.systems>
(cherry picked from commit 20d8630)
itsdevbear pushed a commit to berachain/cometbft that referenced this pull request Jul 4, 2024
@ValarDragon @melekes @cason
perf(p2p): Only update send monitor once per batch packet msg send (c…
1cb097a 
…ometbft#3382)

Small optimization to outbound packet gossip, I expect this to be a 1-2%
speedup to outbound packet gossip as is right now. Will test on mainnet
soon

This is safe as outbound packet gossip is single threaded per peer as is
right now. Technically makes the send monitor marginally less real time,
but this is irrelevant as the send monitor works on 20ms sliding windows
anyway

---

- [ ] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Co-authored-by: Daniel <daniel.cason@informal.systems>
(cherry picked from commit 20d8630)
melekes pushed a commit that referenced this pull request Jul 5, 2024
@mergify @ValarDragon
perf(p2p): Only update send monitor once per batch packet msg send (b…
4e85725 
…ackport #3382) (#3417)

Small optimization to outbound packet gossip, I expect this to be a 1-2%
speedup to outbound packet gossip as is right now. Will test on mainnet
soon

This is safe as outbound packet gossip is single threaded per peer as is
right now. Technically makes the send monitor marginally less real time,
but this is irrelevant as the send monitor works on 20ms sliding windows
anyway

---

#### PR checklist

- [ ] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3382 done by
[Mergify](https://mergify.com).

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
@melekes
Copy link
Contributor
melekes commented Jul 10, 2024

@mergify backport v1.x

@mergify Mergify
Copy link
Contributor
mergify bot commented Jul 10, 2024 •
edited
Loading

backport v1.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 10, 2024
@ValarDragon @melekes
@cason @mergify
perf(p2p): Only update send monitor once per batch packet msg send (#…
9e0a10a 
…3382)

Small optimization to outbound packet gossip, I expect this to be a 1-2%
speedup to outbound packet gossip as is right now. Will test on mainnet
soon

This is safe as outbound packet gossip is single threaded per peer as is
right now. Technically makes the send monitor marginally less real time,
but this is irrelevant as the send monitor works on 20ms sliding windows
anyway

---

#### PR checklist

- [ ] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Co-authored-by: Daniel <daniel.cason@info
9E81
rmal.systems>
(cherry picked from commit 20d8630)
@mergify mergify bot mentioned this pull request Jul 10, 2024
Merged
4 tasks
melekes added a commit that referenced this pull request Jul 10, 2024
@mergify @ValarDragon @melekes
perf(p2p): Only update send monitor once per batch packet msg send (b…
63d437c 
…ackport #3382) (#3487)

Small optimization to outbound packet gossip, I expect this to be a 1-2%
speedup to outbound packet gossip as is right now. Will test on mainnet
soon

This is safe as outbound packet gossip is single threaded per peer as is
right now. Technically makes the send monitor marginally less real time,
but this is irrelevant as the send monitor works on 20ms sliding windows
anyway

---

#### PR checklist

- [ ] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3382 done by
[Mergify](https://mergify.com).

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@faddat faddat mentioned this pull request Oct 15, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cason cason cason approved these changes

@melekes melekes melekes approved these changes

Assignees
No one assigned
Labels
backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ValarDragon @melekes @cason
0