perf(consensus): Run broadcast routines out of process by ValarDragon · Pull Request #3180 · cometbft/cometbft

ValarDragon · 2024-06-04T13:59:39Z

Run broadcast routines out of process. Right now each broadcast routine blocks the consensus mutex for roughly num_peers * process_creation_time, which is genuinely notable! This PR reduces the consensus blocking overhead to just be process_creation_time.

On the latest osmosis branch with improvements, thats 20s of blocking time out of 140s (over the course of 1 hour. This 140s includes block execution!)

Note that WAL write time should go significantly down with open PR's. For HasVote, this is a meaningful increase to consensus mutex lock time, so its worth reducing.

PR checklist

Tests written/updated - I can't think of any test to add
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments - I don't know of any related docs here
Title follows the Conventional Commits spec

melekes

Thanks @ValarDragon ❤️

perf(consensus): Run broadcast routines out of process cometbft#3180

…/pr-101 perf(consensus): Run broadcast routines out of process cometbft#3180 (backport #101)

Remove the return channel and waitgroup from switch.Broadcast as its unused. This was costing us some CPU overhead (2% of broadcast routine as it stands right now, but broadcast routine time should be dropping) and one extra goroutine. This removes more overhead than what #3180 may introduce. It also better highlights the point that maybe all broadcast usecases should consider using TrySend (And we potentially adapt channel buffers), ala the point in #3151 . (This would have significant system-wide impact, by significantly lowering the number of timers/scheduler overhead we have) --- #### PR checklist - [x] Tests written/updated - Simplifies code, the behavior being removed was never tested! - [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 --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

cason · 2024-06-11T15:47:54Z

So we are creating a Go routine to create num_peers Go routines to send a message to each peer? I am really impressed (and somehow suspicious) by the improvement this can provide.

ValarDragon · 2024-06-11T16:39:03Z

Yes! This was surprisingly costly to me as well, but this is what the CPU profiles were saying! Post this change, this fireEvent time is ~entirely removed

cason · 2024-06-12T10:29:35Z

Can't it be the contention of the for loop in the Switch() method? Because it has to retrieve the list of connected peers, and it is synchronized....

ValarDragon · 2024-06-14T17:48:05Z

No, this CPU profile is directly saying its runtime.NewProc and runtime.newObject, which is not based on mutex contentions

We just broadcast alot.

cason

Lets go with this.

I think the best solution is to improve the Broadcast implementation, but the benefits of this minimal change seem to be relevant.

melekes · 2024-07-10T09:36:43Z

@mergify backport v1.x

mergify · 2024-07-10T09:36:55Z

backport v1.x

✅ Backports have been created

Details

#3477 perf(consensus): Run broadcast routines out of process (backport #3180) has been created for branch v1.x but encountered conflicts

Run broadcast routines out of process. Right now each broadcast routine blocks the consensus mutex for roughly `num_peers * process_creation_time`, which is genuinely notable! This PR reduces the consensus blocking overhead to just be `process_creation_time`. On the latest osmosis branch with improvements, thats 20s of blocking time out of 140s (over the course of 1 hour. This 140s includes block execution!) ![image](https://github.com/cometbft/cometbft/assets/6440154/4c202988-a0d1-460e-89bc-7c1be11fd36f) Note that WAL write time should go significantly down with open PR's. For `HasVote`, this is a meaningful increase to consensus mutex lock time, so its worth reducing. --- #### PR checklist - [x] Tests written/updated - I can't think of any test to add - [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 - I don't know of any related docs here - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 110817b) # Conflicts: # .changelog/unreleased/improvements/3180-lower-broadcasts-consensus-overhead.md # internal/consensus/reactor.go

Remove the return channel and waitgroup from switch.Broadcast as its unused. This was costing us some CPU overhead (2% of broadcast routine as it stands right now, but broadcast routine time should be dropping) and one extra goroutine. This removes more overhead than what #3180 may introduce. It also better highlights the point that maybe all broadcast usecases should consider using TrySend (And we potentially adapt channel buffers), ala the point in #3151 . (This would have significant system-wide impact, by significantly lowering the number of timers/scheduler overhead we have) --- #### PR checklist - [x] Tests written/updated - Simplifies code, the behavior being removed was never tested! - [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 --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> (cherry picked from commit e731a3f) # Conflicts: # p2p/switch.go # p2p/switch_test.go

… (#3477) Run broadcast routines out of process. Right now each broadcast routine blocks the consensus mutex for roughly `num_peers * process_creation_time`, which is genuinely notable! This PR reduces the consensus blocking overhead to just be `process_creation_time`. On the latest osmosis branch with improvements, thats 20s of blocking time out of 140s (over the course of 1 hour. This 140s includes block execution!) ![image](https://github.com/cometbft/cometbft/assets/6440154/4c202988-a0d1-460e-89bc-7c1be11fd36f) Note that WAL write time should go significantly down with open PR's. For `HasVote`, this is a meaningful increase to consensus mutex lock time, so its worth reducing. --- #### PR checklist - [x] Tests written/updated - I can't think of any test to add - [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 - I don't know of any related docs here - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3180 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>

Remove the return channel and waitgroup from switch.Broadcast as its unused. This was costing us some CPU overhead (2% of broadcast routine as it stands right now, but broadcast routine time should be dropping) and one extra goroutine. This removes more overhead than what #3180 may introduce. It also better highlights the point that maybe all broadcast usecases should consider using TrySend (And we potentially adapt channel buffers), ala the point in #3151 . (This would have significant system-wide impact, by significantly lowering the number of timers/scheduler overhead we have) --- #### PR checklist - [x] Tests written/updated - Simplifies code, the behavior being removed was never tested! - [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 #3182 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>

…ometbft#3480) Remove the return channel and waitgroup from switch.Broadcast as its unused. This was costing us some CPU overhead (2% of broadcast routine as it stands right now, but broadcast routine time should be dropping) and one extra goroutine. This removes more overhead than what cometbft#3180 may introduce. It also better highlights the point that maybe all broadcast usecases should consider using TrySend (And we potentially adapt channel buffers), ala the point in cometbft#3151 . (This would have significant system-wide impact, by significantly lowering the number of timers/scheduler overhead we have) --- - [x] Tests written/updated - Simplifies code, the behavior being removed was never tested! - [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#3182 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>

…ometbft#3480) (#134) Remove the return channel and waitgroup from switch.Broadcast as its unused. This was costing us some CPU overhead (2% of broadcast routine as it stands right now, but broadcast routine time should be dropping) and one extra goroutine. This removes more overhead than what cometbft#3180 may introduce. It also better highlights the point that maybe all broadcast usecases should consider using TrySend (And we potentially adapt channel buffers), ala the point in cometbft#3151 . (This would have significant system-wide impact, by significantly lowering the number of timers/scheduler overhead we have) --- - [x] Tests written/updated - Simplifies code, the behavior being removed was never tested! - [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#3182 done by [Mergify](https://mergify.com). --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

…tbft#3180) (cometbft#3477) Run broadcast routines out of process. Right now each broadcast routine blocks the consensus mutex for roughly `num_peers * process_creation_time`, which is genuinely notable! This PR reduces the consensus blocking overhead to just be `process_creation_time`. On the latest osmosis branch with improvements, thats 20s of blocking time out of 140s (over the course of 1 hour. This 140s includes block execution!) ![image](https://github.com/cometbft/cometbft/assets/6440154/4c202988-a0d1-460e-89bc-7c1be11fd36f) Note that WAL write time should go significantly down with open PR's. For `HasVote`, this is a meaningful increase to consensus mutex lock time, so its worth reducing. --- #### PR checklist - [x] Tests written/updated - I can't think of any test to add - [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 - I don't know of any related docs here - [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#3180 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>

…tbft#318… (#135) * perf(consensus): Run broadcast routines out of process (backport cometbft#3180) (cometbft#3477) Run broadcast routines out of process. Right now each broadcast routine blocks the consensus mutex for roughly `num_peers * process_creation_time`, which is genuinely notable! This PR reduces the consensus blocking overhead to just be `process_creation_time`. On the latest osmosis branch with improvements, thats 20s of blocking time out of 140s (over the course of 1 hour. This 140s includes block execution!) ![image](https://github.com/cometbft/cometbft/assets/6440154/4c202988-a0d1-460e-89bc-7c1be11fd36f) Note that WAL write time should go significantly down with open PR's. For `HasVote`, this is a meaningful increase to consensus mutex lock time, so its worth reducing. --- #### PR checklist - [x] Tests written/updated - I can't think of any test to add - [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 - I don't know of any related docs here - [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#3180 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> * add back has vote message broadcast --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

Remove the return channel and waitgroup from switch.Broadcast as its unused. This was costing us some CPU overhead (2% of broadcast routine as it stands right now, but broadcast routine time should be dropping) and one extra goroutine. This removes more overhead than what #3180 may introduce. It also better highlights the point that maybe all broadcast usecases should consider using TrySend (And we potentially adapt channel buffers), ala the point in #3151 . (This would have significant system-wide impact, by significantly lowering the number of timers/scheduler overhead we have) --- #### PR checklist - [x] Tests written/updated - Simplifies code, the behavior being removed was never tested! - [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 --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

Run broadcast routines out of process. Right now each broadcast routine blocks the consensus mutex for roughly `num_peers * process_creation_time`, which is genuinely notable! This PR reduces the consensus blocking overhead to just be `process_creation_time`. On the latest osmosis branch with improvements, thats 20s of blocking time out of 140s (over the course of 1 hour. This 140s includes block execution!) ![image](https://github.com/cometbft/cometbft/assets/6440154/4c202988-a0d1-460e-89bc-7c1be11fd36f) Note that WAL write time should go significantly down with open PR's. For `HasVote`, this is a meaningful increase to consensus mutex lock time, so its worth reducing. --- - [x] Tests written/updated - I can't think of any test to add - [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 - I don't know of any related docs here - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

Remove the return channel and waitgroup from switch.Broadcast as its unused. This was costing us some CPU overhead (2% of broadcast routine as it stands right now, but broadcast routine time should be dropping) and one extra goroutine. This removes more overhead than what #3180 may introduce. It also better highlights the point that maybe all broadcast usecases should consider using TrySend (And we potentially adapt channel buffers), ala the point in #3151 . (This would have significant system-wide impact, by significantly lowering the number of timers/scheduler overhead we have) --- #### PR checklist - [x] Tests written/updated - Simplifies code, the behavior being removed was never tested! - [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 --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

Run broadcast routines out of process. Right now each broadcast routine blocks the consensus mutex for roughly `num_peers * process_creation_time`, which is genuinely notable! This PR reduces the consensus blocking overhead to just be `process_creation_time`. On the latest osmosis branch with improvements, thats 20s of blocking time out of 140s (over the course of 1 hour. This 140s includes block execution!) ![image](https://github.com/cometbft/cometbft/assets/6440154/4c202988-a0d1-460e-89bc-7c1be11fd36f) Note that WAL write time should go significantly down with open PR's. For `HasVote`, this is a meaningful increase to consensus mutex lock time, so its worth reducing. --- - [x] Tests written/updated - I can't think of any test to add - [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 - I don't know of any related docs here - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

Run broadcast routines out of process

89dbc8b

ValarDragon requested a review from a team as a code owner June 4, 2024 13:59

ValarDragon requested a review from a team June 4, 2024 13:59

Fix bug, add Changelog

dd0278e

ValarDragon mentioned this pull request Jun 4, 2024

perf(p2p): Remove broadcast return channel #3182

Merged

4 tasks

melekes approved these changes Jun 5, 2024

View reviewed changes

ValarDragon added a commit to osmosis-labs/cometbft that referenced this pull request Jun 5, 2024

Merge pull request #101 from osmosis-labs/bp/3180

c59619d

perf(consensus): Run broadcast routines out of process cometbft#3180

ValarDragon added a commit to osmosis-labs/cometbft that referenced this pull request Jun 5, 2024

Merge pull request #103 from osmosis-labs/mergify/bp/osmo-v25/v0.37.4…

c6c4ecc

…/pr-101 perf(consensus): Run broadcast routines out of process cometbft#3180 (backport #101)

cason mentioned this pull request Jun 11, 2024

[Tracking Issue] Performance improvements in consensus implementation #3245

Open

14 tasks

cason approved these changes Jun 19, 2024

View reviewed changes

cason mentioned this pull request Jun 19, 2024

[Tracking issue] p2p connection optimizations #3053

Open

24 tasks

melekes added this pull request to the merge queue Jun 19, 2024

Merged via the queue into main with commit 110817b Jun 19, 2024

melekes deleted the dev/make_broadcast_routines_out_of_process branch June 19, 2024 14:33

mergify bot mentioned this pull request Jul 10, 2024

perf(consensus): Run broadcast routines out of process (backport #3180) #3477

Merged

4 tasks

mergify bot mentioned this pull request Jul 10, 2024

perf(p2p): Remove broadcast return channel (backport #3182) #3480

Merged

4 tasks

ValarDragon mentioned this pull request Aug 19, 2024

perf(p2p): Remove broadcast return channel (backport #3182) (#3480) osmosis-labs/cometbft#134

Merged

7 tasks

ValarDragon mentioned this pull request Aug 19, 2024

perf(consensus): Run broadcast routines out of process (backport #318… osmosis-labs/cometbft#135

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(consensus): Run broadcast routines out of process#3180

perf(consensus): Run broadcast routines out of process#3180
melekes merged 2 commits intomainfrom
dev/make_broadcast_routines_out_of_process

ValarDragon commented Jun 4, 2024 •

edited

Loading

Uh oh!

melekes left a comment

Uh oh!

cason commented Jun 11, 2024

Uh oh!

ValarDragon commented Jun 11, 2024

Uh oh!

cason commented Jun 12, 2024

Uh oh!

ValarDragon commented Jun 14, 2024 •

edited

Loading

Uh oh!

cason left a comment

Uh oh!

melekes commented Jul 10, 2024

Uh oh!

mergify bot commented Jul 10, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ValarDragon commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

cason commented Jun 11, 2024

Uh oh!

ValarDragon commented Jun 11, 2024

Uh oh!

cason commented Jun 12, 2024

Uh oh!

ValarDragon commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cason left a comment

Choose a reason for hiding this comment

Uh oh!

melekes commented Jul 10, 2024

Uh oh!

mergify bot commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ValarDragon commented Jun 4, 2024 •

edited

Loading

ValarDragon commented Jun 14, 2024 •

edited

Loading

mergify bot commented Jul 10, 2024 •

edited

Loading