Skip to content

HTTP/2: Improve incoming header performance #38834

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

Merged
merged 6 commits into from
Dec 10, 2021
Merged

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Dec 5, 2021

This PR makes a bunch of changes designed to improve the performance of parsing incoming HTTP/2 headers:

  1. The most invasive change is moving validation of headers to when they are added to the dynamic table. They are validated once when added to the table and requests can then skip the check. If a header is invalid then an error is thrown before the header is added to the dynamic table. It seems like this is ok to me because it is a connection error either way, but it would be good to get confirmation. This isn't necessary. Header is already validated using existing technology. Optimization is just detecting whether header comes from dynamic table and then skipping all validation.
    Validation:
    • Checking the header name doesn't have any uppercase values
    • Checking the header field isn't connection-specific
    • Checking the value doesn't contain newlines
  2. Set dynamic headers using HPACK static table index where possible. Faster than setting using the name.

What is the deal with IHttpHeadersHandler being public? I worked around breaking the interface by using a default method implementation. Is this acceptable? Any plans to make the interface internal? It's basically public API. Workaround is to use DIM to avoid breaking change to interface.

Also, checking the value doesn't contain newlines required some hacky code if an encoding selector is configured. Removed.

@JamesNK JamesNK requested a review from a team December 5, 2021 23:07
@Tratcher
Copy link
Member

Tratcher commented Dec 6, 2021

What is the deal with IHttpHeadersHandler being public? I worked around breaking the interface by using a default method implementation. Is this acceptable? Any plans to make the interface internal?

I think it was mainly there for benchmarks. Using a DIM should be fine. Removing it is a different matter. @sebastienros

@sebastienros
Copy link
Member

IHttpHeadersHandler is used by the platform benchmarks https://github.com/aspnet/Benchmarks/blob/e3095f4021fef7171bb3ae86616b9156df39b7bd/src/BenchmarksApps/Kestrel/PlatformBenchmarks/BenchmarkApplication.HttpConnection.cs#L307
I have no idea if it could be removed without breaking the benchmarks, @davidfowl ?

@halter73
Copy link
Member

halter73 commented Dec 7, 2021

IHttpHeadersHandler is used by the platform benchmarks aspnet/Benchmarks@e3095f4/src/BenchmarksApps/Kestrel/PlatformBenchmarks/BenchmarkApplication.HttpConnection.cs#L307 I have no idea if it could be removed without breaking the benchmarks, @davidfowl ?

I think the DIM makes this okay. Even though this would break older netstandard TFMs, Kestrel no longer ships on NuGet.

@JamesNK
Copy link
Member Author

JamesNK commented Dec 7, 2021

Platform benchmarks are also in TechEmpower. I'm guessing that making this type internal and having to copy and paste code to aspnet/benchmarks and techempower isn't ideal.

@halter73
Copy link
Member

halter73 commented Dec 7, 2021

Despite the "pubternal" namespace, we've decided to treat HttpParser and associated types like IHttpHeadersHandler as fully public. We shouldn't make any breaking changes if we can avoid it. If we ever cannot avoid it, we'd make a breaking change announcement.

@JamesNK
Copy link
Member Author

JamesNK commented Dec 7, 2021

PR updated. Logic is simplified by removing validation on adding headers to the dynamic table. It is unnecessary because that validation already happens when header name and value are first received.

Before:

|      Method | HeadersCount | HeadersChange |     Mean |    Error |   StdDev |     Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------ |------------- |-------------- |---------:|---------:|---------:|---------:|------:|------:|------:|----------:|
| MakeRequest |           32 |         False | 34.64 us | 0.687 us | 1.891 us | 28,869.4 |     - |     - |     - |     480 B |

After:

|      Method | HeadersCount | HeadersChange |     Mean |    Error |   StdDev |     Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------ |------------- |-------------- |---------:|---------:|---------:|---------:|------:|------:|------:|----------:|
| MakeRequest |           32 |         False | 33.03 us | 0.650 us | 0.822 us | 30,273.3 |     - |     - |     - |     479 B |

@JamesNK JamesNK force-pushed the jamesnk/hpack-validation branch from 981b612 to 939b765 Compare December 8, 2021 22:43
@JamesNK
Copy link
Member Author

JamesNK commented Dec 9, 2021

@halter73 @Tratcher Could you take another look? I'd like to merge this and apply similar changes to HTTP/3.

@JamesNK JamesNK enabled auto-merge (squash) December 9, 2021 23:58
@JamesNK JamesNK merged commit 7672ca9 into main Dec 10, 2021
@JamesNK JamesNK deleted the jamesnk/hpack-validation branch December 10, 2021 00:05
@ghost ghost added this to the 7.0-preview1 milestone Dec 10, 2021
@davidfowl davidfowl added the Perf label Aug 26, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Perf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants