Reuse previous materialized strings #8374

benaadams · 2019-03-09T18:26:57Z

For each request from the browser a lot of repeated headers are sent that are identical to the previous request. This results in a lot of string allocations, especially for Accept, Cookie and User-Agent when materializing these values.

This change drops the sustained allocations to zero bytes/s for middleware with Json TE headers (e.g. 320MB less per 1M requests); will also drop header string allocation for MVC etc by the same amount.

Which fits with the goals of aspnet applications being able to run under lower memory conditions (See: "Proposal for .NET Core GC Support for Docker Limits")

This PR introduces DisableStringReuse which went false to reduces allocations for repeated request headers.

public partial class KestrelServerOptions
{
/// <summary>
/// Gets or sets a value that controls whether the string values materialized
/// will be reused across requests; if they match, or if the strings will always be reallocated.
/// </summary>
/// <remarks>
/// Defaults to false.
/// </remarks>
public bool DisableStringReuse { get; set; } = false;
}

Pre

               Method |              |       Mean |        Op/s |   Gen 0 | Allocated |
--------------------- |------------- |-----------:|------------:|--------:|----------:|
 PlaintextTechEmpower |              |   650.1 ns | 1,538,199.2 |  8.3008 |    168 KB |
           LiveAspNet |              | 1,487.8 ns |   672,119.6 | 26.3672 |    504 KB |

Post

               Method | ReuseHeaders |       Mean |        Op/s |   Gen 0 | Allocated |
--------------------- |------------- |-----------:|------------:|--------:|----------:|
 PlaintextTechEmpower |        False |   593.3 ns | 1,685,440.5 |  7.3242 |    144 KB |
           LiveAspNet |        False | 1,092.5 ns |   915,301.9 | 22.4609 |    440 KB |
                      |              |            |             |         |           |
 PlaintextTechEmpower |         True |   526.0 ns | 1,901,193.1 |       - |       0 B |
           LiveAspNet |         True |   871.9 ns | 1,146,875.8 |       - |       0 B |

This change drops ~320MB of allocations for ~1M requests of non-pipelined Json TE benchmark

wrk -c 128 -t 4 -d 45 -H "Accept: application/json,text/html;q=0.9,application/xhtml+xml;q=0.9,application/xml;q=0.8,*/*;q=0.7" -H "Connection: keep-alive" http://127.0.0.1:5001

Addresses #8372

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs

src/Servers/Kestrel/Core/ref/Microsoft.AspNetCore.Server.Kestrel.Core.netcoreapp3.0.cs

benaadams · 2019-03-10T00:49:07Z

Using a larger set of headers to represent a browser request

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9
Cache-Control: no-cache
Connection: keep-alive
Host: github.com
Pragma: no-cache
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.121 Safari/537.36
Cookie: prov=20629ccd-8b0f-e8ef-2935-cd26609fc0bc; __qca=P0-1591065732-1479167353442; _ga=GA1.2.1298898376.1479167354; _gat=1; sgt=id=9519gfde_3347_4762_8762_df51458c8ec2; acct=t=why-is-%e0%a5%a7%e0%a5%a8%e0%a5%a9-numeric&s=why-is-%e0%a5%a7%e0%a5%a8%e0%a5%a9-numeric

And a 2 minute run

wrk -c 128 -t 4 -d 120 -H "Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8" -H "Accept-Encoding: gzip, deflate, br" -H "Accept-Language: en-US,en;q=0.9" -H "Cache-Control: no-cache" -H "Connection: keep-alive" -H "Host: github.com" -H "Pragma: no-cache" -H "User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.121 Safari/537.36" -H "Cookie: prov=20629ccd-8b0f-e8ef-2935-cd26609fc0bc; __qca=P0-1591065732-1479167353442; _ga=GA1.2.1298898376.1479167354; _gat=1; sgt=id=9519gfde_3347_4762_8762_df51458c8ec2; acct=t=why-is-%e0%a5%a7%e0%a5%a8%e0%a5%a9-numeric&s=why-is-%e0%a5%a7%e0%a5%a8%e0%a5%a9-numeric" -s ./wrk/scripts/pipeline.lua http://127.0.0.1:5001 -- 16

Before

Pipeline depth: 16
Running 2m test @ http://127.0.0.1:5001
  4 threads and 128 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     5.44ms    3.95ms  56.46ms   76.52%
    Req/Sec    46.44k     9.64k   91.47k    68.45%
  22152336 requests in 2.00m, 2.72GB read
Requests/sec: 184499.18
Transfer/sec:     23.23MB

28GB of headers are allocated

After

Pipeline depth: 16
Running 2m test @ http://127.0.0.1:5001
  4 threads and 128 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     4.77ms    3.48ms  54.37ms   76.96%
    Req/Sec    52.39k    10.31k  113.12k    68.83%
  25000032 requests in 2.00m, 3.07GB read
Requests/sec: 208147.81
Transfer/sec:     26.20MB

0.3MB of headers are allocated

25M requests in 2 minutes is probably quite a high load though 😄

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs

src/Servers/Kestrel/Core/src/Internal/Infrastructure/StringUtilities.cs

benaadams · 2019-03-10T20:34:05Z

Updated perf test results

               Method | ReuseHeaders |       Mean |        Op/s |   Gen 0 | Allocated |
--------------------- |------------- |-----------:|------------:|--------:|----------:|
 PlaintextTechEmpower |        False |   676.1 ns | 1,479,039.7 |  8.3008 |    168 KB |
           LiveAspNet |        False | 1,423.3 ns |   702,610.6 | 25.3906 |    504 KB |
                      |              |            |             |         |           |
 PlaintextTechEmpower |         True |   588.4 ns | 1,699,502.5 |  0.4883 |     24 KB |
           LiveAspNet |         True | 1,233.7 ns |   810,542.5 |  2.9297 |     88 KB |

benaadams · 2019-03-12T02:22:01Z

Now faster than current when not reusing headers, and faster still when reusing headers

               Method | ReuseHeaders |       Mean |        Op/s |   Gen 0 | Allocated |
--------------------- |------------- |-----------:|------------:|--------:|----------:|
 PlaintextTechEmpower |        False |   629.3 ns | 1,589,177.2 |  8.7891 |    168 KB |
           LiveAspNet |        False | 1,357.9 ns |   736,414.0 | 26.3672 |    504 KB |
                      |              |            |             |         |           |
 PlaintextTechEmpower |         True |   552.5 ns | 1,809,801.6 |  0.9766 |     24 KB |
           LiveAspNet |         True | 1,180.8 ns |   846,871.4 |  3.9063 |     88 KB |

mattnischan · 2019-03-12T03:22:48Z

Any chance someone with less than altruistic intent could alter something like the "Accept" header value for everyone with some unsafe antics?

benaadams · 2019-03-12T10:32:18Z

Any chance someone with less than altruistic intent could alter something like the "Accept" header value for everyone with some unsafe antics?

Sort of, it would only change it for the request (the values are only reused on a single connection); and the next request wouldn't match via content equality so it would generate a new one; but then you could change it again.

However; that's a lot of work, since you can happily just replace the value in the headers collection without unsafe.

mattnischan · 2019-03-12T21:39:37Z

Ah OK, I was missing the part where the string is only reused for a connection, not across the application.

benaadams · 2019-03-12T23:05:49Z

was missing the part where the string is only reused for a connection, not across the application.

Also avoids slow down from contention 😉

benaadams · 2019-03-14T18:23:22Z

               Method | ReuseHeaders |       Mean |        Op/s |   Gen 0 | Allocated |
--------------------- |------------- |-----------:|------------:|--------:|----------:|
 PlaintextTechEmpower |        False |   583.3 ns | 1,714,322.7 |  7.3242 |    144 KB |
           LiveAspNet |        False | 1,275.7 ns |   783,867.2 | 24.4141 |    492 KB |
                      |              |            |             |         |           |
 PlaintextTechEmpower |         True |   523.0 ns | 1,912,075.5 |       - |       0 B |
           LiveAspNet |         True | 1,158.9 ns |   862,855.6 |  1.9531 |     76 KB |

benaadams · 2019-03-14T20:24:19Z

               Method | ReuseHeaders |       Mean |        Op/s |   Gen 0 | Allocated |
--------------------- |------------- |-----------:|------------:|--------:|----------:|
 PlaintextTechEmpower |        False |   593.3 ns | 1,685,440.5 |  7.3242 |    144 KB |
           LiveAspNet |        False | 1,092.5 ns |   915,301.9 | 22.4609 |    440 KB |
                      |              |            |             |         |           |
 PlaintextTechEmpower |         True |   526.0 ns | 1,901,193.1 |       - |       0 B |
           LiveAspNet |         True |   871.9 ns | 1,146,875.8 |       - |       0 B |

Tratcher · 2019-03-18T17:47:42Z

Would it be more productive / less dangerous to focus on the HTTP/2 HPACK scenario where this is a first class protocol concept? Or is that already performing well?

analogrelay · 2019-04-08T16:45:20Z

Sounds like we need a little more review here and can't squeeze it in to preview 4. Moving to preview 5. Let's try to close this out in the next day or so though (modulo any delays due to branching).

src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs

halter73 · 2019-04-08T23:27:20Z

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

+
+        private static bool GetDisableHeaderReuseSwitch()
+        {
+            if (AppContext.TryGetSwitch(DisableHeaderReuseSwitch, out var disableHeaderReuse))


Is there a switch to disable the similar changes to the target parsing?

No. I saw it more as a paranoia setting #8374 (comment) as the headers could contain the cookies/auth info (whereas you shouldn't be putting this in the path).

Also the alternate path for startline would be quite messy. Can add it though?

Is it really that messy? It is a little bit of paranoia, but I think having a switch that disable all reuse of previous materialize strings would relieve some of that paranoia.

When I was originally thinking about how to do it; I thought it would be two branches with the two forms of parsing.

However, can just add it as first term in the if tests; so no it wouldn't be messy.

@benaadams we were discussion this and think it should be a regular option with, not an app context switch. It should just be a single option that can also be read from configuration (like we do for a bunch of other settings in kestrel)

[Nevermind, I was confused which one you were referring to.]

Wondering if Options can be "baked" to readonly statics currently
e.g.

if (buffer.Length >= ServerOptions.Limits.MaxRequestLineSize)

is 3 indirections

G_M62071_IG06: 488B9780000000 mov rdx, gword ptr [rdi+128] 488B5218 mov rdx, gword ptr [rdx+24] 8B5220 mov edx, dword ptr [rdx+32] 4C63CA movsxd r9, edx 493BC9 cmp rcx, r9 7C37 jl SHORT G_M62072_IG07

if it can be "baked" then it would just be a const compare

G_M62071_IG06: 493BC9 cmp rcx, 8192 7C37 jl SHORT G_M62072_IG07

Probably not as it would cause a problem if you were running more than once Kestrel instance in a process :-/

Squished the api commits; just in case want it reverted b822a0f

A little difference; but not huge

Config

Method | Mean | Op/s | Scaled | Allocated | --------------------- |---------:|------------:|-------:|----------:| PlaintextTechEmpower | 541.7 ns | 1,846,001.2 | 1.00 | 0 B | LiveAspNet | 891.1 ns | 1,122,221.7 | 1.65 | 0 B |

Switch

Method | Mean | Op/s | Scaled | Allocated | --------------------- |---------:|------------:|-------:|----------:| PlaintextTechEmpower | 536.9 ns | 1,862,399.7 | 1.00 | 0 B | LiveAspNet | 878.1 ns | 1,138,780.0 | 1.64 | 0 B |

src/Servers/Kestrel/Core/test/HttpRequestHeadersTests.cs

halter73 · 2019-04-08T23:37:09Z

src/Servers/Kestrel/shared/KnownHeaders.cs

@@ -196,6 +93,8 @@ public static string GeneratedFile()
                "TE",
                "Translate",
                "User-Agent",
+                "DNT",
+                "Upgrade-Insecure-Requests"


👍 Any other headers we should consider adding to the common headers list while we're thinking about it? @Tratcher

I'd like to add a few more from https://github.com/aspnet/AspNetCore/blob/master/src/Http/Headers/src/HeaderNames.cs including the http/2 ones in that list of :authority,:method,:path, etc

Which was also part of my motivation for halving the size of StringValues dotnet/extensions#1283; so adding more headers wouldn't be too egregious.

However, was also looking to do that in a follow up, rather than this PR

Makes sense. We should probably add the http/2 headers separately.

src/Servers/Kestrel/shared/KnownHeaders.cs

halter73

I would like OnOriginFormTarget and OnAbsoluteTarget to be consistent regarding whether it explicitly resets QueryString to _parsedQueryString. I don't think it matters whether they do or don't, but they should be the same.

Co-Authored-By: benaadams <[email protected]>

benaadams · 2019-04-09T00:19:39Z

@halter73 feedback addressed? d15c3a5

benaadams · 2019-04-09T01:09:20Z

it should be a regular option with, not an app context switch

@davidfowl feedback addressed? b822a0f

halter73 · 2019-04-09T18:37:52Z

Thanks @benaadams!

benaadams · 2019-04-10T12:36:15Z

😸 🚀

benaadams force-pushed the reuse-previous-headers branch from 9ba21aa to 581d65b Compare March 9, 2019 21:03

davidfowl reviewed Mar 9, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs Outdated Show resolved Hide resolved

davidfowl reviewed Mar 9, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs Show resolved Hide resolved

davidfowl reviewed Mar 9, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs Show resolved Hide resolved

davidfowl reviewed Mar 9, 2019

View reviewed changes

src/Servers/Kestrel/Core/ref/Microsoft.AspNetCore.Server.Kestrel.Core.netcoreapp3.0.cs Outdated Show resolved Hide resolved

benaadams changed the title ~~Reuse previous headers~~ Reuse previous materialized header values Mar 9, 2019

benaadams force-pushed the reuse-previous-headers branch 2 times, most recently from 6bb0e81 to c4ae5a4 Compare March 9, 2019 22:37

jo-ninja reviewed Mar 10, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs Outdated Show resolved Hide resolved

jo-ninja reviewed Mar 10, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Infrastructure/StringUtilities.cs Outdated Show resolved Hide resolved

jo-ninja reviewed Mar 10, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Infrastructure/StringUtilities.cs Outdated Show resolved Hide resolved

Eilon added the area-servers label Mar 10, 2019

benaadams force-pushed the reuse-previous-headers branch from 53495b4 to 2cead3c Compare March 12, 2019 11:23

benaadams mentioned this pull request Mar 14, 2019

Avoid boxing allocations for async in Tier0 dotnet/coreclr#22984

Merged

benaadams force-pushed the reuse-previous-headers branch 2 times, most recently from d130f47 to 115a26a Compare March 14, 2019 17:29

benaadams changed the title ~~Reuse previous materialized header values~~ Reuse previous materialized strings Mar 14, 2019

benaadams force-pushed the reuse-previous-headers branch from 115a26a to f6674f3 Compare March 14, 2019 17:55

halter73 self-requested a review March 14, 2019 21:28

benaadams force-pushed the reuse-previous-headers branch from 03676cd to 22c3c6a Compare March 15, 2019 08:51

analogrelay modified the milestones: 3.0.0-preview4, 3.0.0-preview5 Apr 8, 2019

benaadams mentioned this pull request Apr 8, 2019

Use one SequenceReader+Rewind rather than 2 SequenceReaders #8076

Merged