Remove two async state machines for typical HTTP/1.1 request path #58092
Conversation
stephentoub
added
area-System.Net.Http
tenet-performance
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsRather than having a dedicated async method for SendAndProcessAltSvcAsync, we can just have the sole call site call DetermineVersionAndSendAsync and ProcessAltSvc. And in DetermineVersionAndSendAsync, special-case the default configuration for SocketsHttpHandler that will result in HTTP/1.1 messages being sent, such that we avoid all the tests for HTTP/2 and HTTP/3 and the async state machine for DetermineVersionAndSendAsync when HTTP/1.1 is being requested and used. This doesn't entirely get us back to the allocation profile of .NET 5, but it's much closer.
I also noticed an unused parameter to the HTTP/2 and HTTP/3 try send async methods, so I removed those as well, removing those parameters from the corresponding state machines. Fixes #57977
|
|
Should we keep #57977 open and consider other improvements, or do we think this is good for 6.0? |
The changes in this PR seemed reasonable to me even without a goal of reducing state machines, i.e. I don't feel like we're sacrificing layering or factoring or anything like that. Beyond these two, though, it seems like we would have to make such choices, which makes me uncomfortable. I think we can live with the resulting 10% allocation increase over .NET 5 for this raw, do-as-little-as-possible benchmark. The moment you start doing anything more real, this won't matter. And if you start touching headers, .NET 6 is already way better. That said, if you had ideas for getting rid of the SendUsingHttp11Async state machine (it's basically what remains now), I'd be interested. There's also the option of using the pooling async method builder, but I think it's late in the game at this point to be exploring that here. |
|
We could just inline GetHttp11ConnectionAsync into SendUsingHttp11Async. |
Would you be ok with that? That felt like something you were trying to keep separate. But if you're cool with it, I'll add it to this PR. |
I think it's fine. SendUsingHttp11Async is not doing much currently aside from calling GetHttp11ConnectionAsync and then actually doing the send on the connection. |
|
Oh, wait, that's not going to help. GetHttp11ConnectionAsync typically completes synchronously. It's SendUsingHttp11Async itself we'd need to inline, into SendWithRetryAsync. |
That doesn't seem crazy either. |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
Inlining it would look like: |
I think so, yeah. It doesn't seem that ugly to me. I kind of wish we had a better name than "SendWithRetryAsync" now, since it's also doing the version determination... but I don't have a great suggestion. It's basically the main Send routine now (the only thing that happens before this is proxy auth) so perhaps we could call it SendAsyncCore or similar? I dunno. Thoughts? Does this give a noticeable win vs previous changes? I imagine we could do something similar here to get rid of a state machine for each of HTTP2 and HTTP3, right? Not super important at this point, but perhaps something to consider in the future. That said, re HTTP3, I kind of want to refactor that a bit. Seems like some logic that's currently in TrySendUsingHttp3Async could instead be in GetHttp3ConnectionAsync, and the 421 retry logic could just be incorporated into our general retry logic. But none of that needs to happen for 6.0. |
|
Ok, I think this is reasonable. @geoffkizer please take another look. At this point, the simple scenario previously benchmarked is only 48 bytes more than .NET 5 (I could easily get it to 32 bytes by doing something unnatural I chose not to do).
|
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Show resolved
Hide resolved
|
Also, let's run outerloop before merging. |
|
Is the plan to backport to 6.0 as is? If so I'm fine with that (assuming clean outerloop run) |
|
/azp list |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
I'd like to, yes. I feel a lot better about an only ~3% increase in allocation than an ~33% increase in allocation, even for a minimal benchmark. |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1173957232 |
ghost
locked as resolved and limited conversation to collaborators
Rather than having a dedicated async method for SendAndProcessAltSvcAsync, we can just have the sole call site call DetermineVersionAndSendAsync and ProcessAltSvc. And in DetermineVersionAndSendAsync, special-case the default configuration for SocketsHttpHandler that will result in HTTP/1.1 messages being sent, such that we avoid all the tests for HTTP/2 and HTTP/3 and the async state machine for DetermineVersionAndSendAsync when HTTP/1.1 is being requested and used.
This doesn't entirely get us back to the allocation profile of .NET 5, but it's much closer.
I also noticed an unused parameter to the HTTP/2 and HTTP/3 try send async methods, so I removed those as well, removing those parameters from the corresponding state machines.
Best reviewed without whitespace diffing: https://github.com/dotnet/runtime/pull/58092/files?w=1
Fixes #57977