Reduce allocations when parsing response headers in WinHttpHandler

[justinvp]

There are currently many unnecessary allocations when creating the response message in WinHttpHandler.

The native WinHttpQueryHeaders function is called a few times to retrieve response information, such as the version string, reason phrase, and raw headers.

Each call to WinHttpQueryHeaders currently results in 3 allocations (9 total for the 3 calls):

1. The StringBuilder buffer passed to the native function to be filled in.
2. The char[] inside the StringBuilder.
3. The string allocated when StringBuilder.ToString() is called.

On platforms that need to do manual decompression (lower than Win 8.1), there's another 2-3 allocations to get the Content-Encoding header value.

We can reduce the allocations by 1) modifying the p/invoke signature for WinHttpQueryHeaders to take a char[] instead of StringBuilder and 2) allocate a single char[] large enough to be reused across multiple calls to WinHttpQueryHeaders.

With those changes, 9 allocations are down to 2: an allocation for the raw headers char[] and another for the reason phrase string.

There are many common reason phrases, such as "OK" for a 200 status code, or "Not Found" for a 404 status code. There's already internal code in HttpStatusDescription for looking up a known reason phrase string based on the status code, and we can make use of this existing code to avoid the reason phrase string allocation if the reason phrase char[] array segment is equal to a known reason phrase. 9 allocations down to 1, for common reason phrases.

When parsing the raw headers, the current code is using string.Split to split the raw headers by lines separated by "\r\n". There are a bunch of unnecessary allocations associated with the use of string.split: internally string.Split allocates two arrays to keep track of the indexes and separator sizes, there's the overall array returned from Split, plus each string in the array representing each line. Each line then needs to be split further to get the header name and value for each line. The header value was also being trimmed with string.Trim, another potential allocation. Also, the first line of the raw headers is the status line, which is being allocated as a string even though this line is currently being skipped.

All of these intermediate allocations can be avoided by parsing the char[] directly, and only allocating strings for the individual header names and values. And we can avoid string allocations for known header names and values, similar to the known reason phrase optimization.

Notes:

• HttpKnownHeaderNames.TryGetHeaderName is looking up all known header name constants, which include both request and response headers, while we only really care about response headers. My opinion is that this is OK. I think it's easier to maintain the lookup of known headers if it is just looking up all known headers, instead of trying to segregate which ones are only response headers.
• HttpKnownHeaderNames.TryGetHeaderName is currently doing an ordinal lookup. Header names are case insensitive, so I'd be open to changing this to do an case insensitive lookup. I kept it ordinal so the string remains the exact same as the response string from WinHTTP. However, it looks like WinHTTP normalizes the casing of headers it knows about (e.g. WinHTTP returns "Content-Length" when the actual server response is "content-length"), so maybe switching to case insensitive lookup isn't a big deal.
• The PR is broken up into multiple commits to make it easier to review. The earlier commits are mostly just adding helpers and tests, and moving some of the internal files to Common (e.g. HttpStatusDescription). I can squash some of the commits together, if desired.
• There are still a bunch of allocations associated with how headers are enumerated and stored internally on each request/response in HttpClient that I'm looking into improving in a separate PR.

[davidsh]

cc: @CIPop @SidharthNabar @stephentoub

I'm in the process of some serious code re-arranging as I am working on #3000. So, I'm not sure if this PR will be handled now or later.

[davidsh]

I do want to comment on this aspect of the PR:

This isn't currently a difference between CurlHandler and WinHttpHandler, but we may want to make HttpResponseHeaderReader more lenient about potential white space characters between the http version and status code and between the status code and reason phrase. Right now, both handlers are only looking for a single space (per the RFC), but WinHTTP actually allows additional white space characters between these, which it normalizes to a single space character when WinHttpQueryHeaders is called with WINHTTP_QUERY_RAW_HEADERS_CRLF (on Windows 10; not sure about other versions). I haven't investigated what CURL allows and whether it normalizes the white space between these to a single character like WinHTTP.

WinHttpHandler does not parse out the http version, status code and reason phrase itself using WINHTTP_QUERY_RAW_HEADERS. We let WinHTTP do that for us and ask for those things separately via other WinHTTP header query APIs. WinHTTP already does the work of parsing that so there is no point in doing it ourselves. We only use WINHTTP_QUERY_RAW_HEADERS to get the headers themselves in order to parse that into our strongly typed header collections.

So, I'm not sure of the value of putting that particular parsing logic into Common since WinHttpHandler won't be using it.

[justinvp]

I'm not sure of the value of putting that particular parsing logic into Common since WinHttpHandler won't be using it.

I meant to call this out specifically. This is to avoid the allocations associated with retrieving the version and reason phrase char[]s. If we're already parsing those, it's easy enough to also parse the status code as part of that. We could still grab the status code directly from WinHTTP as there are no allocations associated with retrieving a number, or could add a Debug.Assert to ensure the status code we parsed is the same as WinHTTP.

[justinvp]

Thinking about it more, if we want to avoid parsing the status line ourselves (even though it looks like we have to do it for CURL) and get the version and reason phrase from WinHTTP, we could do it in a way that avoids the two intermediate char[] allocations for the version and reason phrase:

1. Get the size of the char[] buffer needed for WINHTTP_QUERY_RAW_HEADERS_CRLF, which would be big enough for either the version or reason phrase, because the raw headers includes the status line that has those.
2. Create a single char[] of that size, and use that same single char[] buffer for each of the interop calls to retrieve the version, reason phrase, and raw headers from WinHTTP.

[justinvp]

I removed the status line parsing from HttpResponseHeaderReader; PR updated. WinHttpHandler is back to getting the version, status code, and reason phrase by calling WinHttpQueryHeaders, but I've reduced the allocations by creating a single shared char[] buffer that's large enough to be reused across multiple calls to WinHttpQueryHeaders.

[CIPop]

@davidsh @justinvp While the description makes sense, I think the ordering should be:

1. Add the Async perf-enhancement to WinHttpHandler
2. Add PerformanceTests and analysis to compare CPU, memory and network utilization to existing stacks (HttpWebRequest, NodeJS, etc)
3. Add this change if the comparison with existing results shows improvements.

The PR is broken up into multiple commits to make it easier to review. The earlier commits are mostly just adding helpers and tests, and moving some of the internal files to Common (e.g. HttpVersion and HttpStatusDescription ). I can squash some of the commits together, if desired.

+1 on this PR method :)

[davidsh]

@justinvp Do you think you can refresh this PR so that we can consider it? Thx.

[justinvp]

@davidsh I've updated the PR, but for some reason CI doesn't run when I force push the changes after rebasing. I'm going to close and reopen the PR to see if that'll kick off a CI build.

[davidsh]

@stephentoub PTAL

[davidsh]

@stephentoub Do you have any further comment on this revision? Thx.

[stephentoub]

Sorry, David, I lost track of this... will review later today.

[stephentoub]

Nit: this comment is repeated from above

[justinvp]

Yeah, I intentionally added the same comment at both the top and bottom of the file. Based on your feedback, I went ahead and removed the comment at the bottom. One comment at the top should be sufficient.

[stephentoub]

It's OK if this is null

In what case would it be null?

[justinvp]

I removed the comment (it was a holdover from an earlier iteration that I should have been deleted).

[stephentoub]

Overall this looks good, but some comments/questions.

[justinvp]

I added a new commit that addresses the review feedback as well as another commit that tweaks one of the tests slightly. (I will squash after the review).

@stephentoub, I converted the Try methods to non-Try except for HttpKnownHeaderNames.TryGetHeaderName which I left as a Try* because testing found/not-found seems more straightforward with the Try* method. Though, I don't feel strongly about it as it certainly could return a string (allocating a new string if not a known header); I'd just have to update the tests to call the method more than once and check to see if the same object is returned for the found case and different objects for the not-found case.

Would it be possible for you to share a before/after look at what the allocations are involved in an HttpClient request on Windows with your PR?

I temporarily instrumented WinHttpResponseParser.CreateResponseMessage with Console.WriteLine(GC.GetTotalMemory(forceFullCollection: true)) at the beginning of the method and Console.WriteLine(GC.GetTotalMemory(forceFullCollection: false)) at the end of the method and ran the following before & after the changes in this PR:

using (var client = new HttpClient())
{
    var response = client.GetAsync("http://www.bing.com").GetAwaiter().GetResult();
}

Here are the results:

	GC.GetTotalMemory Beginning	GC.GetTotalMemory End
Before	146,920	179,688
After	146,920	171,496

[davidsh]

@stephentoub Do you have an final comments on this PR?

[stephentoub]

LGTM. Thanks, @justinvp.

Note that while the GC total memory before/after is interesting, it doesn't really reflect the savings from the PR, as you'd expect the GC to be running during the test and cleaning up rather than just letting things leak, plus other things contribute. I was more interested in understanding the frequency and amount of allocations happening during the test. I just ran a similar test:

using (HttpClient client = new HttpClient())
{
    for (int i = 0; i < 500; i++) 
    {
        client.GetAsync("http://httpbin.org/ip").GetAwaiter().GetResult().Dispose();
    }
}

Before:

After:

[davidsh]

LGTM

[justinvp]

Thanks guys.

@stephentoub, Ah, the profiler data is much better (thanks). Note: the allocation reductions aren't limited to String alone. Here are some other relevant types with reductions (running your same test):

Before:

After:

[CIPop]

@justinvp @davidsh @stephentoub I believe this is now allocating twice (or even 4x) the required buffer, for each request:

• 2x from the following code path: GetResponseHeaderCharBufferLength is calling into WinHttpQueryHeaders which returns the buffer size in bytes not C# char which is 16bit wide.
• another 2x from double buffering instead of lazily extracting headers as they are requested by the application.

The last point is debatable and would need to be profiled (which, if I have time, I will do part of my perf. work). Lacking the necessary telemetry it's hard to understand if most apps really care about the headers or not.

[justinvp]

If you look at the code before this PR you'll see that it was worse before... Previously it queried WINHTTP_QUERY_RAW_HEADERS_CRLF along with a few other headers. Each time it did so a StringBuilder would be allocated (along with underlying char[] buffer), then there would be copying going on back and forth as part of the interop marshaling, and then a call to ToString would allocate a string to return, so previously there was a lot of allocations and copying going on. With this PR, all those StringBuilder/char[]/string allocations (and copying) are reduced to a only a single char[] buffer allocation.

I agree it'd be nice if the headers were lazily extracted as requested by callers -- avoiding the buffer allocation and parsing, if unnecessary.

[CIPop]

@justinvp Thanks for explaining: I didn't get the time to closely look at the prior implementation.
I'll open 2 separate issues to track the problems since the first one is much simpler to solve.

[CIPop]

/cc @davidsh @himadrisarkar
Opened #11978 and #11979.