Use default interfaces for a more x3 gain in getting and setting headers #31519

benaadams · 2021-04-04T02:20:27Z

Faster header get set by using default interfaces.

Also a cleaner API surface

Results

Test # 4 HeaderCollectionBenchmark

Method	Branch	Type	Mean	Op/s	Delta
GetHeaders	main	Plaintext	25.793 ns	38,770,569.6	-
GetHeaders	PR	Plaintext	12.775 ns	78,279,480.0	+101.9%
GetHeaders	main	Common	121.355 ns	8,240,299.3	-
GetHeaders	PR	Common	37.598 ns	26,597,474.6	+222.8%
GetHeaders	main	Unknown	366.456 ns	2,728,840.7	-
GetHeaders	PR	Unknown	223.472 ns	4,474,824.0	+64.0%

SetHeaders	main	Plaintext	49.324 ns	20,273,931.8	-
SetHeaders	PR	Plaintext	34.996 ns	28,574,778.8	+40.9%
SetHeaders	main	Common	635.060 ns	1,574,654.3	-
SetHeaders	PR	Common	108.041 ns	9,255,723.7	+487.7%
SetHeaders	main	Unknown	1,439.945 ns	694,470.8	-
SetHeaders	PR	Unknown	517.067 ns	1,933,985.7	+178.4%

Effected headers in tests:

Plaintext: Content-Type (indirectly via DefaultHttpRequest.ContentType)
Common: Content-Type, Connection, Cache-Control, Vary, Content-Encoding, Expires, Last-Modified, Set-Cookie, ETag, Transfer-Encoding, Content-Language, Upgrade, Via, Access-Control-Allow-Origin, Access-Control-Allow-Credentials, Access-Control-Expose-Headers
Unknown: Content-Type, Link, X-UA-Compatible, X-Powered-By, X-Content-Type-Options, X-XSS-Protection, X-Frame-Options, Strict-Transport-Security, Content-Security-Policy

Example user code change to take advantage of fast-path (outside of the default features/middleware/mvc)

Snippet from the default interface partial as example

Mechanism

Add default interface get, set to IHeaderDictionary for each header defined in HeaderNames; that calls through to the regular string lookup on same interface
For implemented headers these get,sets can be implemented by going direct to the field; entirely bypassing the lookup. (Kestrel fdf63ca , IIS/HttpSys d36ab8d)
For non-implemented headers (e.g. Kestrel) again these can bypass the initial lookup against the implemented headers and go direct to the non-implemented Dictionary<string, StringValues> lookup.
Use the faster header access in default features/middleware/mvc so benefits are felt without code changes 8542283
Roll out to other places (tests/samples e59d8d3) so the newer way is more obvious to people looking

This is an RFC as it still would need:

Tests to verify using string lookup and direct is the same
Generator for for the default interface portion of IHeaderDictionary from HeaderNames; however including a source generator (e.g. code for benaadams/Ben.Generator.KeyedIDictionary) makes the api checker unhappy as its a used assembly not shipped; it doesn't seem to distinguish between build-time only assemblies and runtime required ones.
PublicAPI.Unshipped.txt spilt by framework; as IHeaderDictionary multi-targets netfx, netstd and net6.0; but only net6.0 supports default interfaces.

ghost · 2021-04-04T02:24:14Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

benaadams · 2021-04-04T06:12:24Z

Also updated IIS/HttpSys to use fast-path for Request headers as wouldn't want @NickCraver to miss out 😉

src/Http/Http.Features/src/HeaderNames.cs

NickCraver · 2021-04-04T11:35:41Z

I mean the perf is great, but this is just far easier to read as a baseline. Absolute 👍

benaadams · 2021-04-04T14:57:23Z

CI suggests HTTP/2 headers need a bit of patching up with this change; but will wait till outcome of api review

davidfowl · 2021-04-04T15:58:26Z

@benaadams Any improvement for gets?

src/Http/Http/src/Features/RequestCookiesFeature.cs

davidfowl · 2021-04-04T16:06:25Z

So the main thing we need to resolve with the API is how to represent:

A header name that doesn't exist
A header name that exists but is empty
Removing a header value

This leads to StringValues? on the property which makes things slightly more annoying to use.

benaadams · 2021-04-04T16:47:32Z

So the main thing we need to resolve with the API is how to represent:
A header name that doesn't exist

StringValues.Length == 0 its what get's returned today

A header name that exists but is empty

StringValues.Length == 1 but entry is String.Empty

Removing a header value

= default(StringValues)

They are the semantics that already exist? e.g. .Headers[HeaderNames.XFrameOptions] will never return null even if the header has never been set; just default(StringValues) (e.g. .Length == 0)

It is specified in IHeaderDictionary that null is not a thing

aspnetcore/src/Http/Http.Features/src/IHeaderDictionary.cs

Lines 14 to 19 in 0aae147

    
           /// <summary> 
        
           /// IHeaderDictionary has a different indexer contract than IDictionary, where it will return StringValues.Empty for missing entries. 
        
           /// </summary> 
        
           /// <param name="key"></param> 
        
           /// <returns>The stored value, or StringValues.Empty if the key is not present.</returns> 
        
           new StringValues this[string key] { get; set; }

benaadams · 2021-04-04T22:43:58Z

Any improvement for gets?

I assume similar; though there doesn't seem to be a benchmark for it. Will make one...

benaadams · 2021-04-05T12:37:39Z

Aside I was curious how PGO and GuardedDevirtualization would effect this; so rearranged the benchmark so the setting of the headers was in local function outside loop then upped the iteration count and invocation count and tried some Jit switches

Method	Variant	Type	Mean	Op/s	Delta
SetHeaders	main	Plaintext	49.34 ns	20,266,332.7	-
SetHeaders	PR	Plaintext	33.90 ns	29,494,554.7	+45.5%
SetHeaders	PR+PGO	Plaintext	32.90 ns	30,394,840.5	+50.0%
SetHeaders	PR+PGO+Devirt	Plaintext	26.08 ns	38,343,238.7	+89.2%
SetHeaders	main	Common	651.33 ns	1,535,311.8	-
SetHeaders	PR	Common	107.55 ns	9,297,966.7	+505.6%
SetHeaders	PR+PGO	Common	100.37 ns	9,963,457.9	+549.0%
SetHeaders	PR+PGO+Devirt	Common	88.94 ns	11,243,864.9	+632.4%
SetHeaders	main	Unknown	1,416.02 ns	706,206.2	-
SetHeaders	PR	Unknown	523.40 ns	1,910,585.2	+170.5%
SetHeaders	PR+PGO	Unknown	503.89 ns	1,984,563.7	+181.0%
SetHeaders	PR+PGO+Devirt	Unknown	474.35 ns	2,108,151.3	+198.5%

/cc @AndyAyersMS

main & PR

set COMPlus_ReadyToRun=1
set COMPlus_TC_QuickJitForLoops=0
set COMPlus_TieredPGO=0
set COMPlus_JitEnableGuardedDevirtualization=0

PGO

set COMPlus_ReadyToRun=0
set COMPlus_TC_QuickJitForLoops=1
set COMPlus_TieredPGO=1
set COMPlus_JitEnableGuardedDevirtualization=0

Devirt

set COMPlus_ReadyToRun=0
set COMPlus_TC_QuickJitForLoops=1
set COMPlus_TieredPGO=1
set COMPlus_JitEnableGuardedDevirtualization=1

benaadams · 2021-04-06T00:41:03Z

Tweaked benchmark and added Get variant

Method	Branch	Type	Mean	Op/s	Delta
GetHeaders	main	Plaintext	25.793 ns	38,770,569.6	-
GetHeaders	PR	Plaintext	12.775 ns	78,279,480.0	+101.9%
GetHeaders	main	Common	121.355 ns	8,240,299.3	-
GetHeaders	PR	Common	37.598 ns	26,597,474.6	+222.8%
GetHeaders	main	Unknown	366.456 ns	2,728,840.7	-
GetHeaders	PR	Unknown	223.472 ns	4,474,824.0	+64.0%

SetHeaders	main	Plaintext	49.324 ns	20,273,931.8	-
SetHeaders	PR	Plaintext	34.996 ns	28,574,778.8	+40.9%
SetHeaders	main	Common	635.060 ns	1,574,654.3	-
SetHeaders	PR	Common	108.041 ns	9,255,723.7	+487.7%
SetHeaders	main	Unknown	1,439.945 ns	694,470.8	-
SetHeaders	PR	Unknown	517.067 ns	1,933,985.7	+178.4%

AndyAyersMS · 2021-04-06T01:07:26Z

I expect most users of PGO will simply end up using:

set COMPlus_TC_QuickJitForLoops=1
set COMPlus_TieredPGO=1

(as GDV is on by default now when PGO on, and disabling R2R is hopefully going to be less important as we'll have static PGO for the key methods that get prejitted)...

src/Http/Http.Features/src/HeaderNames.cs

Tratcher · 2021-04-06T21:48:52Z

src/Http/Headers/src/Microsoft.Net.Http.Headers.csproj

Did you do it this way because of the Http.Features netstandard2.0 target?

Ideally Http.Features would depend on Http.Headers, and that way you don't have to move HeaderNames.

If HeaderNames is moving, you'll need a TypeForward attribute.

Did you do it this way because of the Http.Features netstandard2.0 target?

Http.Headers is net6.0 only

Http.Features is net6.0, netstd2.0 and net461

Ideally Http.Features would depend on Http.Headers,

3 options there?

Drop support for netstd2.0 and net461 in Http.Features

Add support for netstd2.0 and net461 in Http.Headers

Conditionally reference Http.Headers but only for the net6.0 version of Http.Features

Option I'd go for is removing netstd2.0 and net461...

The SignalR client consumes the Features package so we can't drop TFMs from it.

Add support for netstd2.0 and net461 in Http.Headers

How bad is it? I assume it's using APIs not available on net461, but I can't guess how many. This isn't a library we've aggressively optimized.

Conditionally reference Http.Headers but only for the net6.0 version of Http.Features

So the IHeadersDictionary partial would be conditional to net6.0? Does that break anything upstream?

So the IHeadersDictionary partial would be conditional to net6.0?

Well neither netstd2.0 or net461 support default interface methods so it would have to be.

#31723 will address this by splitting up the Http.Features assembly.

After #32043 please switch this around so Http.Features depends on Http.Headers and revert the type move.

src/Http/Http.Features/src/IHeaderDictionary.Keyed.cs

benaadams · 2021-04-21T14:38:58Z

Added a tweak to the .get asm sharplab.io

Pre

    L0000: xor eax, eax
    L0002: mov rdx, [rcx+8]
    L0006: test edx, 0x100000
    L000c: je short L0012
    L000e: mov rax, [rcx+0x30]
    L0012: ret

Post

    L0000: mov rax, [rcx+0x30]
    L0004: test dword ptr [rcx+8], 0x100000
    L000b: je short L000e
    L000d: ret
    L000e: xor eax, eax
    L0010: ret

davidfowl · 2021-04-22T04:23:16Z

@benaadams you'll want to rebase on top of #32043

benaadams · 2021-04-26T20:03:13Z

Rebased and CI passed

davidfowl · 2021-04-26T20:13:03Z

src/Servers/Kestrel/Core/src/Internal/Http2/Http2HeaderEnumerator.cs

        {
-            _genericEnumerator = headers.GetEnumerator();
-            _headersType = HeadersType.Untyped;
+            switch (headers)


This is interesting. Why did this change?

Http2HPackEncoderTests.cs changed to this overload as cast the collection to IHeaderDictionary; which meant it started coming through as HeadersType.Untyped

Tratcher · 2021-04-26T19:29:15Z

src/Http/Http.Features/src/IHeaderDictionary.Keyed.cs

+
+namespace Microsoft.AspNetCore.Http
+{
+    public partial interface IHeaderDictionary


Is this the flow for adding a new header?

Add it to HeaderNames

Add it to IHeaderDictionary

Re-run the kestrel code generator, it will pick up the new header from HeaderNames.

Does anything else need to be updated?

Yes; I had a source generator for the IHeaderDictionary (how I originally generated them); but the CI rejected it as a used project that wasn't in the shipped API, which seemed to be going wrong since it was just internal and not to be shipped.

Could automate it as part of the Kestrel code generator (so only need to add to HeaderNames); however that would need an attribute on some of the headers (e.g. DNT) to say don't add them (Obselete may work or may be too strong; since it also has Path,Scheme, Method etc.)

Tratcher · 2021-04-26T22:53:18Z

Thanks for waiting for the rebase, and updating all of the usage.

wtgodbe · 2021-04-28T16:15:18Z

src/Middleware/ResponseCaching/test/ResponseCachingPolicyProviderTests.cs

            var context = TestUtils.CreateTestContext(sink);
            context.HttpContext.Request.Method = HttpMethods.Get;
-            context.HttpContext.Request.Headers[HeaderNames.Authorization] = "Basic plaintextUN:plaintextPW";
+            context.HttpContext.Request.Headers.Authorization = "Basic plaintextUN:plaintextPW";


@benaadams @JunTaoLuo can we use the string Placeholder for the pw here? Looks like this introduced a credscan bug (which is surprising since the creds haven't changed).

Bonus, so credscan recognises the new Authorization header; whereas it didn't in the old style?

Seems that way!

ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 4, 2021

davidfowl added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Apr 4, 2021

davidfowl added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 4, 2021

benaadams force-pushed the Faster-headers branch 2 times, most recently from cbad508 to 881e4db Compare April 4, 2021 03:37

benaadams changed the title ~~Use default interfaces to provide faster header lookups~~ Use default interfaces to provide ~15% cumulative gain per header Apr 4, 2021

benaadams force-pushed the Faster-headers branch 2 times, most recently from 8df0090 to 38ca2a6 Compare April 4, 2021 06:10

benaadams force-pushed the Faster-headers branch from 5e1ee91 to 7a06ae9 Compare April 4, 2021 11:10

NickCraver reviewed Apr 4, 2021

View reviewed changes

src/Http/Http.Features/src/HeaderNames.cs Outdated Show resolved Hide resolved

benaadams force-pushed the Faster-headers branch from 7a06ae9 to 3ceba0d Compare April 4, 2021 12:21

davidfowl reviewed Apr 4, 2021

View reviewed changes

src/Http/Http/src/Features/RequestCookiesFeature.cs Outdated Show resolved Hide resolved

Pilchie added area-runtime Perf labels Apr 5, 2021

BrennanConroy assigned davidfowl Apr 5, 2021

benaadams changed the title ~~Use default interfaces to provide ~15% cumulative gain per header~~ Use default interfaces for a more x3 gain in getting and setting headers Apr 6, 2021

IDisposable reviewed Apr 6, 2021

View reviewed changes

src/Http/Http.Features/src/HeaderNames.cs Outdated Show resolved Hide resolved

Tratcher reviewed Apr 6, 2021

View reviewed changes

pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 19, 2021

benaadams force-pushed the Faster-headers branch from 5e18d77 to 1f94bc3 Compare April 21, 2021 11:53

benaadams marked this pull request as ready for review April 21, 2021 11:55

benaadams requested review from a team, BrennanConroy, halter73, javiercn and jkotalik as code owners April 21, 2021 11:55

benaadams mentioned this pull request Apr 22, 2021

Rebalance common headers #31495

Merged

Tratcher self-assigned this Apr 22, 2021

davidfowl removed their assignment Apr 26, 2021

Use default interfaces for getting and setting headers

f31fd63

benaadams force-pushed the Faster-headers branch from 9bc7c11 to f31fd63 Compare April 26, 2021 18:42

davidfowl reviewed Apr 26, 2021

View reviewed changes

davidfowl approved these changes Apr 26, 2021

View reviewed changes

Tratcher added this to the 6.0-preview5 milestone Apr 26, 2021

halter73 approved these changes Apr 26, 2021

View reviewed changes

Tratcher approved these changes Apr 26, 2021

View reviewed changes

Tratcher merged commit 6427d9c into dotnet:main Apr 26, 2021

benaadams deleted the Faster-headers branch April 26, 2021 22:58

wtgodbe reviewed Apr 28, 2021

View reviewed changes

benaadams mentioned this pull request Apr 28, 2021

credscan false positive fix #32249

Merged

amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Use default interfaces for a more x3 gain in getting and setting headers #31519

Use default interfaces for a more x3 gain in getting and setting headers #31519

Uh oh!

Conversation

benaadams commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Faster header get set by using default interfaces.

Results

Example user code change to take advantage of fast-path (outside of the default features/middleware/mvc)

Snippet from the default interface partial as example

Mechanism

This is an RFC as it still would need:

Uh oh!

ghost commented Apr 4, 2021

Uh oh!

benaadams commented Apr 4, 2021

Uh oh!

Uh oh!

NickCraver commented Apr 4, 2021

Uh oh!

benaadams commented Apr 4, 2021

Uh oh!

davidfowl commented Apr 4, 2021

Uh oh!

Uh oh!

davidfowl commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Apr 4, 2021

Uh oh!

benaadams commented Apr 4, 2021

Uh oh!

benaadams commented Apr 5, 2021

Uh oh!

benaadams commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS commented Apr 6, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benaadams commented Apr 21, 2021

Uh oh!

davidfowl commented Apr 22, 2021

Uh oh!

benaadams commented Apr 26, 2021

Uh oh!

davidfowl Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Apr 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

benaadams commented Apr 4, 2021 •

edited

Loading

davidfowl commented Apr 4, 2021 •

edited

Loading

benaadams commented Apr 6, 2021 •

edited

Loading

davidfowl Apr 26, 2021 •

edited

Loading