-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Reduce memory footprint of HTTP/2 connections #112719
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @dotnet/ncl |
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.
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/DynamicTable.cs
Show resolved
Hide resolved
public const int DefaultStringOctetsSize = 4096; | ||
|
||
// This is the initial size. Buffers will be dynamically resized as needed. | ||
public const int DefaultStringOctetsSize = 32; |
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.
The reduced default string octets size of 32 may lead to more frequent buffer resizes if header values typically exceed this length; please ensure that the change is backed by performance benchmarks for realistic workloads.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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.
You could see more intermediate smaller allocations, but that is offset by the fact we're allocating less overall on average.
E.g. we may allocate more for value buffers in the worst case, but we'll allocate less for names. In either case these are temporary per-connection costs.
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.
Do we have data on expected string sizes?
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Show resolved
Hide resolved
@@ -194,7 +194,7 @@ public async ValueTask SetupAsync(CancellationToken cancellationToken) | |||
try | |||
{ | |||
_outgoingBuffer.EnsureAvailableSpace(Http2ConnectionPreface.Length + | |||
FrameHeader.Size + FrameHeader.SettingLength + | |||
FrameHeader.Size + 2 * FrameHeader.SettingLength + |
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.
This calculation was technically wrong before, but 52 vs 58 both end up with at least 64.
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: consider adding parens around the multiplication. I realize it's not necessary, but it took me a moment to visually parse what was being done.
This would go a long way towards addressing dotnet/aspnetcore#60313. Probably reducing about 25% of memory footprint per connection. |
{ | ||
var newBuffer = new HeaderField[maxSize / HeaderField.RfcOverhead]; | ||
Debug.Assert(_count + 1 <= _maxSize / HeaderField.RfcOverhead); |
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.
Consider putting _maxSize / HeaderField.RfcOverhead
into a local (maxCount or whatever) so that it's clear we're validating _count against the same expression that's used in resizing.
|
||
var entry = new HeaderField(staticTableIndex, name, value); | ||
_buffer[_insertIndex] = entry; | ||
_insertIndex = (_insertIndex + 1) % _buffer.Length; |
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 this be faster and equivalent as:
if (++_insertIndex == _buffer.Length)
{
_insertIndex = 0;
}
or does it not matter?
var newBuffer = new HeaderField[maxSize / HeaderField.RfcOverhead]; | ||
Debug.Assert(_count + 1 <= _maxSize / HeaderField.RfcOverhead); | ||
|
||
int newBufferSize = Math.Min(Math.Max(4, _buffer.Length * 2), _maxSize / HeaderField.RfcOverhead); |
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.
If someone is using the dynamic table feature, is it likely they'll only have 4 items in the table? Maybe I misunderstand typical use, but that seems small.
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'll heavily depend on the use case, but you're right, 4 is likely to be exceeded, it should likely start larger.
E.g. if you sent a single request, this might be the number of response headers for that request.
If you're hitting an API where response headers look the same, it might never grow after that.
If you were to use HttpClient as a crawler, then you're practically guaranteed to either use 0 if the server doesn't use it, or maxSize if it does.
Since the logic is shared, this applies to both SocketsHttpHandler and Kestrel.
Possibly addresses dotnet/aspnetcore#60313
cc: @AlgorithmsAreCool, @halter73, @JamesNK for the server side
HPackDecoder
allocates 4 buffers:_stringOctets
, 4 KB_headerNameOctets
, 4 KB_headerValueOctets
DynamicTable
IMO these are oversized for initial sizes:
I changed these to start very small, letting them grow as needed.
We already have resizing logic, I only modified
DynamicTable
to also support starting with a non-max-size buffer (it already could resize afterwards).Looking at HttpClient talking to Kestrel, I expect this should save around