Reduce CurlHandler header parse allocations

[justinvp]

Use existing known header and reason phrase instances instead of always allocating new strings (similar to #3199 for WinHttpHandler) and avoid unnecessary intermediate string allocations.

cc: @stephentoub, @kapilash, @davidsh

(@stephentoub, is there a good way to profile the allocations for a before/after comparison on Unix?)

Note: I only tested this on OS X as I didn't have a Linux environment readily available; relying on CI for the other *nixes.

[stephentoub]

Thanks, @justinvp!

@stephentoub, is there a good way to profile the allocations for a before/after comparison on Unix?

PerfView has been updated with support for data captured on Linux:
http://blogs.msdn.com/b/vancem/archive/2016/02/20/analyzing-cpu-traces-from-linux-with-perfview.aspx
but I don't believe it's yet at a point where it can help with allocation analysis, though at some point it will be able to. In the meantime, you can do something very rudimentary and macro by looking at GC.CollectionCount(0) for example before and after some perf test.

[stephentoub]

Why not Func<T, int, char>? Similar question for the delegate below?

[justinvp]

I originally had Func<...> but changed them to concrete delegates to make the parameters more clear. Though, it doesn't matter all that much here. I can change it back to Func<...>s if you want.

[stephentoub]

I always have a small preference for not introducing new delegate types, but if you think it leads to the code being more understandable, it's ok to leave as-is.

[justinvp]

Switched back to Func.

[stephentoub]

This is very nice, Justin. Thanks for doing this. A few nits, otherwise LGTM.

[justinvp]

Thanks, Steve and David. I added another commit that addresses the feedback (and a few other minor nits I noticed when going over the code again). I will squash before merging.

I ran the following before/after on OS X:

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

Before: 4
After: 3

I suppose if I increased the iterations we might see a more dramatic drop in Gen0 GCs.

[kapilash]

This is great, Thanks @justinvp

[stephentoub]

Test Innerloop CentOS7.1 Debug Build and Test please
Test Innerloop OSX Debug Build and Test please

[justinvp]

We definitely want to be using Latin1 encoding (HttpRuleParser.DefaultHttpEncoding) over Marshal.PtrToStringAnsi to properly carry forward non-US-ASCII octets, to be consistent with WinHttpHandler and the RFC. I've updated the "Address PR feedback" commit.

I have found inconsistencies between libcurl and WinHTTP with how WinHTTP handles invalid chars. libcurl doesn't do much validation at all. It mostly just passes each header line to the header callback and in our callback we don't add much validation ourselves. For example, WinHTTP fails if a null char is in a header value, but libcurl doesn't complain (nor do we). I'll open a separate PR soon (with tests) to address this.

Let me know if the changes in this PR look good and I'll squash into a single commit.

[davidsh]

I have found inconsistencies between libcurl and WinHTTP with how WinHTTP handles invalid chars. libcurl doesn't do much validation at all. It mostly just passes each header line to the header callback and in our callback we don't add much validation ourselves. For example, WinHTTP fails if a null char is in a header value, but libcurl doesn't complain (nor do we). I'll open a separate PR soon (with tests) to address this.

If you have time, it would be a good datapoint to get similar test results on validation from .NET Framework (Desktop) using HttpClient (HttpClientHandler). That uses the original managed sockets stack.

[stephentoub]

I've updated the "Address PR feedback" commit.

Thanks!

I'll open a separate PR soon (with tests) to address this.

Sounds good. Thanks.

[stephentoub]

Let me know if the changes in this PR look good and I'll squash into a single commit.

LGTM. No need to squash; the two commits is fine.