Skip to content

Conversation

@stephentoub
Copy link
Member

For a request like this (which I created just by looking at what headers my browser is sending):

var m = new HttpRequestMessage(HttpMethod.Get, uri);
AddHeader(m, "Accept", "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9");
AddHeader(m, "accept-encoding", "gzip, deflate, br");
AddHeader(m, "accept-language", "en-US,en;q=0.9");
AddHeader(m, "cache-control", "no-cache");
AddHeader(m, "pragma", "no-cache");
AddHeader(m, "sec-fetch-dest", "document");
AddHeader(m, "sec-fetch-mode", "navigate");
AddHeader(m, "sec-fetch-site", "none");
AddHeader(m, "sec-fetch-user", "?1");
AddHeader(m, "upgrade-insecure-requests", "1");
AddHeader(m, "user-agent", "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4090.0 Safari/537.36 Edg/83.0.467.0");

using (var resp = await c.SendAsync(m, default))
using (var respStream = await resp.Content.ReadAsStreamAsync())
{
    await respStream.CopyToAsync(Stream.Null);
}

to a default web api application, this PR reduces allocation by ~20%. This is a benchmark.net run of doing the above 100K times in parallel per iteration (and with the server on the same machine, so grain of salt on the timings... the most interesting numbers are the GC/allocation-related ones):

Method Toolchain Mean Ratio Gen 0 Gen 1 Allocated
Request \master\corerun.exe 1.653 s 1.00 60000.0000 20000.0000 351.92 MB
Request \pr\corerun.exe 1.631 s 0.99 46000.0000 15000.0000 272.61 MB

Commit 1: Just manually inlines a small async method. This removes its state machine allocation in the common case, but it also really didn't need to be its own method.

Commit 2: Today whenever a header is added to a header collection with TryAddWithoutValidation, it's wrapped in a HeaderStoreItemInfo, even though we could just store the raw string directly. This PR changes it so that we store the raw string, and only upgrade it to being wrapped when necessary, e.g. whenever we actually parse it, if we need to. I'm interested in feedback as to whether folks think it makes the code too complicated / whether it's worthwhile.

Commit 3: Caches the Date and Server header values on a connection. Since Server should generally never change for all requests on a connection, and Date should only change once a second-ish, we can avoid those string allocations in the typical high-throughput case. Again, I'm interested in feedback on whether folks see a better way to do this.

I tried this with the ASP.NET HttpClient benchmark, and it had a small but repeatable positive impact of around 1-2% on RPS.

Depends on #34667
cc: @scalablecory, @davidsh, @dotnet/ncl

@ghost
Copy link

ghost commented Apr 8, 2020

Tagging @dotnet/ncl as an area owner

@wfurt
Copy link
Member

wfurt commented Apr 9, 2020

looks suspicious.

System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_HttpClientHandlerTest_Headers_Http11.GetAsync_EmptyResponseHeader_Success [FAIL]
      System.InvalidOperationException : Collection was modified; enumeration operation may not execute.
      Stack Trace:
        /_/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs(1201,0): at System.Collections.Generic.Dictionary`2.Enumerator.MoveNext()
        /_/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs(315,0): at System.Net.Http.Headers.HttpHeaders.GetEnumeratorCore()+MoveNext()
        /_/src/libraries/System.Linq/src/System/Linq/Count.cs(39,0): at System.Linq.Enumerable.Count[TSource](IEnumerable`1 source)
        /_/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs(157,0): at System.Net.Http.Functional.Tests.HttpClientHandlerTest_Headers.<>c__DisplayClass6_0.

@wfurt
Copy link
Member

wfurt commented Apr 9, 2020

hmm. I see the linked issue. that make sense.

Just manually inline the tiny helper that's only used from one call site.
For the common case of a raw string, let the dictionary store the raw string directly rather than always wrapping it in a HeaderStoreItemInfo.
For Server, we expect it to be the same for every request on the same connection.

For Date, if we're making lots of requests in a high-throughput fashion, we expect many responses on the same connection to have the same value.

In both cases, we compare the received value against the last one received, and if it's the same, reuse the same string.
@stephentoub
Copy link
Member Author

/azp list

@stephentoub
Copy link
Member Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephentoub stephentoub merged commit c06810b into dotnet:master Apr 11, 2020
@stephentoub stephentoub deleted the httpallocs2 branch April 11, 2020 18:21
return true;
}

internal static bool EqualsOrdinalAscii(string left, ReadOnlySpan<byte> right)
Copy link
Member

Choose a reason for hiding this comment

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

Can always pick up BytesOrdinalEqualsStringAndAscii from aspnetcore's ServerInfrastructure/StringUtilities

https://github.com/dotnet/aspnetcore/blob/c16d3364d4f83307ac3c6f74de15f8ca96b89759/src/Shared/ServerInfrastructure/StringUtilities.cs#L313-L463

Copy link
Member Author

Choose a reason for hiding this comment

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

@stephentoub stephentoub added the tenet-performance Performance related issue label Apr 15, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants