-
Notifications
You must be signed in to change notification settings - Fork 566
Added new QUIC_STREAM_EVENT: QUIC_STREAM_EVENT_COPIED_TO_FRAME #5094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…tten from SEND_REQUEST to frame Signed-off-by: HHN <[email protected]>
@microsoft-github-policy-service agree |
My preference here would be to not expose this, and instead implement the deadline scheduling logic inside MsQuic. This exposes too much internals IMO which can cause perf issues (very frequent callbacks) as well as fragility (what if we offload this layer to HW in the future?). |
context: this feature request based on a requirement for deadline bounded delivery of objects
I would love to see that but, but don't you agree it is a very niche request which would only make sense for real time applications which can afford to loose some objects?
I have modelled it very closely to
Yes, that was a concern I had too that is why we only trigger the event once unconditionally. After that it is upto the application to decide if it wants to nuke transport layer performance, I believe we can give the application that freedom. Also the default behaviour if the application does not handle the callback is that it never gets called again, so existing MsQuic applications will not see much performance regression. I am open to the idea of disabling event unless a flag is set in
What is the fragility in question here? |
If we don't completely implement the deadline feature in MsQuic, I'm wondering if instead we should use some variation of That being said, I suspect many real-time application will have a need for deadline aware scheduling. So it might be worth adding to MsQuic. |
My only concern with a deadline timeout solution within MsQuic is the possibility of mis-use, I need it for timeouts of the range of 100ms |
To motivate this PR, let me phrase it a bit differently We add a feature to let the user request for an event once x bytes of a certain SEND_REQUEST has been written to a frame. mvfst (another implementation of QUIC) does have a similar feature
as quoted by a maintainer of mvfst over here I believe the API for the feature can be found over here. |
What kind of mis-use are you worried about exactly? So what if they set a deadline of 10 seconds for a 1 GB upload? |
@@ -1521,6 +1521,7 @@ typedef enum QUIC_STREAM_EVENT_TYPE { | |||
QUIC_STREAM_EVENT_IDEAL_SEND_BUFFER_SIZE = 8, | |||
QUIC_STREAM_EVENT_PEER_ACCEPTED = 9, | |||
QUIC_STREAM_EVENT_CANCEL_ON_LOSS = 10, | |||
QUIC_STREAM_EVENT_COPIED_TO_FRAME = 11, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to go forward with this model, and I'm not saying we would, this new event MUST be opt-in. We cannot deliver it to old clients that have no idea about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you suggest opt in at compile time or runtime?
I was thinking of having a new STREAM_SEND flag and delivery QUIC_STREAM_EVENT_COPIED_TO_FRAME events only if that flag is set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put this under the #PREVIEW_FEATURES macro.
for a deadline of 100ms, it'd be as simple as checking the current bandwidth and every time data from the stream is being written onto a frame, check if it can be transmitted within the deadline. If we believe we can't simply reset the stream because the deadline is small enough that we do not expect the network conditions to suddenly improve drastically. Now for a 1GB upload over 10 seconds, we need a wider context of how the network has been behaving and predict longer into the future how the network will behave, if I am right congestion control algorithms only try to predict 2-3 seconds into the future, we also need more tolerance for adverse network conditions as there is a better chance network conditions can drastically improve. I have a feeling a simple model would not be the best for all use cases and it might benefit to let the application decide what is best. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5094 +/- ##
==========================================
- Coverage 87.00% 86.05% -0.95%
==========================================
Files 59 59
Lines 18035 18047 +12
==========================================
- Hits 15692 15531 -161
- Misses 2343 2516 +173 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Right now there is no way of knowing when the stream writes data into frames, we add a new event which triggers when data from a SEND_REQUEST is written to a QUIC frame.
If we signal this event every time data is written to a frame, there would be severe performance penalties as we would keep calling the associated callback. Hence we signal the event unconditionally only the first time data from a given SEND_REQUEST is written to a frame and in the event the application must set a value (
BytesCopiedBeforeNextEvent
) which indicates how many more bytes from this send request need to be written into frames before the nextQUIC_STREAM_EVENT_COPIED_TO_FRAME
event is triggered. By default (if the application does not set theBytesCopiedBeforeNextEvent
parameter) it is set to a very high value effectively disabling this event.Possible Issues: No way to
BytesCopiedBeforeNextEvent
from outside the event effectively forcing the application to decide when it wants the nextQUIC_STREAM_EVENT_COPIED_TO_FRAME
when the event is called.Testing
Very basic test written, have not executed it hoping CI checks it for me
Documentation
Yes, documentation updated in
QUIC_STREAM_EVENT.md
Appropriate comments added in source