-
Notifications
You must be signed in to change notification settings - Fork 430
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.
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
perf: Speedup flowrate by removing time.Now() from every update call #3016
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ValarDragon ❤️
internal/flowrate/util.go
Outdated
// sleep, then increment the clock value | ||
// This is the only place the clock value is updated. | ||
// Ensures that the clock value starts at 0. | ||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how I feel about us not ever exiting here. I guess it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can use some more CAS instructions to make it exit if all timers are done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT didn't work
Not sure if I agree with this approach. Using an atomic int field to keep time at higher granularity is good. Using sleeps to increase it is not great, we can't trust that sleep sleeps what it is supposed to. An alternative is to use something like |
Good point 👍 |
Great point! Fixed in latest commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I think this PR is good to go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ometbft#3016) Closes cometbft#2958 Speeds up flowrate by removing time.Now() calls from every `clock` call, instead making one goroutine responsible for all clock reads. On my most recent mainnet benchmark, this should shave 14% off of the synchronous time in sending packets: ![image](https://github.com/cometbft/cometbft/assets/6440154/f2f65080-7bf1-4c4c-9775-774d97ed016a) There may be slight differences at the boundary of these 20ms windows, but I do not think that is important. --- - [x] Tests written/updated - existing tests pass - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
…ometbft#3016) Closes cometbft#2958 Speeds up flowrate by removing time.Now() calls from every `clock` call, instead making one goroutine responsible for all clock reads. On my most recent mainnet benchmark, this should shave 14% off of the synchronous time in sending packets: ![image](https://github.com/cometbft/cometbft/assets/6440154/f2f65080-7bf1-4c4c-9775-774d97ed016a) There may be slight differences at the boundary of these 20ms windows, but I do not think that is important. --- - [x] Tests written/updated - existing tests pass - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 8d51a10)
Should we backport anything from here into .37 or .38 ? |
Yes please, lets backport to 38! |
@mergify backport v1.x |
✅ Backports have been created
|
…3016) Closes #2958 Speeds up flowrate by removing time.Now() calls from every `clock` call, instead making one goroutine responsible for all clock reads. On my most recent mainnet benchmark, this should shave 14% off of the synchronous time in sending packets: ![image](https://github.com/cometbft/cometbft/assets/6440154/f2f65080-7bf1-4c4c-9775-774d97ed016a) There may be slight differences at the boundary of these 20ms windows, but I do not think that is important. --- #### PR checklist - [x] Tests written/updated - existing tests pass - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 4513c68) # Conflicts: # .changelog/unreleased/improvements/3016-remove-expensive-calls-from-flowrate.md
…(backport #3016) (#3467) Closes #2958 Speeds up flowrate by removing time.Now() calls from every `clock` call, instead making one goroutine responsible for all clock reads. On my most recent mainnet benchmark, this should shave 14% off of the synchronous time in sending packets: ![image](https://github.com/cometbft/cometbft/assets/6440154/f2f65080-7bf1-4c4c-9775-774d97ed016a) There may be slight differences at the boundary of these 20ms windows, but I do not think that is important. --- #### PR checklist - [x] Tests written/updated - existing tests pass - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] 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 #3016 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>
…(backport cometbft#3016) (cometbft#3467) Closes cometbft#2958 Speeds up flowrate by removing time.Now() calls from every `clock` call, instead making one goroutine responsible for all clock reads. On my most recent mainnet benchmark, this should shave 14% off of the synchronous time in sending packets: ![image](https://github.com/cometbft/cometbft/assets/6440154/f2f65080-7bf1-4c4c-9775-774d97ed016a) There may be slight differences at the boundary of these 20ms windows, but I do not think that is important. --- - [x] Tests written/updated - existing tests pass - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] 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 cometbft#3016 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>
Closes #2958
Speeds up flowrate by removing time.Now() calls from every
clock
call, instead making one goroutine responsible for all clock reads. On my most recent mainnet benchmark, this should shave 14% off of the synchronous time in sending packets:There may be slight differences at the boundary of these 20ms windows, but I do not think that is important.
PR checklist
.changelog
(we use unclog to manage our changelog)docs/
orspec/
) and code comments