Skip to content
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

[QUIC] Consider implementing send buffering on .NET side #73691

Closed
CarnaViire opened this issue Aug 10, 2022 · 3 comments
Closed

[QUIC] Consider implementing send buffering on .NET side #73691

CarnaViire opened this issue Aug 10, 2022 · 3 comments
Assignees
Labels
Milestone

Comments

@CarnaViire
Copy link
Member

We currently utilize send buffering on msquic side. Msquic will copy app buffer and return as soon as copy is done.

We had to (temporarily) introduce an additional copy in #72746 to overcome a too-early-user-buffer-release problem on abort. If we will not be able to solve this problem in a different way, it doesn't make sense to have 2x copying, so we should consider switching to app-level buffering.

Using our own send buffering would also allow to optimize the buffering for our usecases better than a general-case msquic buffering. We can also benefit from copying happening in parallel on managed threads in comparison to copying happening sequentially on msquic thread.

Note: in case of app level buffering, msquic will hold onto the app buffer and return only after ACK is received.

Note 2 from msquic docs: To fill the pipe in this mode, the app is responsible for keeping enough sends pending at all times to ensure the connection doesn't go idle. MsQuic indicates the amount of data the app should keep pending in the QUIC_STREAM_EVENT_IDEAL_SEND_BUFFER_SIZE event. The app should always have at least two sends pending at a time. If only a single send is used, the connection can go idle for the time between that send is completed and the new send is queued.

Some related discussions:
microsoft/msquic#1602 (comment)
#44782 (comment)

@CarnaViire CarnaViire added this to the 8.0.0 milestone Aug 10, 2022
@ghost
Copy link

ghost commented Aug 10, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

We currently utilize send buffering on msquic side. Msquic will copy app buffer and return as soon as copy is done.

We had to (temporarily) introduce an additional copy in #72746 to overcome a too-early-user-buffer-release problem on abort. If we will not be able to solve this problem in a different way, it doesn't make sense to have 2x copying, so we should consider switching to app-level buffering.

Using our own send buffering would also allow to optimize the buffering for our usecases better than a general-case msquic buffering. We can also benefit from copying happening in parallel on managed threads in comparison to copying happening sequentially on msquic thread.

Note: in case of app level buffering, msquic will hold onto the app buffer and return only after ACK is received.

Note 2 from msquic docs: To fill the pipe in this mode, the app is responsible for keeping enough sends pending at all times to ensure the connection doesn't go idle. MsQuic indicates the amount of data the app should keep pending in the QUIC_STREAM_EVENT_IDEAL_SEND_BUFFER_SIZE event. The app should always have at least two sends pending at a time. If only a single send is used, the connection can go idle for the time between that send is completed and the new send is queued.

Some related discussions:
microsoft/msquic#1602 (comment)
#44782 (comment)

Author: CarnaViire
Assignees: -
Labels:

tenet-performance, area-System.Net.Quic

Milestone: 8.0.0

@ManickaP
Copy link
Member

ManickaP commented Sep 2, 2022

While discussing this improvement and the issues discovered in #72696, one thought was to tie send buffer memory release to StreamClose, e.g. Dispose SendBuffer within MsQuicContextSafeHandle.ReleaseHandle for the stream. This would also address an issue of not releasing the memory in case QuicStream is not disposed of, but only GCed since there's no finalizer for it.

@ManickaP
Copy link
Member

ManickaP commented Jul 7, 2023

Closing based on the almost 50% perf regression on the basic send buffering implementation from this PR: #88320

IMHO, it'll be really hard to beat already optimized msquic buffering, especially with all WriteAsync calls needing to be always serialized.

We can always re-open if we decide to give this another spin.

@ManickaP ManickaP closed this as completed Jul 7, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants