From ab985f12bcc225ee5963867538a04fa577b8bd69 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 10 Jul 2024 15:04:41 +0400 Subject: [PATCH] perf(p2p): Remove broadcast return channel (backport #3182) (#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 #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) --- - [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
This is an automatic backport of pull request #3182 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha Co-authored-by: Anton Kaliaev --- ...-remove-switch-broadcast-return-channel.md | 2 ++ p2p/switch.go | 23 +++---------------- p2p/switch_test.go | 13 +---------- 3 files changed, 6 insertions(+), 32 deletions(-) create mode 100644 .changelog/v0.38.8/improvements/3182-remove-switch-broadcast-return-channel.md diff --git a/.changelog/v0.38.8/improvements/3182-remove-switch-broadcast-return-channel.md b/.changelog/v0.38.8/improvements/3182-remove-switch-broadcast-return-channel.md new file mode 100644 index 0000000000..a3c043f5bf --- /dev/null +++ b/.changelog/v0.38.8/improvements/3182-remove-switch-broadcast-return-channel.md @@ -0,0 +1,2 @@ +- `[p2p]` Remove `Switch#Broadcast` unused return channel + ([\#3182](https://github.com/cometbft/cometbft/pull/3182)) diff --git a/p2p/switch.go b/p2p/switch.go index 68ad5669b3..988e0f268f 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -3,7 +3,6 @@ package p2p import ( "fmt" "math" - "sync" "time" "github.com/cosmos/gogoproto/proto" @@ -266,33 +265,17 @@ func (sw *Switch) OnStop() { // Peers // Broadcast runs a go routine for each attempted send, which will block trying -// to send for defaultSendTimeoutSeconds. Returns a channel which receives -// success values for each attempted send (false if times out). Channel will be -// closed once msg bytes are sent to all peers (or time out). +// to send for defaultSendTimeoutSeconds. // // NOTE: Broadcast uses goroutines, so order of broadcast may not be preserved. -func (sw *Switch) Broadcast(e Envelope) chan bool { - sw.Logger.Debug("Broadcast", "channel", e.ChannelID) - +func (sw *Switch) Broadcast(e Envelope) { peers := sw.peers.List() - var wg sync.WaitGroup - wg.Add(len(peers)) - successChan := make(chan bool, len(peers)) - for _, peer := range peers { go func(p Peer) { - defer wg.Done() success := p.Send(e) - successChan <- success + _ = success }(peer) } - - go func() { - wg.Wait() - close(successChan) - }() - - return successChan } // NumPeers returns the count of outbound/inbound and outbound-dialing peers. diff --git a/p2p/switch_test.go b/p2p/switch_test.go index ad4040760f..1aa1a5dc15 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -840,22 +840,11 @@ func BenchmarkSwitchBroadcast(b *testing.B) { b.ResetTimer() - numSuccess, numFailure := 0, 0 - // Send random message from foo channel to another for i := 0; i < b.N; i++ { chID := byte(i % 4) - successChan := s1.Broadcast(Envelope{ChannelID: chID}) - for s := range successChan { - if s { - numSuccess++ - } else { - numFailure++ - } - } + s1.Broadcast(Envelope{ChannelID: chID}) } - - b.Logf("success: %v, failure: %v", numSuccess, numFailure) } func TestSwitchRemovalErr(t *testing.T) {