Open ProTip opened 1 year ago
@mgravell Does this seem like a perf win?
Interesting one; there's a few things in play here - although first I must add a caveat that the Kestrel pipe-writer is very different to the http.sys pipe-writer, so large parts of this topic may depend strongly on whether you're using http.sys vs iis vs regular kestrel
I'm going to talk mostly in terms of regular kestrel here:
Stream
does a lousy job of offering a reliable "bytes remaining" API, so I can totally understand why it doesn't try to do anything too cleverFlushAsync
Stream.ReadAsync
(which we shouldn't change, to avoid causing deadlocks in complex scenarios where later completion depends on actually forwarding the existing data - the classic example being a NetworkStream
that won't get any more data until we've written something from the existing)FlushAsync
starts being non-synchronous)PipeWriter.CopyFromAsync(Stream...)
(which is the same thing as Stream.CopyToAsync(PipeWriter...)
) could allow improved throughputIMO what we should do here is: prove it; i.e. come up with some scenario where it actually makes a demonstrable difference that we can show in metrics; this is slightly complicated because we're probably mostly interested in external streams (like the postgres example here; possibly files, although files have better APIs) - things with latency
Perhaps ironically, the main thing I'm thinking looking at this code is "we should special-case MemoryStream
with some TryGetBuffer
love"
@mgravell Should we close this issue? It sounds like if we even wanted to allow something like this we'd want the size hint to be per-use and flow through all the Pipe APIs.
And we also need a use case that demonstrates the perf benefit.
Is there an existing issue for this?
Is your feature request related to a problem? Please describe the problem.
I'm attempting to optimize streaming response bodies from Postgres through asp.net via Npgsql's large object manager. While going down a rabbit hole creating incremental hash pipe wrappers, I noticed that only
4096b
of memory is ever requested from the response body wrapper'sGetMemory
method.After much digging around it appears that the commonly used
CopyToAsync(PipeWriter..)
convenience methods do not pass asizeHint
through, nor do any of the other buffer or size options factor into how much memory is requested. Both the extension on stream and the method onStreamPipeReader
seems to end up calling bareGetMemory
here: https://github.com/dotnet/runtime/blob/v7.0.3/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeWriter.cs#L127 . This goes back toPipe
and seems to fall back on theMinimumSegmentSize
which default to 4096.Describe the solution you'd like
If we could supply options to the Pipe constructor in the
HttpContextBuilder
we could establish a global minimum segment size.A better solution IMHO(though in runtime) would be to properly wire
sizeHint
through all thePipe
APIs so this can be handled on a per-use basis. I can't tell if it wasn't fully introduced, or if it was but was ripped out 🤔Additional context
I'm seeing a pretty significant decrease in latency locally serving a 110k png to the browser. Latency drops from ~
10-14ms
to a ~7-11ms
when I overridesizeHint
in my wrappersGetMemory
method to8192
. Doubling it again seems to drop down to a5ms
best case with further reduction in variance.Not super scientific and I was just introduced to
System.IO.Pipelines
yesterday so I'm not sure if I'm missing something obvious here..Edit: I had previously linked to where the test host builder instantiates the pipe 😄 Have not yet been able to track down where Kestrel instantiates the
IDuplexPipe
(?) that underpins the response body writer..