Allow headers to match on ReferenceEquals before OrdinalIgnoreCase #9341

benaadams · 2019-04-13T05:00:13Z

Provide a performance advantage to using the Microsoft.Net.Http.Headers.HeaderNames constants.

Change HeaderNames to static properties rather than string constants; so reference equality works; prior to falling back to an OrdinalIgnoreCase characters comparison. (string constants embed directly in the calling assembly so don't end up being reference equal)

Use these properties in Kestrel header methods rather than its own string constants so the reference equality works there.

ResponseHeaderCollectionBenchmark (Common is the main change +18%)

      Method |                 Type |        Mean |         Op/s | Allocated |
 ----------- |--------------------- |------------:|-------------:|----------:|
- SetHeaders |               Common |   889.92 ns |  1,123,698.8 |       0 B |
+ SetHeaders |               Common |   751.84 ns |  1,330,077.7 |       0 B |
- SetHeaders |  ContentLengthString |    51.64 ns | 19,363,867.1 |       0 B |
+ SetHeaders |  ContentLengthString |    51.90 ns | 19,268,524.6 |       0 B |
- SetHeaders |            Plaintext |    55.45 ns | 18,034,860.0 |       0 B |
+ SetHeaders |            Plaintext |    51.40 ns | 19,455,753.2 |       0 B |
- SetHeaders |              Unknown | 1,539.01 ns |    649,770.4 |       0 B |
+ SetHeaders |              Unknown | 1,589.23 ns |    629,237.4 |       0 B |

The inlined string property becomes a constant mov at Jit time e.g.

if (ReferenceEquals(HeaderNames.Host, key))

becomes

G_M13073_IG06:
       mov      rdx, 0x15B8E12EBA8        ; HeaderNames:get_Host():ref
       cmp      gword ptr [rdx], rdi      ; ReferenceEquals
       jne      SHORT G_M13073_IG07

For plaintext with the default builder (including host filtering)

Before

After

Background

Noticed this while looking at something else #9359

If you use the HeaderNames class they don't match with reference equality e.g.

context.Request.Headers[HeaderNames.Host]

as they are defined in a different assembly to the string constants used in Kestrel; so it moves to an OrdinalIgnoreCase compare. Using the same HeaderNames header names in Kestrel means it will first match using ReferenceEquals in coreclr

public bool Equals(string? value, StringComparison comparisonType)
{
    if (object.ReferenceEquals(this, value))
    {
        CheckStringComparison(comparisonType);
        return true;
    }

and won't need to fallback to the OrdinalIgnoreCase when HeaderNames is used e.g when setting

response.ContentType = "text/plain";

as DefaultHttpResponse also uses these constants

set
{
    if (string.IsNullOrEmpty(value))
    {
        HttpResponseFeature.Headers.Remove(HeaderNames.ContentType);
    }
    else
    {
        HttpResponseFeature.Headers[HeaderNames.ContentType] = value;
    }
}

…Case

benaadams · 2019-04-14T02:43:04Z

ResponseHeaderCollectionBenchmark

      Method |                 Type |        Mean |         Op/s | Allocated |
 ----------- |--------------------- |------------:|-------------:|----------:|
- SetHeaders |               Common |   889.92 ns |  1,123,698.8 |       0 B |
+ SetHeaders |               Common |   751.84 ns |  1,330,077.7 |       0 B |
- SetHeaders |  ContentLengthString |    51.64 ns | 19,363,867.1 |       0 B |
+ SetHeaders |  ContentLengthString |    51.90 ns | 19,268,524.6 |       0 B |
- SetHeaders |            Plaintext |    55.45 ns | 18,034,860.0 |       0 B |
+ SetHeaders |            Plaintext |    51.40 ns | 19,455,753.2 |       0 B |
- SetHeaders |              Unknown | 1,539.01 ns |    649,770.4 |       0 B |
+ SetHeaders |              Unknown | 1,589.23 ns |    629,237.4 |       0 B |

benaadams · 2019-04-14T19:18:39Z

src/Mvc/test/WebSites/BasicWebSite/RequestIdMiddleware.cs

@@ -26,7 +27,7 @@ public Task Invoke(HttpContext context)
                throw new InvalidOperationException("RequestId should be null here");
            }

-            var requestId = context.Request.Headers["RequestId"];


Note: this was "RequestId" but HostingApplicationDiagnostics.cs uses "Request-Id"; now they are both using the same value via HeaderNames.RequestId (of "Request-Id")

davidfowl · 2019-04-14T23:25:43Z

src/Http/Headers/ref/Microsoft.Net.Http.Headers.netcoreapp3.0.cs

@@ -118,74 +118,87 @@ public partial class EntityTagHeaderValue
    }
    public static partial class HeaderNames
    {
-        public const string Accept = "Accept";


I think this is a breaking change? These can't be used in attributes anymore?

Indeed. Do we have any examples where it's relevant? I have not seen these used in attributes.

Was one in http2 tests; though that was testing the headers

Wouldn't think it was common? e.g. everything in ASP.NET Core repos work and this was the only change.

Suppose its a choice:

Fast header string matching (extremely common; used in every request, 1258 uses in ASP.NET Core itself)

Use in attributes (extremely uncommon, 2 test uses in ASP.NET Core itself and those testing http/2 protocol psudeo headers)

It’s not common no. The only other thing is that I’d like to eventually burn the headers assembly to the ground

Keeping it would involve duplication, which would get pretty confusing.

To caveat the scope, its source code breaking if used in attributes or other string const initalization; otherwise isn't source code breaking.

Oh I didn't see the whole change. I think this change is fine then. The few cases I found on the internet were not in attribute cases where a const is required. They were just regular imperative code. Carry on.

We use these quite a bit in case statements for what it's worth...though I guess not anymore. Bleh.

src/Http/Headers/src/HeaderNames.cs

halter73 · 2019-04-15T18:52:26Z

src/Servers/Kestrel/shared/KnownHeaders.cs

+                "Request-Id",
+                "Correlation-Context",
+                "TraceParent",
+                "TraceState"


So I see you did come up with a few more non-HTTP/2 headers. Do these make sense to you @Tratcher?

They are from hosting as it was checking each header that was the same length with IgnoreOrdinalCase for

if (!headers.TryGetValue(TraceParentHeaderName, out var correlationId)) { headers.TryGetValue(RequestIdHeaderName, out correlationId);

etc

halter73 · 2019-04-15T18:58:13Z

src/Servers/Kestrel/shared/KnownHeaders.cs

-                    }}
-                    break;")}
+                {{{Each(byLength.OrderBy(h => !h.PrimaryHeader), header => $@"
+                    if (ReferenceEquals(HeaderNames.{header.Identifier}, key))


Is manually calling object.ReferenceEquals and then following up with string.Equals StringComparison.OrdinalIgnoreCase really faster than doing the string comparison up front? If so, shouldn't we just add an object.ReferenceEquals fast path to the start of string.Equals? Or at the very least add a helper method to Kestrel that does this?

Also, AFIACT the body of this if block is identical to the body of the string.Equals if block below. Wouldn't it be better to just add || HeaderNames.{header.Identifier}.Equals(key, StringComparison.OrdinalIgnoreCase) to this if condition even if it isn't part of some helper method?

string.Equals does have a ReferenceEquals at the start (though its uninlinable since its kinda chunky)

The two issues this resolves are

Quick inlined reference check

Check all the references first before checking IgnoreOrdinalCase

So if there were 3 headers of that length; and it was the 3rd header it would do

ReferenceEquals, IgnoreOrdinalCase, ReferenceEquals, IgnoreOrdinalCase, ReferenceEquals => match

With this approach it instead does

ReferenceEquals, ReferenceEquals, ReferenceEquals => match

Only if none of the ReferenceEquals match does it then start doing IgnoreOrdinalCase.

Noticed this issue after adding TraceParent and trying to workout why ContentMD5 was being tested IgnoreOrdinalCase.

halter73 · 2019-04-15T19:00:40Z

src/Servers/Kestrel/shared/KnownHeaders.cs

+                        return false;")}
+                    }}")}
+    {Each(byLength.OrderBy(h => !h.PrimaryHeader), header => $@"
+                    if (HeaderNames.{header.Identifier}.Equals(key, StringComparison.OrdinalIgnoreCase))


Everywhere there are two if blocks in a row with identical bodies and one checking object.ReferenceEquals and the other checking string.Equals, I have the same questions I did for TryGetValueFast.

As above. Checks all the ReferenceEquals first; then does OrdinalIgnoreCase; rather than interleaving the IgnoreCase checks

Tratcher

@anurse @Eilon for breaking change awareness.

src/Middleware/Session/ref/Microsoft.AspNetCore.Session.csproj

benaadams · 2019-04-16T20:51:12Z

Build: Windows x64/x86 Job failed on a YARN error (transient)

Eilon · 2019-04-16T20:55:12Z

I think @javiercn was looking at YARN reliability.

benaadams · 2019-04-16T21:03:14Z

I think @javiercn was looking at YARN reliability.

Can you restart that job? 😉 I only have the power to restart everything, not an individual item.

halter73 · 2019-04-16T21:06:58Z

Can you restart that job? 😉 I only have the power to restart everything, not an individual item.

Generally I wait until all the builds are complete before triaging and restarting the failed build. I'm not sure if you can restart just the failed build before then. I'm setting a reminder to requeue once this is finished.

javiercn · 2019-04-16T21:23:57Z

I think @javiercn was looking at YARN reliability.

Yeah, but this PR needs to be rebased on top of master. I just moved the yarn install for all projects to the beginning of the build and made them happen sequentially, that seems to have fixed the issue you are seeing.

halter73 · 2019-04-16T21:30:47Z

Yeah, but this PR needs to be rebased on top of master.

Doesn't our PR validation automatically rebase on the target branch?

javiercn · 2019-04-16T22:00:18Z

Doesn't our PR validation automatically rebase on the target branch?

It doesn't AFAIK. It simply checks out the repo/branch and builds it, you still have to deal with merges/rebases.

halter73 · 2019-04-17T03:33:14Z

Looks good to go. I'll merge this tomorrow so I remember to make the breaking change announcement at the same time.

analogrelay · 2019-04-17T15:00:02Z

It doesn't AFAIK. It simply checks out the repo/branch and builds it, you still have to deal with merges/rebases.

It doesn't rebase, but it does use the "merge ref" from GitHub (refs/pull/9341/merge for example) which contains the merge of this PR and master. Though, I don't know when GitHub updates this ref (do they do it for every master commit or just for every commit to the PR).

analogrelay · 2019-04-17T15:00:56Z

At a minimum, it's safe to assume the build is incorporating all changes from the most recent commit to the PR branch and all changes that were in master at the time the most recent PR commit was made

Eilon · 2019-04-17T15:49:06Z

Can probably close & re-open the PR to get the master ref to update and re-try?

benaadams · 2019-04-18T01:44:15Z

@davidfowl merge conflicted me

…nce-equal-headernames

benaadams · 2019-04-18T03:38:08Z

merge conflict resolved

halter73 · 2019-04-18T18:59:53Z

aspnet/Announcements#356

benaadams requested review from analogrelay, jkotalik and Tratcher as code owners April 13, 2019 05:00

benaadams changed the title ~~Allow headers to match on reference Equality before OrdinalIgnoreCase~~ [WIP] Allow headers to match on reference Equality before OrdinalIgnoreCase Apr 13, 2019

benaadams force-pushed the reference-equal-headernames branch from 4988c1a to 252a77c Compare April 13, 2019 16:48

benaadams changed the title ~~[WIP] Allow headers to match on reference Equality before OrdinalIgnoreCase~~ Allow headers to match on reference Equality before OrdinalIgnoreCase Apr 13, 2019

benaadams force-pushed the reference-equal-headernames branch from c19add9 to 9efd8a7 Compare April 13, 2019 20:32

Allow headers to match on ReferenceEquals before trying OrdinalIgnore…

bbd1c8a

…Case

benaadams force-pushed the reference-equal-headernames branch from 9efd8a7 to bbd1c8a Compare April 14, 2019 02:32

benaadams mentioned this pull request Apr 14, 2019

Improve HostFiltering #9359

Merged

benaadams requested review from BrennanConroy, halter73 and mikaelm12 as code owners April 14, 2019 16:01

benaadams force-pushed the reference-equal-headernames branch 3 times, most recently from cc8986d to bfdf578 Compare April 14, 2019 19:07

benaadams commented Apr 14, 2019

View reviewed changes

Use HeaderNames more consistently

82f897c

benaadams force-pushed the reference-equal-headernames branch from bfdf578 to 82f897c Compare April 14, 2019 20:18

benaadams changed the title ~~Allow headers to match on reference Equality before OrdinalIgnoreCase~~ Allow headers to match on ReferenceEquals before OrdinalIgnoreCase Apr 14, 2019

davidfowl reviewed Apr 14, 2019

View reviewed changes

benaadams mentioned this pull request Apr 15, 2019

Enhancements for Dynamic Endpoints #9372

Merged

analogrelay removed their request for review April 15, 2019 14:49

natemcmaster added the area-servers label Apr 15, 2019

halter73 reviewed Apr 15, 2019

View reviewed changes

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

halter73 reviewed Apr 15, 2019

View reviewed changes

Tratcher reviewed Apr 15, 2019

View reviewed changes

src/Middleware/Session/ref/Microsoft.AspNetCore.Session.csproj Outdated Show resolved Hide resolved

Internal the consts

c9d21ce

benaadams force-pushed the reference-equal-headernames branch from 93e7b1e to c9d21ce Compare April 16, 2019 20:11

benaadams added 2 commits April 18, 2019 03:04

Merge remote-tracking branch 'refs/remotes/aspnet/master' into refere…

6fef5a7

…nce-equal-headernames

undo accidental change

8db0ab3

halter73 merged commit 8fcadf7 into dotnet:master Apr 18, 2019

benaadams deleted the reference-equal-headernames branch April 18, 2019 19:01

jongalloway mentioned this pull request Apr 18, 2019

[Discussion] HeaderNames now contains static readonly fields instead of const fields #9514

Closed

benaadams mentioned this pull request Apr 19, 2019

Proxy use HeaderNames rather than string const aspnet/Benchmarks#903

Merged

Tratcher mentioned this pull request Jun 26, 2019

[Microsoft.AspNetCore.Authentication.JwtBearer; 3.0.0-preview5-19227-01+] MissingFieldException #11593

Closed

analogrelay mentioned this pull request Jul 22, 2019

Make HTTP request methods as const strings #12439

Closed

benaadams mentioned this pull request Feb 8, 2020

String Deduplication Design Doc dotnet/runtime#31971

Merged

benaadams mentioned this pull request Feb 24, 2020

Vectorize codegen of switch statements dotnet/runtime#32757

Closed

j82w mentioned this pull request Aug 26, 2020

Performance: Use static readonly instead of const Azure/azure-cosmos-dotnet-v3#1806

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow headers to match on ReferenceEquals before OrdinalIgnoreCase #9341

Allow headers to match on ReferenceEquals before OrdinalIgnoreCase #9341

benaadams commented Apr 13, 2019 •

edited

benaadams commented Apr 14, 2019

benaadams Apr 14, 2019

davidfowl Apr 14, 2019

Tratcher Apr 14, 2019 •

edited

benaadams Apr 15, 2019 •

edited

benaadams Apr 15, 2019

davidfowl Apr 15, 2019

Tratcher Apr 16, 2019

benaadams Apr 16, 2019

Eilon Apr 16, 2019

NickCraver Apr 18, 2019

benaadams Apr 18, 2019

halter73 Apr 15, 2019

benaadams Apr 15, 2019 •

edited

halter73 Apr 15, 2019

benaadams Apr 15, 2019

halter73 Apr 15, 2019

benaadams Apr 15, 2019

Tratcher left a comment

benaadams commented Apr 16, 2019

Eilon commented Apr 16, 2019

benaadams commented Apr 16, 2019

halter73 commented Apr 16, 2019

javiercn commented Apr 16, 2019

halter73 commented Apr 16, 2019

javiercn commented Apr 16, 2019

halter73 commented Apr 17, 2019

analogrelay commented Apr 17, 2019

analogrelay commented Apr 17, 2019

Eilon commented Apr 17, 2019

benaadams commented Apr 18, 2019

benaadams commented Apr 18, 2019

halter73 commented Apr 18, 2019

Allow headers to match on ReferenceEquals before OrdinalIgnoreCase #9341

Allow headers to match on ReferenceEquals before OrdinalIgnoreCase #9341

Conversation

benaadams commented Apr 13, 2019 • edited

benaadams commented Apr 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher Apr 14, 2019 • edited

Choose a reason for hiding this comment

benaadams Apr 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams Apr 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher left a comment

Choose a reason for hiding this comment

benaadams commented Apr 16, 2019

Eilon commented Apr 16, 2019

benaadams commented Apr 16, 2019

halter73 commented Apr 16, 2019

javiercn commented Apr 16, 2019

halter73 commented Apr 16, 2019

javiercn commented Apr 16, 2019

halter73 commented Apr 17, 2019

analogrelay commented Apr 17, 2019

analogrelay commented Apr 17, 2019

Eilon commented Apr 17, 2019

benaadams commented Apr 18, 2019

benaadams commented Apr 18, 2019

halter73 commented Apr 18, 2019

benaadams commented Apr 13, 2019 •

edited

Tratcher Apr 14, 2019 •

edited

benaadams Apr 15, 2019 •

edited

benaadams Apr 15, 2019 •

edited