Fast path for validating static table HTTP/2 headers

[JamesNK]

Incoming request headers are currently identified during validation by comparing header bytes. This can be made faster for headers with a static table index. Static table headers can also skip validation that names are all lower case.

This change will speed up validation of request headers. Once it is merged I think the next step is to add a method on HttpRequestHeaders collection that sets headers using the index instead of name. Update: Now also in this PR.

Note: There will probably need to be a reaction in dotnet/runtime to the IHttpHeadersHandler.OnStaticIndexedHeader overloads being used. If runtime doesn't want to use the static index then it is pretty easy to implement the methods like so:

public void OnStaticIndexedHeader(int index)
{
    ref readonly var entry = ref H2StaticTable.Get(index - 1);
    OnHeader(entry.Name, entry.Value);
}

public void OnStaticIndexedHeader(int index, ReadOnlySpan<byte> value)
{
    OnHeader(H2StaticTable.Get(index - 1).Name, value);
}

public void OnHeader(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
{
    // ...
}

@geoffkizer I don't know if you are still looking at HttpClient perf but you might be able to get some perf gains in the client by comparing the index instead of bytes/strings.

[JamesNK]

What is the deal with GeneratedCodeIsUpToDate? I have updated KnownHeaders.cs and run the code generation app, but the test is complaining about a difference 🤷

[geoffkizer]

@geoffkizer I don't know if you are still looking at HttpClient perf but you might be able to get some perf gains in the client by comparing the index instead of bytes/strings.

Yeah, we have an issue on this: dotnet/runtime#1505

Definitely would be nice to do, but we punted it out of 5.0. Having this implemented in the decoder itself is nice and will make this easier to do in the future.

If you have scenarios where you think this would help, let me know and we can try to squeeze it into 5.0 -- but time is very short :)

[geoffkizer]

Per discussion with @karelz etc about this, it's too late to take this change in the runtime. So if/when you merge this to aspnetcore, we will stop pulling updates until 6.0 (which we were planning to do soon anyway).

[JamesNK]

Doesn't sync go in the other direction? i.e. runtime is the authoritative source of truth for the shared HTTP/2 source, and aspnetcore keeps in sync with it.

How can I change this file in aspnetcore without updating runtime? An option to avoid any behavior changes in runtime could be to have #ifdef to preserve what it currently does. Or turn off the sync check that aspnetcore does until runtime branches master for 6.0.

[geoffkizer]

I'm not clear on how the code sync works; I'll defer to @karelz and @scalablecory here...

[Tratcher]

The shared code syncs from runtime to aspnetcore. We should not allow them to diverge. If this can't go into runtime for 5.0 then we should hold it for both repos until 6.0.

[JamesNK]

😢

@geoffkizer Are you ok with a PR for this in runtime if the changes are #ifdefed out by default? Runtime will be unchanged, and ASP.NET Core can set a compiler constant to get static indexes.

[scalablecory]

Current plan is for runtime "master" to be .NET 6 on 8/17.

From our standpoint it can be merged at that point (assuming it comes with accompanying SocketsHttpHandler change to light it up) and still keep the repos synced.

[JamesNK]

Ok. It can wait until then.

[geoffkizer]

I'm fine with that but it seems a little weird... you're going to be syncing to your 5.0 branch from our 6.0 branch?

[Tratcher]

I'm fine with that but it seems a little weird... you're going to be syncing to your 5.0 branch from our 6.0 branch?

Yeah, that's not good. If anything we'd change the sync bot to sync the new runtime 5.0 branch to aspnetcore's master branch. AspNetCore won't branch for 6.0 for months.

[geoffkizer]

If you really want this for 5.0, then I'd prefer to just get it in rather than do #ifdefs or something like that. It's a reasonably safe change. Needs to get into runtime this week, though.

@karelz @scalablecory Any objections here?

[JamesNK]

Hmmm, is sync only on master? If it is then that will be happening automatically until both repos branch for 6.0.

If syncing changes when branching happens then this would still be problematic.

[karelz]

@JamesNK if you can do the change in runtime (we're a bit busy to help this week before branching sadly :(), then I'd be ok to take it based on @geoffkizer's assessment of risk above in #24730 (comment)

[Tratcher]

The H2StaticTable.Get off-by-one is leaking all over the code. Please fix it in this PR or a follow-up one.

[JamesNK]

The H2StaticTable.Get off-by-one is leaking all over the code. Please fix it in this PR or a follow-up one.

It is tricky because the code is shared with runtime. Next time I update it in both places I'll make that change.