feat(s2n-quic-transport): Adds stream batching functionality #2428
+192
−24
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
| @@ -35,6 +35,8 @@ const MAX_KEEP_ALIVE_PERIOD_DEFAULT: Duration = Duration::from_secs(30); | |||
| //# received. | |||
| pub const ANTI_AMPLIFICATION_MULTIPLIER: u8 = 3; | |||
|
|
|||
| pub const STREAM_BURST_SIZE: u8 = 1; | |||
There was a problem hiding this comment.
Suggested change
| pub const STREAM_BURST_SIZE: u8 = 1; | |
| pub const DEFAULT_STREAM_BURST_SIZE: u8 = 1; |
Comment on lines
+148
to
+149
| transmission_counter: u8, | ||
| transmission_limit: u8, |
There was a problem hiding this comment.
I think there might be a better place for these, since this struct is containing only lists
| @@ -206,7 +264,7 @@ impl<S: StreamTrait> InterestLists<S> { | |||
| waiting_for_frame_delivery_link, | |||
| waiting_for_frame_delivery | |||
| ); | |||
| sync_interests!( | |||
| sync_trans_interests!( | |||
There was a problem hiding this comment.
I'm wondering if it would be more intuitive to have the batching logic in stream_container::iterate_transmission_list rather in update_interests
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:
N/A
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.
Call-outs:
I'm not exactly pleased with the code as-is. Every interest list calls the update_interest() function, which means that if I want to change the sending behavior of only the transmission list, I need a lot of branches. I think a better solution would be to pull the transmission code out into its own codepath. But with this iteration I am trying to keep the same codepaths.
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.