-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Use zero byte reads for incoming TLS frames #30863
Conversation
- We recently did some work to reduce the amount of memory allocated by SslStream on idle reads so take advantage of it here by using a new StreamPipeReader feature that makes zero byte reads on the underlying Stream.
Glad to see this having the desired effect. Should this in some way respect the WaitForDataBeforeAllocatingBuffer setting at the transport layer? Or if not because it's the wrong layer, if such a setting was important there, is one important here? |
Definitely possible to respect this setting but it would need more public API to flow the setting via a feature as part of the transport layer or have an https specific setting (it'd be a layering violation to flow this existing setting directly into kestrel). I didn't want to do either as part of this PR because it requires a new API and I didn't measure any performance difference is basic scenarios. |
I filed this to consider adding a new API #30877. |
Great to see this validate the SslStream work. |
Hi @geoffkizer. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
You can see some of that work here:
The scenario is 5000 connections sending websocket pings from both server and client:
These are GC Heap sizes:
5000 NO TLS - 68,967,016 B
Before:
5000 TLS - 310,459,272 B
After:
5000 TLS - 119,175,792
SslStream is still holding onto a 4K buffer but this is a big improvement. I want to get it much closer to the non-TLS connections.
cc @geoffkizer @stephentoub @wfurt