-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Rework HttpClient content buffering #109642
Conversation
6f5edcb
to
a235c91
Compare
If we hit bursty scenarios where the pool is exhausted, we end up allocating buffers, and then there's not enough room in the pool so we end up dropping buffers, won't using such a large buffer be particularly problematic because it's above the LOH threshold and will always be LOH / gen2? |
|
||
private bool _lastBufferIsPooled; | ||
private byte[] _lastBuffer; | ||
private byte[]?[]? _pooledBuffers; |
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.
Would it be possible to leverage MutliArrayBuffer
, which already has a collection of pooled buffers -- or will that be terribly inefficient?
Or the other way around: could something from here improve the MutliArrayBuffer implementation?
It just feels a little bit like reimplementing a very similar idea, but maybe I'm missing something.
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.
It's a good question and to a degree we are doing very similar things.
Using MultiArrayBuffer
as-is would indeed be inefficient when it comes to larger responses because it uses only one fixed size for buffers (16 KB). A large content could quickly negate any pooling benefits by starving out the array pool for that bucket size.
That shouldn't be as much of an issue with our current uses of MultiArrayBuffer
because those are generally meant to be short-lived and relatively small (e.g. hand off point between the connection's read loop and the request's calls to ReadAsync
).
Side note: if we wanted to get really fancy, we could try to avoid that extra memory copy and instead rent new buffers for connection's reads, but that's something for the (far) future.
I think it's more likely we would go the other way as you mention and tweak the MultiArrayBuffer
implementation instead. Its use of a single buffer size seems quite ingrained in most of the implementation, but looking more at our current usages of it, it shouldn't be necessary (for example, we don't care about faster indexing into the buffer that that allows for). I'd be more inclined to replace MultiArrayBuffer
entirely with something closer to the buffering logic here. It might be easier to reason about the change as a follow-up PR though (at risk of having two somewhat duplicatey concepts in the repo for now).
The implementation I'm adding in HttpContent
does have some odd specifics around its buffer growth logic, and optional switching from pooled to non-pooled buffers. That part probably isn't really transferable to other uses, but I think we can make a shared implementation work without regressing those.
a235c91
to
79463d6
Compare
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.
Couple of nits, feel free to ignore, LGTM.
{ | ||
return encoding.GetString(firstBuffer.Slice(bomLength)); | ||
} | ||
else |
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.
nit: redundant else
src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs
Outdated
Show resolved
Hide resolved
79463d6
to
ed30c82
Compare
* Rework HttpClient response buffering * Fix string preamble detection order * Less var * Lower initial buffer size for chunked responses to 16 KB * Apply some style changes
* Rework HttpClient response buffering * Fix string preamble detection order * Less var * Lower initial buffer size for chunked responses to 16 KB * Apply some style changes
Contributes to #81628
Fixes #62845
Fixes #75631
When buffering the
HttpContent
, we currently use either aMemoryStream
orLimitArrayPoolWriteStream
.If the
Content-Length
is known upfront, we'll allocate the correct buffer size right away.If not, we'll keep resizing the buffer as we read the content.
If the buffered content is exposed outside of our control (outside of
GetByteArrayAsync
/GetStringAsync
), we'll curently avoid using the pooled variant, even when growing.This change does a couple things:
Content-Length
upfront, we'll still allocate the exact buffer upfront.GetByteArrayAsync
results below show a time improvement even though allocations are the same.GetStringAsync
), so we start with a larger buffer (currently 16 KB) to avoid another memory copy for smaller responses, and to lower the number of buffers we have to rent & track.I kept the behavior the same w.r.t. allocating a new
byte[]
every time the user callsReadAsByteArrayAsync
(as discussed in #81628) for now, but we can do so in a follow-up.Benchmarks when the response doesn't have a
Content-Length
header, in-memory only (no I/O).