-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Several improvements to SocketsHttpHandler perf #41640
Conversation
When enumerating headers to write them out, do not force them to be parsed if the user explicitly asked for them not to be with "WithoutValidation". If the key/values are incorrectly formatted, the request may end up being written incorrectly on the wire, but that's up to the developer explicitly choosing it.
Several releases ago, when we weren't paying attention to ExpectContinue, we optimized away the backing field for it into a lazily-initialized collection; that made it cheaper when not accessed but more expensive when accessed, which was fine, as we wouldn't access it from the implementation and developers would rarely set it. But now SocketsHttpHandler checks it on every request, which means we're paying for the more expensive thing always. So, revert the optimization for this field.
dfe8860
to
866931b
Compare
First pass looks okay to me; will make a second pass later. Would love if we could avoid mixing logic changes with style changes -- reading this takes some effort :) |
Which style changes? Just the |
I'm curious/hopeful if this change will help with #22638. |
When we enumerate the headers to write them out, we currently allocate a string[] for each. We can instead just fill the same array over and over and over.
866931b
to
df25530
Compare
Re retrieving header values: It would be nice to avoid the string[] allocation here entirely. This is particularly relevant for HTTP3 since there's no easy way to reuse the string[] across requests -- each request is on its own QUIC stream. But I don't see an obvious way to do this. Maybe a callback model? We can defer that for now. |
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
* Do not force-parse TryAddWithoutValidation headers in SocketsHttpHandler When enumerating headers to write them out, do not force them to be parsed if the user explicitly asked for them not to be with "WithoutValidation". If the key/values are incorrectly formatted, the request may end up being written incorrectly on the wire, but that's up to the developer explicitly choosing it. * Revert HttpRequestHeaders.ExpectContinue optimization Several releases ago, when we weren't paying attention to ExpectContinue, we optimized away the backing field for it into a lazily-initialized collection; that made it cheaper when not accessed but more expensive when accessed, which was fine, as we wouldn't access it from the implementation and developers would rarely set it. But now SocketsHttpHandler checks it on every request, which means we're paying for the more expensive thing always. So, revert the optimization for this field. * Avoid allocating a string[] per header When we enumerate the headers to write them out, we currently allocate a string[] for each. We can instead just fill the same array over and over and over. Commit migrated from dotnet/corefx@4d346a9
Fixes https://github.com/dotnet/corefx/issues/39835
Commit 1:
Per https://github.com/dotnet/corefx/issues/39835#issuecomment-538755366, the intent of HttpHeaders.TryAddWithoutValidation was that we wouldn't validate the headers. But while we're currently not validating them as part of that API call itself, we're currently validating them as part of enumerating the headers to write the out to the wire, for both HTTP/1.1 and HTTP/2.0. Stop doing that. This does mean that we could end up writing out something that would render the HTTP request invalid, but that's the nature of "WithoutValidation" and what the developer asked for.
Commit 2:
Several releases ago, when we weren't paying attention to ExpectContinue, we optimized away the backing field for it into a lazily-initialized collection; that made it cheaper when not accessed but more expensive when accessed, which was fine, as we wouldn't access it from the implementation and developers would rarely set it. But now SocketsHttpHandler checks it on every request, which means we're paying for the more expensive thing always. So, revert the optimization for this field.
Commit 3:
When we enumerate the headers to write them out, we currently allocate a string[] for each. We can instead just fill the same array over and over and over.
Benchmark: