Improve Http2Connection buffer management

[MihaZupan]

Contributes to #61223

This PR brings two related changes to Http2Connection:

• Rent buffers from the shared ArrayPool and return them when idle, for both incoming and outgoing
• Issue zero-byte reads to the underlying transport

This reduces the memory usage of each idle Http2Connection by up to 80 kB. This number comes from:

• The 16 kB read buffer (that's what we'll end up expanding it to as soon as we see any data frames over 8 kB)
• The 32 kB write buffer (that's what we'll expand it to, capped by UnflushedOutgoingBufferSize)
• The 32 kB SslStream buffer that we don't need to keep around while waiting for data due to the zero-byte read

We further avoid pinning buffers for long periods of time (on Windows), which leads to less memory fragmentation so the GC can do its job, which again leads to lower memory consumption.

I haven't yet run throughput benchmarks on this change, but I wouldn't expect the difference to be too significant (based on what we've seen in YARP where we default to using zero-byte reads on all response streams).

Do we want this to be configurable? My vote is on no.
ASP.NET Core does have a WaitForDataBeforeAllocatingBuffer flag, but it is ON by default, and it doesn't look like it's really getting any use.
There's also the workaround of providing a custom stream via ConnectCallback that no-ops on a zero-byte read.

Risks

The changes in #61913 enabled the consumer of HttpClient response stream to issue zero-byte reads.
Notably, they did not change the behavior unless the consumer explicitly asked for zero-byte reads.

This change does affect the default behavior. Some stream implementations are not aware of the possibility of zero-length read buffers and may break. HttpClient itself is less susceptible to such issues as they would only appear when the ConnectCallback is overridden to return such an incompatible stream. This shouldn't prevent us from making the change, but I wanted to call it out.

Edit: As pointed out by Stephen, we already use zero-byte reads on HTTP/1.1 as part of the connection pool scavenging logic - we're therefore not introducing completely new behavior for HttpClient consumers.

[ghost]

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

Issue Details

---

Contributes to #61223

This PR brings two related changes to Http2Connection:

• Rent buffers from the shared ArrayPool and return them when idle, for both incoming and outgoing
• Issue zero-byte reads to the underlying transport

This reduces the memory usage of each idle Http2Connection by up to 80 kB. This number comes from:

• The 16 kB read buffer (that's what we'll end up expanding it to as soon as we see any data frames over 8 kB)
• The 32 kB write buffer (that's what we'll expand it to, capped by UnflushedOutgoingBufferSize)
• The 32 kB SslStream buffer that we don't need to keep around while waiting for data due to the zero-byte read

We further avoid pinning buffers for long periods of time (on Windows), which leads to less memory fragmentation so the GC can do its job, which again leads to lower memory consumption.

I haven't yet run throughput benchmarks on this change, but I wouldn't expect the difference to be too significant (based on what we've seen in YARP where we default to using zero-byte reads on all response streams).

Do we want this to be configurable? My vote is on no.
ASP.NET Core does have a WaitForDataBeforeAllocatingBuffer flag, but it is ON by default, and it doesn't look like it's really getting any use.
There's also the workaround of providing a custom stream via ConnectCallback that no-ops on a zero-byte read.

Risks

The changes in #61913 enabled the consumer of HttpClient response stream to issue zero-byte reads.
Notably, they did not change the behavior unless the consumer explicitly asked for zero-byte reads.

This change does affect the default behavior. Some stream implementations are not aware of the possibility of zero-length read buffers and may break. HttpClient itself is less susceptible to such issues as they would only appear when the ConnectCallback is overridden to return such an incompatible stream. This shouldn't prevent us from making the change, but I wanted to call it out.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	area-System.Net.Http, tenet-performance
Milestone:	8.0.0

[stephentoub]

Some stream implementations are not aware of the possibility of zero-length read buffers and may break

The only two valid behaviors a Stream could have are to return immediately or to wait for data to be available and then return. Typical zero-byte read consumption doesn't break either, since in the worst correct case the first read just completes immediately and then the second read does the work of waiting. You don't get the benefits, but it shouldn't break. We already rely on this in our comnection pooling:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

Line 218 in e31ddfd

async ValueTask<int> ReadAheadWithZeroByteReadAsync()

I've not looked at your change yet and won't be able to until January, but we should be able to do it in a safe-enough manner.

[stephentoub]

I haven't yet run throughput benchmarks on this change, but I wouldn't expect the difference to be too significant

That would be great though I'm a bit surprised. Historically, at least with http/1.1, number of syscalls performed had a measurable impact on throughput.

[MihaZupan]

You don't get the benefits, but it shouldn't break.

Before zero-byte reads were a thing, you could get away with code like

int read = await inner.ReadAsync(buffer);
if (read == 0 && ExpectingMoreDataToRead)
{
    throw EOF();
}

or input validation like

if (count <= 0 || count > buffer.Length - offset) 
{ 
   throw new ArgumentOutOfRangeException(...); 
} 

These are arguably just bugs in those stream implementations, but they do exist.
We saw a few such wrapper streams in ASP.NET Core that did break like this after we made the changes in YARP: dotnet/aspnetcore#41305, dotnet/aspnetcore#41692, dotnet/aspnetcore#41287

We already rely on this in our comnection pooling

Oh right, slipped my mind when writing this. We can ignore the 'risks' then as we're already doing this.

[stephentoub]

These are arguably just bugs in those stream implementations

Exactly. I'm making a distinction between correctly and incorrectly implemented streams. Practically any change we make could "break" an incorrect one and I care much less about those.

[JamesNK]

I'm looking at making gRPC IPC better in .NET 8. Part of that is using named pipes as a transport, which means wiring the named pipes stream up with ConnectCallback.

It looks like NamedPipeClientStream no-ops with zero-bytes:

runtime/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs

Lines 94 to 98 in 6407eae

	if (buffer.Length == 0)
	{
	UpdateMessageCompletion(false);
	return new ValueTask<int>(0);
	}

No-op is fine. I think there is a test that uses named pipes over HTTP/2 so hopefully that provides verification that it still works.

[MihaZupan]

I think there is a test that uses named pipes over HTTP/2 so hopefully that provides verification that it still works.

We do have 1 test for it :)

[MihaZupan]

I haven't yet run throughput benchmarks on this change, but I wouldn't expect the difference to be too significant

That would be great though I'm a bit surprised. Historically, at least with http/1.1, number of syscalls performed had a measurable impact on throughput.

The only regression seems to be Linux + TLS with many HttpClient instances and 1 request per client (~5 %).
Everything else (Windows/non-TLS/multiple requests per client) looks within the margin of error or even shows this change as an improvement.

@davidfowl do you remember any Kestrel results re: this, given that WaitForDataBeforeAllocatingBuffer is ON by default in ASP.NET, but OFF for benchmarks?

[davidfowl]

@davidfowl do you remember any Kestrel results re: this, given that WaitForDataBeforeAllocatingBuffer is ON by default in ASP.NET, but OFF for benchmarks?

Here's the PR. We had that big performance push in .NET 3.0 when we added this flag after investigating some of the benchmarks. I believe this only really showed up for the PlaintextPlatform benchmark aspnet/Benchmarks#1469. IIRC @adamsitnik did the original investigation.

[adamsitnik]

IIRC @adamsitnik did the original investigation.

IIRC dotnet/aspnetcore#19396 allowed us to reduce the number of sys-calls (one read()), which had an impact on the JSON benchmarks (as it's just a throughput benchmark).

[ManickaP]

Apart from adding code comments, LGTM, thanks!

[MihaZupan]

Build failure is known according to Build Analysis

[stephentoub]

Note ArrayPool will use Array.Empty for 0 byte requests, so this could also be:

_bytes =
    usePool ? ArrayPool<byte>.Shared.Rent(initialSize) :
    initialSize == 0 ? Array.Empty<byte>() :
    new byte[initialSize];

It's unlikely to really matter, but if we expect initialSize to most commonly be non-zero, you might want to reorder it.

[stephentoub]

ArrayPool is fine with Array.Empty being returned; it just gets ignored.

[MihaZupan]

I'll remove the extra check, given that we won't be calling ClearAndReturnBuffer on already-empty buffers often.