-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add HPack dynamic compression #20058
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
Conversation
9cdacb1
to
fac451e
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.
So you went with encode-everything. I'm curious to see the perf impact.
src/Servers/Kestrel/Core/src/Internal/Http2/HPack/StatusCodes.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Http2/HPack/StatusCodes.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Http2/HPack/IHeaderSensitivityDetector.cs
Outdated
Show resolved
Hide resolved
@Tratcher Question for you about SETTINGS_HEADER_TABLE_SIZE. If the client sends a settings frame that updates the header table size, should the server issue a dynamic table size update? https://tools.ietf.org/html/rfc7541#section-6.3 I'm not sure whether the dynamic table size update is always sent, or if it exists for the situation when the server decides to change the size without the client sending SETTINGS_HEADER_TABLE_SIZE. |
RPS Before:
RPS After:
Dynamic compression is slightly slower when headers continuously change, and slightly faster when headers are static. Size: This shows the change in headers response size when all headers are reused on the second request. If they are different then there will be no change from the first request. 1 header first request: 104 bytes 4 headers first request: 284 bytes 32 headers first request: 1986 bytes |
These benchmarks are underwhelming so far, minimal change to rps and no change to allocations. We're going to need more numbers to show this is worth doing. These are the metrics I'd expect to improve with this change:
|
src/Servers/Kestrel/Core/src/Internal/Http2/HPack/Http2HPackEncoder.cs
Outdated
Show resolved
Hide resolved
This will be the biggest improvement. You can see the significant decrease in HEADERS frame size some of the unit tests where multiple calls are made. I'll get a byte break down from the benchmark. I think the benchmarks so far mostly shows that the impact HPack encoding has on the server CPU usage is minimum in either direction. Best case it improves (and IMO this will probably be the typical case), and worst case where every header is different it is only slightly slower. |
Benchmarks response size added - #20058 (comment) |
Consider the difference between the table size limit set by the client and the actual table size being maintained by the server. The server's actual table size can always be smaller than the client's limit. So yes, the server has to actually acknowledge that it is changing the table size (if it wants to do so after a limit change, or for any other reason).
This can happen in both directions. The client could choose to raise the table size limit beyond what the server wants to utilize, in which case the server will not change the table size to match the new limit. Similarly the server could choose to lower the table size for some other reason (e.g. low traffic) without the client having changed the limit. https://tools.ietf.org/html/rfc7541#section-4.2
This also answers the ACK ordering question. ACK first. |
@scalablecory Is this something HttpClient might use? |
We prototyped this and weren't happy with the performance and implementation complexity tradeoffs. One scenario we looked at was the gRPC client. Perhaps response headers are a different story, though. Or, maybe this implementation will be faster than what we came up with and it's worth revisiting. @JamesNK if you want to take a shot at it and can demonstrate good benefit, I'd be happy to look at a PR. |
Do you still have the client prototype code? I know you looked at huffman but I wasn't aware you investigated static/dynamic compression. I'm surprised you found static compression not worth it. It was a pretty quick win to implement in Kestrel. |
I think it's at https://github.com/aik-jahoda/corefx/tree/jahoda/dynamicheaders
We haven't looked at pure static compression; I think that could be an easy win. We looked at it combined with dynamic compression. CC @aik-jahoda We have a concept of "known headers" already; it would not be hard to augment it with known values (we kind of have these already but only use it for string interning, not protocol) |
Ah, you tried to reuse the decoder dynamic table. HPack implementations I looked at used a separate data structure for the encoder dynamic table and the decoder dynamic table. They store the same thing, but the way you search them and the output you want from them is different. Encoder dynamic tables typically combine a hash table (based on header name) for fast lookups with a doubly linked list to track age and make FIFO evictions. The encoder dynamic table I've written does that, and is also based around strings rather than bytes because string values are what we build up in the headers collection, and what we want to compare against when matching. If you're interested in reusing this code then I think I make be able to remove dependencies on ASP.NET Core types. At the moment this PR uses ASP.NET's |
448450d
to
72dc315
Compare
Do we want to validate SETTINGS_MAX_HEADER_LIST_SIZE on the server? https://tools.ietf.org/html/rfc7540#section-6.5.2 Currently Kestrel doesn't do anything with SETTINGS_MAX_HEADER_LIST_SIZE sent by the client. Kestrel could validate headers are under the setting size and throw an exception if headers exceeds it. I'd like to leave it for the future. |
https://tools.ietf.org/html/rfc7540#section-10.5.1 Enforcement on our end is optional. Might start with a debug log. Sending the headers is the clearest way to indicate to the client the actual error (headers too large), there's no other standard error available that would convey that. |
Being optional is why I guessed we didn't support it. I'm not eager to add it right now so I'm going to leave it alone. |
Chrome's headers and HttpClient's dynamic table content after visiting different websites below. Note that some headers aren't in the dynamic table because they've matched the static table, e.g. status, content-encoding https://blog.cloudflare.com/tools-for-debugging-testing-and-using-http-2/ - Includes all headers https://dotnet.microsoft.com/ - Includes all headers (Originally from Kestrel. Reverse proxy (HttpSys?) is adding HPack? Content after visiting https://www.google.com/ - Doesn't include Set-Cookie |
ba2fc87
to
b05955b
Compare
67b84a9
to
4fbf018
Compare
07bd63c
to
2517f0e
Compare
Fixes #4715
TODO
KnownHeaders
enum) out of the encoder and encapsulated it inHttp2HeadersEnumerator
. I'm leaving the code in ASP.NET Core. If the client team decides they want to add the feature in the future we can collaborate on additional changes required to it then.