Skip to content

Consistently return StringValues.Empty instead of default for missing headers#46226

Merged
halter73 merged 7 commits into
mainfrom
halter73/44509
Mar 2, 2023
Merged

Consistently return StringValues.Empty instead of default for missing headers#46226
halter73 merged 7 commits into
mainfrom
halter73/44509

Conversation

@halter73
Copy link
Copy Markdown
Member

Fixes #44509

@ghost ghost added the area-runtime label Jan 23, 2023
@halter73 halter73 force-pushed the halter73/44509 branch 2 times, most recently from b9b0dfd to 06f85e0 Compare January 23, 2023 22:26
@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Jan 24, 2023

Could there be any performance impact here? There is lots of setting, getting and resetting of headers per request.

@davidfowl
Copy link
Copy Markdown
Member

This definitely needs a benchmark before this gets merged.

Copy link
Copy Markdown
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as TryGetValue still returns false.

Comment thread src/Shared/HttpSys/RequestProcessing/RequestHeaders.Generated.tt Outdated
Comment thread src/Shared/HttpSys/RequestProcessing/RequestHeaders.Generated.tt Outdated
Comment thread src/Shared/HttpSys/RequestProcessing/RequestHeaders.cs
@ghost
Copy link
Copy Markdown

ghost commented Feb 7, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 7, 2023
@halter73
Copy link
Copy Markdown
Member Author

halter73 commented Mar 2, 2023

HeaderCollectionBenchmark Microbenchmark results

Before

HeaderCollectionBenchmark (Non-empty values)

Method Type Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
SetHeaders ContentLengthNumeric 17.524 ns 0.2337 ns 0.2072 ns 57,063,164.5 - - - -
GetHeaders ContentLengthNumeric 5.473 ns 0.0458 ns 0.0657 ns 182,728,202.3 - - - -
SetHeaders ContentLengthString 33.655 ns 0.3636 ns 0.3401 ns 29,713,310.7 - - - -
GetHeaders ContentLengthString 10.344 ns 0.1054 ns 0.0986 ns 96,674,609.9 - - - -
SetHeaders Plaintext 32.613 ns 0.2117 ns 0.1877 ns 30,662,870.7 - - - -
GetHeaders Plaintext 9.606 ns 0.0880 ns 0.0687 ns 104,099,323.2 - - - -
SetHeaders Common 223.445 ns 0.1301 ns 0.1154 ns 4,475,382.8 - - - -
GetHeaders Common 29.346 ns 0.2203 ns 0.1840 ns 34,076,300.6 - - - -
SetHeaders Unknown 456.815 ns 3.8157 ns 3.5692 ns 2,189,068.3 - - - -
GetHeaders Unknown 145.684 ns 0.3838 ns 0.3205 ns 6,864,184.4 - - - -

HeaderCollectionBenchmark.GetHeaders (Empty values)

Method Type Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
GetHeaders ContentLengthNumeric 5.613 ns 0.1114 ns 0.1327 ns 178,163,323.8 - - - -
GetHeaders ContentLengthString 8.556 ns 0.0838 ns 0.0700 ns 116,875,653.8 - - - -
GetHeaders Plaintext 10.696 ns 0.2026 ns 0.1990 ns 93,489,301.0 - - - -
GetHeaders Common 34.490 ns 0.1691 ns 0.1582 ns 28,994,028.5 - - - -
GetHeaders Unknown 54.222 ns 0.1896 ns 0.1583 ns 18,442,826.8 - - - -

After

HeaderCollectionBenchmark (Non-empty values)

Method Type Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
SetHeaders ContentLengthNumeric 17.858 ns 0.1414 ns 0.1254 ns 55,996,048.3 - - - -
GetHeaders ContentLengthNumeric 5.296 ns 0.0842 ns 0.0936 ns 188,814,077.8 - - - -
SetHeaders ContentLengthString 35.876 ns 0.4560 ns 0.4265 ns 27,874,029.0 - - - -
GetHeaders ContentLengthString 10.194 ns 0.1119 ns 0.1047 ns 98,094,078.7 - - - -
SetHeaders Plaintext 33.039 ns 0.2072 ns 0.1938 ns 30,267,093.1 - - - -
GetHeaders Plaintext 9.366 ns 0.0912 ns 0.0853 ns 106,769,050.7 - - - -
SetHeaders Common 224.466 ns 0.2419 ns 0.2020 ns 4,455,008.9 - - - -
GetHeaders Common 31.516 ns 0.1768 ns 0.1568 ns 31,729,482.7 - - - -
SetHeaders Unknown 461.242 ns 3.2458 ns 3.0361 ns 2,168,059.7 - - - -
GetHeaders Unknown 151.437 ns 0.5270 ns 0.4929 ns 6,603,389.3 - - - -

HeaderCollectionBenchmark.GetHeaders (Empty values)

Method Type Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
GetHeaders ContentLengthNumeric 5.468 ns 0.0946 ns 0.0930 ns 182,888,637.4 - - - -
GetHeaders ContentLengthString 9.760 ns 0.1130 ns 0.1057 ns 102,457,565.7 - - - -
GetHeaders Plaintext 11.957 ns 0.2128 ns 0.1990 ns 83,636,108.4 - - - -
GetHeaders Common 39.305 ns 0.1598 ns 0.1416 ns 25,441,734.5 - - - -
GetHeaders Unknown 69.573 ns 0.1160 ns 0.0969 ns 14,373,433.1 - - - -

The biggest regression is in the "Unkown (Empty values) GetHeaders" microbenchmark which reads 3 known headers and then 8 unknown headers from the underlying "unknown" generic dictionary. This went from approximately 55 to 70 nanoseconds. Before this returned default instead of StringValues.Empty, so it makes sense there is some regression.

string ReadHeaders()
{
var headers = _response.Headers;
value = headers.Date;
value = headers.Server;
value = headers.ContentType;
value = headers.Link;
value = headers.XUACompatible;
value = headers.XPoweredBy;
value = headers.XContentTypeOptions;
value = headers.XXSSProtection;
value = headers.XFrameOptions;
value = headers.StrictTransportSecurity;
value = headers.ContentSecurityPolicy;
return value;
}

The next biggest regression is in"Common (Empty values) GetHeaders" microbenchmark which reads 18 known headers from fields within the source-genned dictionary. This went from approximately 34 to 39 nanoseconds.

string ReadHeaders()
{
var headers = _response.Headers;
var value = headers.Date;
value = headers.Server;
value = headers.ContentType;
value = headers.Connection;
value = headers.CacheControl;
value = headers.Vary;
value = headers.ContentEncoding;
value = headers.Expires;
value = headers.LastModified;
value = headers.SetCookie;
value = headers.ETag;
value = headers.TransferEncoding;
value = headers.ContentLanguage;
value = headers.Upgrade;
value = headers.Via;
value = headers.AccessControlAllowOrigin;
value = headers.AccessControlAllowCredentials;
value = headers.AccessControlExposeHeaders;
return value;
}

The microbenchmark results for setting and reading non-empty values seems unnaffected. I plan to merge this PR considering it fixes a functional inconsistency and only causes a minor performance regression when reading a lot of unset headers. We can keep an eye on the automated benchmarks after this is merged. @sebastienros

@halter73 halter73 removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 2, 2023
@halter73 halter73 enabled auto-merge (squash) March 2, 2023 22:10
@halter73 halter73 merged commit cd2907e into main Mar 2, 2023
@halter73 halter73 deleted the halter73/44509 branch March 2, 2023 23:13
@ghost ghost added this to the 8.0-preview3 milestone Mar 2, 2023
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(string[])Request.Headers["NonExistentHeader"] is null while (IEnumerable<string>)Request.Headers["NonExistentHeader"] is not

5 participants