SSLStream should not use the ArrayPool for it's internal buffers

[davidfowl]

Description

Today SSLStream uses a 4K buffer for the TLS handshake and a 16K buffer for the reading TLS frames. These buffers come from the array pool today which makes it really easy to exhaust the buckets in the shared array pool for those commonly used sizes. This affects websocket and any other scenario that results in a large number of long lived concurrent connections.

Regression?

No.

Data

A demo of 5000 websocket connections connected to an ASP.NET Core server using Kestrel:

Here's a heap dump sorted by inclusive size. Most of what is being held onto is array pool buffers.

On top of that, I took a trace of the same set of connections and you can see the ArrayPool event source says the number of buffers allocated is 5000. The buffers are all 32K in size (because of how the SslSteam adds the FrameOverhead).

Allocation profile of 5000 websocket connections. You can see the array pool has the most byte[] allocations.

They're mostly in the handshake and reading (I'm not writing in this app), just sitting mostly idle with ping pongs:

Analysis

We shouldn't use the array pool buffers for reads that may take a long time. That means both for the handshake and the encryption/decryption:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

Line 1040 in 5dacf1e

_internalBuffer = ArrayPool<byte>.Shared.Rent(ReadBufferSize);

The handshake will end up doing the same for 8K buffers in the pool but potentially for a much shorter duration as clients negotiate quickly and there's a timeout to complete the handshake (in kestrel).

Tagging subscribers to this area: @tannergooding, @pgovind, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

Today SSLStream uses a 4K buffer for the TLS handshake and a 16K buffer for the reading TLS frames. These buffers come from the array pool today which makes it really easy to exhaust the buckets in the shared array pool for those commonly used sizes. This affects websocket and any other scenario that results in a large number of long lived concurrent connections.

Regression?

No.

Data

A demo of 5000 websocket connections connected to an ASP.NET Core server using Kestrel:

Here's a heap dump sorted by inclusive size. Most of what is being held onto is array pool buffers.

On top of that, I took a trace of the same set of connections and you can see the ArrayPool event source says the number of buffers allocated is 5000 while the number of buffers rented was 16. The buffers are all 32K in size (because of how the SslSteam adds the FrameOverhead).

Analysis

We shouldn't use the array pool buffers for reads that may take a long time. That means both for the handshake and the encryption/decryption:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

Line 1040 in 5dacf1e

_internalBuffer = ArrayPool<byte>.Shared.Rent(ReadBufferSize);

The handshake will end up doing the same for 8K buffers in the pool but potentially for a much shorter duration as clients negotiate quickly and there's a timeout to complete the handshake (in kestrel).

Author:	davidfowl
Assignees:	-
Labels:	area-System.Buffers, tenet-performance, untriaged
Milestone:	-

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

Issue Details

---

Description

Today SSLStream uses a 4K buffer for the TLS handshake and a 16K buffer for the reading TLS frames. These buffers come from the array pool today which makes it really easy to exhaust the buckets in the shared array pool for those commonly used sizes. This affects websocket and any other scenario that results in a large number of long lived concurrent connections.

Regression?

No.

Data

A demo of 5000 websocket connections connected to an ASP.NET Core server using Kestrel:

Here's a heap dump sorted by inclusive size. Most of what is being held onto is array pool buffers.

On top of that, I took a trace of the same set of connections and you can see the ArrayPool event source says the number of buffers allocated is 5000 while the number of buffers rented was 16. The buffers are all 32K in size (because of how the SslSteam adds the FrameOverhead).

Analysis

We shouldn't use the array pool buffers for reads that may take a long time. That means both for the handshake and the encryption/decryption:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

Line 1040 in 5dacf1e

_internalBuffer = ArrayPool<byte>.Shared.Rent(ReadBufferSize);

The handshake will end up doing the same for 8K buffers in the pool but potentially for a much shorter duration as clients negotiate quickly and there's a timeout to complete the handshake (in kestrel).

Author:	davidfowl
Assignees:	-
Labels:	area-System.Buffers, area-System.Net.Security, tenet-performance, untriaged
Milestone:	-

[stephentoub]

We shouldn't use the array pool buffers for reads that may take a long time.

That may be true, but i don't know that's supported by the data here. You can have 5000 buffers from the same bucket in the pool all out at the same time; what matters is how many end up being returned at the same time. If the arrays only spend a small amount of time in the pool, you can have a much larger number than the bucket size with few being discarded. (I've called out before we don't have the right events in the pool right now to successfully answer these questions and I'd like us to fix that. )

So, let's say SslStream stopped using the pool. What gets better here?

[davidfowl]

So, let's say SslStream stopped using the pool. What gets better here?

Anyone renting 32K buffers wouldn't end up allocating them from the heap since the pool is drained. In websocket scenarios that end up with lots of pending reads, this could be a win. Though ultimately I think the better answer in this case is to read directly into the user's buffer, I figured I'd file this issue anyways.

The memory usage with TLS compared to non TLS websockets is stark and I'm wishing that I had spent more time looking at the implementation sooner....

[stephentoub]

Anyone renting 32K buffers wouldn't end up allocating them from the heap since the pool is drained.

But instead SslStream would be allocating those same buffers, no? That's what I'm trying to understand. At the end of the day, is it not just changing who's allocating the buffer?

[davidfowl]

But instead SslStream would be allocating those same buffers, no? That's what I'm trying to understand. At the end of the day, is it not just changing who's allocating the buffer?

I don't want to conflate the issues but yes, this particular isn't about reducing overall memory usage (maybe I should file that one too). It's other code in the app trying to use the pool and ending up allocating because the bucket(s) are drained.

[stephentoub]

It's other code in the app trying to use the pool and ending up allocating because the bucket(s) are drained.

I'm still trying to understand. Let's say the buckets are drained because SslStream consumed them. How is that different from the buckets being empty because no one has allocated / returned any arrays yet? And if these components allocate arrays and then return them, in general won't the arrays be there for them (or others other components including SslStream returned) when they then go back to get another? Sorry if I'm not understanding the concern.

Things go poorly with the array pool when arrays are returned and dropped because the pool is full. That's the main thing I was alluding to about wanting an event for, as it's actually the sign of a problem (there's also a concern if a buffer is rented and never returned, but that's much less common in well-behaved code, e.g. we sometimes won't return a buffer if an exception is thrown and have generally considered that to be ok). Is the SslStream use somehow causing such dropping to happen more often?

[davidfowl]

How is that different from the buckets being empty because no one has allocated / returned any arrays yet?

Maybe I'm misunderstanding something but ArrayPool.Shared is optimized for synchronous operations. When the array pool is used for arrays are being rented and returned quickly then there's very little allocation and lots of reuse (once things are "warm"). When used for operations that may be pending for a long time (like the websocket scenario) the array pool it is more likely that arrays are out of the pool than in. TL;DR Long running operations make it more likely that buffers are out of the pool for longer than need be, resulting in more overall allocations rending the pool less useful.

[jkotas]

less useful != useless

We know about many situations where the current ArrayPool behavior is sub-optimal (#853, #12800, #13289, ...). The GC team has allocated time in .NET 6 to make this better.