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

Enable zero-byte reads in streaming & WebSocket scenarios #1325

Closed
Tratcher opened this issue Oct 26, 2021 · 11 comments · Fixed by #1415
Closed

Enable zero-byte reads in streaming & WebSocket scenarios #1325

Tratcher opened this issue Oct 26, 2021 · 11 comments · Fixed by #1415
Assignees
Labels
tenet-performance Impacting performance Type: Bug Something isn't working

Comments

@Tratcher
Copy link
Member

The StreamCopier is used today for the request body, response body, as well as upgrade request and response streams. It rents a 64kb buffer per direction per request.

private const int DefaultBufferSize = 65536;

This is good for throughput, but not so good for scale. Streaming and WebSocket requests may spend long periods idle, holding onto 128kb of memory per connection.

In .NET 6 there was a lot of work to enable Zero-byte reads, allowing the caller to pass in empty buffers and wait to be notified of data before allocating. https://twitter.com/davidfowl/status/1451230807197040649?s=20

See where we can apply a similar pattern in YARP.

@Tratcher Tratcher added Type: Bug Something isn't working tenet-performance Impacting performance labels Oct 26, 2021
@karelz karelz changed the title Enable zero-byte reads in streaming & WebSocket sceanrios Enable zero-byte reads in streaming & WebSocket scenarios Oct 26, 2021
@karelz karelz added this to the Backlog milestone Oct 26, 2021
@MihaZupan MihaZupan modified the milestones: Backlog, YARP 1.1.0 Nov 9, 2021
@MihaZupan
Copy link
Member

MihaZupan commented Nov 11, 2021

To get the full benefit out of zero-byte reads, we need runtime changes to enable them on the HttpClient response streams.
The following numbers were obtained using a patched System.Net.Http.dll containing MihaZupan/runtime@f47c787 and YARP containing MihaZupan@b9d3af3.

The memory usage of YARP per 1000 WebSockets:

Current New Difference
HTTP-HTTP 156 MB 27.9 MB 5.6 x
HTTPS-HTTP 159 MB 30.6 MB 5.2 x
HTTP-HTTPS 189 MB 29.0 MB 6.5 x
HTTPS-HTTPS 192 MB 31.7 MB 6.1 x

The main footprint is taken up by various byte[]s.
There are 3x 4 KB buffers (HttpClient's HttpConnection, Kestrel Read, Kestrel Write) in each of these scenarios.
Currently, there is an extra 2x 64 KB buffers from YARP's StreamCopier + another 32 KB from SslStream when used by HttpClient.

Note that even the "current" already includes Kestrel zero-byte read improvements with SslStream.

Full memory usage for the zero-byte HTTPS-HTTPS above

image
image
image

VS memory usage reports for each of the above 8 scenarios: https://1drv.ms/u/s!AtN8Uab8waEXje4ucv87PFavDX3Fcg?e=Tkffqp

cc: @davidfowl

Note: This applies to all requests, nothing here is WebSocket specific.

@davidfowl
Copy link
Contributor

Any way we could do this in a patch of 6.0?

@geoffkizer @stephentoub ?

@Tratcher
Copy link
Member Author

How could YARP take a dependency on a patch in the runtime? We would need an approach that still works if the patch isn't there.

@davidfowl
Copy link
Contributor

YARP doesn't need to take a dependency on the patch (I don't see why that matters). It's about consumers being able to get a bigger benefit when those changes are available.

@stephentoub
Copy link
Member

Any way we could do this in a patch of 6.0? @geoffkizer @stephentoub ?

If the change ends up being small, low risk, and impactful.

We should start with changes in main and go from there.

@geoffkizer
Copy link

See also dotnet/runtime#61223 (comment) -- in particular bullet (2) on that comment.

We should probably split out a separate issue specifically to track zero-byte read support on response streams (including RawConnectionStream since that's used by websockets), since this should be straightforward to do and adds clear value for YARP scenarios and others.

@geoffkizer
Copy link

I added an issue specifically to track this: dotnet/runtime#61475

@Tratcher
Copy link
Member Author

YARP doesn't need to take a dependency on the patch (I don't see why that matters). It's about consumers being able to get a bigger benefit when those changes are available.

@davidfowl It looks like YARP needs to call ReadAsync differently if the fix is present or not in HttpClient's response stream. Right @MihaZupan?
MihaZupan/runtime@f47c787#diff-2b6b9edc973a391ab1db8b2d7acd95e30e3f03e824d69a0cf52a014275f8df46L105-R111

@MihaZupan
Copy link
Member

We can always try to perform 0 byte reads, but if the underlying stream doesn't support them it just means we will waste a few cycles to return the buffer only to rent it back right away.

If we know that they won't be supported (<= 5.0), we can avoid trying at all, but functionally it would be fine either way.

@geoffkizer
Copy link

We can always try to perform 0 byte reads, but if the underlying stream doesn't support them it just means we will waste a few cycles to return the buffer only to rent it back right away.

I think you could even address this by doing the following:

(1) Issue 0 byte read, but don't await it
(2) Check if it's completed. If it has, no sense in releasing the buffer
(3) If it hasn't, release the buffer, await the 0 byte read, acquire buffer again
(4) Issue real read

In the case where the underlying stream doesn't support 0 byte read, the 0 byte read will always complete immediately and you'll never release the buffer.

In the case where the underlying stream does support 0 byte read, you will also benefit because if there's more data available right now, the 0 byte read will complete immediately and you won't need to release/reacquire the buffer.

@MihaZupan
Copy link
Member

Changes on runtime's side: dotnet/runtime#61913
With the minimal change for just HTTP/1.1 WebSockets: dotnet/runtime@25029a7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tenet-performance Impacting performance Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants