perf(consensus): Use TrySend for hasVote/HasBlockPart messages by ValarDragon · Pull Request #3407 · cometbft/cometbft

ValarDragon · 2024-07-03T10:25:14Z

The broadcast routine in its entirely is the source of ~75% of all created channels in the system, all coming from the NewTimer call in Send. (and 75% of all NewTimer) Furthermore, the NewTimer call from Broadcast is 2% of all CPU time across the system. However I think this is actually more impact, due to the following.

We spend 11% of all CPU time in the scheduler, and 4% of all CPU time is CheckTimers in the scheduler logic. I'd expect reducing most of the Timer calls reduces this. We also reduce total mallocgc time here by 5% by removing NewTimer, which should hopefully reduce GC load as well.

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 · 2024-07-03T10:37:56Z

Going to test this in osmosis prod to see the scheduler improvement, mostly because I'm curious if it reflects my understanding.

The test I added tests TryBroadcast to same extent Broadcast is tested within switch. (I didn't see any other test)

ValarDragon · 2024-07-03T10:53:10Z

I fixed the old Go Benchmark (It hasn't been working for months).

BenchmarkSwitchBroadcast-12    	  347770	      4304 ns/op	    1238 B/op	      16 allocs/op
BenchmarkSwitchTryBroadcast-12     2268736	       482.7 ns/op	     381 B/op	      11 allocs/op

So a significant CPU cost reduction!

…metbft into dev/use_trysend_for_hasXmsgs

ValarDragon · 2024-07-03T14:15:28Z

This appears to have been an empiric 20% reduction in scheduler time! (At consensus state and mempool times being roughly equal)

And a 5-10% GC time reduction!

cason · 2024-07-03T19:57:27Z

I will take a detailed look maybe tomorrow, but conceptually I agree, in particular because missing those two messages is not a problem: they are optimizations, preventing duplication, they are not core messages of the protocol.

…ft#3407

cason

Great!

Please fix the changelog before committing.

.changelog/unreleased/improvements/3407-make-hasvote-broadcast-use-trysend.md

cason · 2024-07-05T07:56:46Z

.changelog/unreleased/improvements/3407-make-hasvote-broadcast-use-trysend.md

+    use TrySend instead of Send. This saves notable amounts of performance,
+    as TrySend just drops sending when the channel is full, instead of creating
+    a timer / new channel.
+  ([\#3342](https://github.com/cometbft/cometbft/issues/3342))


Is this link right? #3342 is something else.

p2p/switch.go

hvanz

🚀

…-use-trysend.md Co-authored-by: Daniel <daniel.cason@informal.systems>

Co-authored-by: Daniel <daniel.cason@informal.systems>

…-use-trysend.md Co-authored-by: Daniel <daniel.cason@informal.systems>

.changelog/unreleased/improvements/3407-make-hasvote-broadcast-use-trysend.md

…-use-trysend.md Co-authored-by: Daniel <daniel.cason@informal.systems>

melekes

Thanks @ValarDragon ❤️

p2p/switch.go

.changelog/unreleased/improvements/3407-make-hasvote-broadcast-use-trysend.md

melekes · 2024-07-09T08:47:24Z

--- FAIL: TestStateOversizedBlock (0.38s)
    --- FAIL: TestStateOversizedBlock/max_size,_correct_block (0.38s)
        state_test.go:276: Detected real max data size for block; size 65255 softMaxDataBytes 64805
        state_test.go:307: Block Sizes; Limit 65536 Current 65535
        state_test.go:308: Proposal Parts; Maximum 1 Current 1
panic: Timeout expired while waiting for NewVote event [recovered]
	panic: Timeout expired while waiting for NewVote event

goroutine 37363 [running]:
testing.tRunner.func1.2({0x1359020, 0x175c150})
	/opt/hostedtoolcache/go/1.22.5/x64/src/testing/testing.go:1631 +0x3f7
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.22.5/x64/src/testing/testing.go:1634 +0x6b6
panic({0x1359020?, 0x175c150?})
	/opt/hostedtoolcache/go/1.22.5/x64/src/runtime/panic.go:770 +0x132
github.com/cometbft/cometbft/internal/consensus.ensureVote(0xc008015b00, 0x1, 0x1, 0x2)
	/home/runner/work/cometbft/cometbft/internal/consensus/common_test.go:740 +0x47e
github.com/cometbft/cometbft/internal/consensus.ensurePrecommit(...)
	/home/runner/work/cometbft/cometbft/internal/consensus/common_test.go:728
github.com/cometbft/cometbft/internal/consensus.TestStateOversizedBlock.func1(0xc004f48d00)
	/home/runner/work/cometbft/cometbft/internal/consensus/state_test.go:331 +0xcec
testing.tRunner(0xc004f48d00, 0xc0031f2300)
	/opt/hostedtoolcache/go/1.22.5/x64/src/testing/testing.go:1689 +0x21f
created by testing.(*T).Run in goroutine 37362
	/opt/hostedtoolcache/go/1.22.5/x64/src/testing/testing.go:1742 +0x826
FAIL	github.com/cometbft/cometbft/internal/consensus	20.370s
FAIL

ValarDragon · 2024-07-09T12:11:16Z

I'm at conferences this week, I can work on debugging on weekend! Sorry for delay on replies

cason · 2024-07-09T14:48:51Z

This error is odd, are you sure it was introduced by this PR?

cason · 2024-07-09T14:49:21Z

I mean, TestStateOversizedBlock don't use use a reactor.

melekes · 2024-07-09T16:34:55Z

This error is odd, are you sure it was introduced by this PR?

not sure

Closes #3151 The broadcast routine in its entirely is the source of ~75% of all created channels in the system, all coming from the `NewTimer` call in Send. (and 75% of all NewTimer) Furthermore, the NewTimer call from Broadcast is 2% of all CPU time across the system. However I think this is actually more impact, due to the following. We spend 11% of all CPU time in the scheduler, and 4% of all CPU time is `CheckTimers` in the scheduler logic. I'd expect reducing most of the Timer calls reduces this. We also reduce total `mallocgc` time here by 5% by removing NewTimer, which should hopefully reduce GC load as well. --- #### PR checklist - [x] Tests written/updated - [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: Daniel <daniel.cason@informal.systems> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> (cherry picked from commit 55493e0) # Conflicts: # internal/consensus/reactor.go # p2p/switch.go # p2p/switch_test.go

…ort #3407) (#3466) Closes #3151 The broadcast routine in its entirely is the source of ~75% of all created channels in the system, all coming from the `NewTimer` call in Send. (and 75% of all NewTimer) Furthermore, the NewTimer call from Broadcast is 2% of all CPU time across the system. However I think this is actually more impact, due to the following. We spend 11% of all CPU time in the scheduler, and 4% of all CPU time is `CheckTimers` in the scheduler logic. I'd expect reducing most of the Timer calls reduces this. We also reduce total `mallocgc` time here by 5% by removing NewTimer, which should hopefully reduce GC load as well. --- #### PR checklist - [x] Tests written/updated - [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 #3407 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>

…ort cometbft#3407) (cometbft#3466) Closes cometbft#3151 The broadcast routine in its entirely is the source of ~75% of all created channels in the system, all coming from the `NewTimer` call in Send. (and 75% of all NewTimer) Furthermore, the NewTimer call from Broadcast is 2% of all CPU time across the system. However I think this is actually more impact, due to the following. We spend 11% of all CPU time in the scheduler, and 4% of all CPU time is `CheckTimers` in the scheduler logic. I'd expect reducing most of the Timer calls reduces this. We also reduce total `mallocgc` time here by 5% by removing NewTimer, which should hopefully reduce GC load as well. --- - [x] Tests written/updated - [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#3407 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 #3151 The broadcast routine in its entirely is the source of ~75% of all created channels in the system, all coming from the `NewTimer` call in Send. (and 75% of all NewTimer) Furthermore, the NewTimer call from Broadcast is 2% of all CPU time across the system. However I think this is actually more impact, due to the following. We spend 11% of all CPU time in the scheduler, and 4% of all CPU time is `CheckTimers` in the scheduler logic. I'd expect reducing most of the Timer calls reduces this. We also reduce total `mallocgc` time here by 5% by removing NewTimer, which should hopefully reduce GC load as well. --- - [x] Tests written/updated - [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: Daniel <daniel.cason@informal.systems> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

Use TrySend for has X messages

cb57698

ValarDragon requested a review from a team as a code owner July 3, 2024 10:25

ValarDragon requested a review from a team July 3, 2024 10:25

ValarDragon added 2 commits July 3, 2024 11:35

Add changelog

d660846

Update test

9d3047c

ValarDragon and others added 7 commits July 3, 2024 11:53

Merge branch 'main' into dev/use_trysend_for_hasXmsgs

1d34fdf

Fix benchmarks

210e000

Merge branch 'dev/use_trysend_for_hasXmsgs' of github.com:cometbft/co…

5a1886f

…metbft into dev/use_trysend_for_hasXmsgs

Better var name

0315261

Fix lint

d1d3b20

Correct more code comments

6a338d7

Fix lint

c8a775c

itsdevbear pushed a commit to berachain/cometbft that referenced this pull request Jul 4, 2024

perf(consensus): Use TrySend for hasVote/HasBlockPart messages cometb…

d3e09a7

…ft#3407

cason approved these changes Jul 5, 2024

View reviewed changes

hvanz approved these changes Jul 5, 2024

View reviewed changes

ValarDragon and others added 4 commits July 5, 2024 22:37

Update .changelog/unreleased/improvements/3407-make-hasvote-broadcast…

2d9b52a

…-use-trysend.md Co-authored-by: Daniel <daniel.cason@informal.systems>

Update .changelog/unreleased/improvements/3407-make-hasvote-broadcast…

4146725

…-use-trysend.md Co-authored-by: Daniel <daniel.cason@informal.systems>

Update p2p/switch.go

66714dc

Co-authored-by: Daniel <daniel.cason@informal.systems>

Update .changelog/unreleased/improvements/3407-make-hasvote-broadcast…

fa2a1e5

…-use-trysend.md Co-authored-by: Daniel <daniel.cason@informal.systems>

cason reviewed Jul 8, 2024

View reviewed changes

.changelog/unreleased/improvements/3407-make-hasvote-broadcast-use-trysend.md Outdated Show resolved Hide resolved

Update .changelog/unreleased/improvements/3407-make-hasvote-broadcast…

65909cc

…-use-trysend.md Co-authored-by: Daniel <daniel.cason@informal.systems>

melekes approved these changes Jul 8, 2024

View reviewed changes

p2p/switch.go Outdated Show resolved Hide resolved

.changelog/unreleased/improvements/3407-make-hasvote-broadcast-use-trysend.md Outdated Show resolved Hide resolved

.changelog/unreleased/improvements/3407-make-hasvote-broadcast-use-trysend.md Outdated Show resolved Hide resolved

melekes added 2 commits July 9, 2024 12:36

remove debug calls

c821b85

Merge branch 'main' into dev/use_trysend_for_hasXmsgs

c08c151

melekes enabled auto-merge July 9, 2024 08:38

Merge branch 'main' into dev/use_trysend_for_hasXmsgs

c6bf893

melekes added consensus backport-to-v1.x p2p labels Jul 10, 2024

melekes disabled auto-merge July 10, 2024 06:14

melekes added this pull request to the merge queue Jul 10, 2024

Merged via the queue into main with commit 55493e0 Jul 10, 2024

melekes deleted the dev/use_trysend_for_hasXmsgs branch July 10, 2024 06:35

mergify bot mentioned this pull request Jul 10, 2024

perf(consensus): Use TrySend for hasVote/HasBlockPart messages (backport #3407) #3466

Merged

4 tasks

Conversation

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

Uh oh!

PR checklist

Uh oh!

ValarDragon commented Jul 3, 2024

Uh oh!

ValarDragon commented Jul 3, 2024

Uh oh!

ValarDragon commented Jul 3, 2024

Uh oh!

cason commented Jul 3, 2024

Uh oh!

cason left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cason Jul 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hvanz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

melekes commented Jul 9, 2024

Uh oh!

ValarDragon commented Jul 9, 2024

Uh oh!

cason commented Jul 9, 2024

Uh oh!

cason commented Jul 9, 2024

Uh oh!

melekes commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ValarDragon commented Jul 3, 2024 •

edited

Loading