Bound gossip channels #5104
Open
Bound gossip channels #5104
+60
−14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpubot
requested review from
behzadnouri,
gregcusack,
jeffwashington and
alessandrod
|
pr looks good but i am running some tests to get a better understanding of the behavior |
gregcusack
reviewed
Mar 1, 2025
| @@ -71,6 +72,7 @@ impl StreamerReceiveStats { | |||
| packet_batches_count: AtomicUsize::default(), | |||
| full_packet_batches_count: AtomicUsize::default(), | |||
| max_channel_len: AtomicUsize::default(), | |||
| num_packets_dropped: AtomicUsize::default(), | |||
There was a problem hiding this comment.
side note: is it time to start reporting StreamerReceiveStats? or is there a reason we don't actually report these stats.
There was a problem hiding this comment.
I can certainly include stat reporting in this PR.
As far as I can tell, when the stats component of streamer was added, reporting of those stats was not added to all usage sites, presumably to satisfy type checking of that change without making assumptions about reporting. I.e., I don't think we're deliberately not reporting stats
gregcusack
reviewed
Mar 1, 2025
cpubot
force-pushed
the
bound-gossip-channels
branch
from
9c85fbd to
cb9bae8
Compare
|
+1 from me |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The
unboundedchannel uses what is effectively a linked list of "blocks" of 31 slots. As volume surges, it has to dynamically allocate additional blocks to accommodate the extra slots required. It does this on demand, one-by-one, which adds extra synchronization overhead on the sending and receiving side in addition to the time waiting for allocation / deallocation -- this slows down the entire pipeline of packet processing. Contrast this to aboundedchannel, which uses a single contiguous allocation without any of the block synchronization overhead.Summary of Changes
Gossip packet buffers now used
boundedchannels, with a fixed, preallocated, capacity.It was observed that under extreme load, the channel caps out at around 11k packet batches. The channel capacity introduced here is
16,384(2^14) packet batches, which is the 11k figure rounded up to the next power of 2. This ensures that load shedding is highly unlikely to occur on the sender and continues to be done on the receiver side.Senders are also updated to use
try_sendand introduce error handling in the event that channels are full. It is highly unlikely that these errors are actually hit in practice given the large capacity of the channels and the draining that is occurring in the receivers. This adjustment is made here for correctness and completeness, and also dovetail with forthcoming updates to load shed at the send site.To reiterate, there is no practical change to current load shedding behavior in this branch, save for edge cases that were never observed in load testing and are highly unlikely to be encountered.
Broken out of #5065