feat(s2n-quic-transport): Adds stream batching functionality #2433
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
camshaft
reviewed
Jan 2, 2025
| /// | ||
| /// The `stream::Controller` will be notified of streams that have been | ||
| /// closed to allow for further streams to be opened. | ||
| #[cfg(test)] | ||
| pub fn iterate_transmission_list<F>(&mut self, controller: &mut stream::Controller, mut func: F) |
There was a problem hiding this comment.
is there a reason we need to keep this and not just merge the two?
There was a problem hiding this comment.
There are two reasons to iterate over the transmission list currently. One is to send on each stream node in the transmission list. The other is to read the order of the transmission list (used only in testing).
Basically I now need to distinguish these two codepaths, because when we're reading over the list, we don't want to increment the transmission counter. Obviously we do want that behavior when sending on the list. So the two separate codepaths are now iterate_transmission_list, which is only used for tests, and send_on_transmission_list, which is what contains the counting logic.
camshaft
reviewed
Jan 4, 2025
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.
Release Summary:
Adds stream batching functionality to s2n-quic sending behavior. Stream batching is a sending strategy which provides each stream with the opportunity to fill up a packet "batch-size" times, before then passing that priority to the next stream.
Resolved issues:
Previous iteration of this PR: #2428
Description of changes:
Adds a way for users to turn on stream batching. The current sending strategy can be described as stream batching with a size of 1 (this can be seen with the fact that all current tests pass with the default steam batch size of 1). With this change we can express batch sizes > 1.
Essentially now the transmission list keeps track of how many times its head node gets to be at the front of the list. The head node is popped off the list and moved to the back of the list once the limit is reached. I had to extract the sending behavior out of the iterate_interruptable macro because we only want this behavior when we're trying to send on the transmission list.
Call-outs:
Testing:
Includes a unit test of the batching feature.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.