Skip to content

[release/6.0] Remove two async state machines for typical HTTP/1.1 request path - #58252

Merged
stephentoub merged 5 commits into
release/6.0from
backport/pr-58092-to-release/6.0
Aug 30, 2021
Merged

[release/6.0] Remove two async state machines for typical HTTP/1.1 request path#58252
stephentoub merged 5 commits into
release/6.0from
backport/pr-58092-to-release/6.0

Conversation

@github-actions

@github-actionsgithub-actionsBot commented Aug 27, 2021

Copy link
Copy Markdown
Contributor

Backport of #58092 to release/6.0

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.

MethodToolchainMeanRatioAllocated
Request\5.0.9\corerun.exe56.31 us1.001,416 B
Request\main\corerun.exe56.60 us1.011,888 B
Request\pr\corerun.exe54.84 us0.971,464 B

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

/cc @stephentoub

Customer Impact

A simple HTTP/1.1 request made with SocketsHttpHandler allocated 1,416 bytes in .NET 5. In release/6.0 currently, that same request allocates 33% more. This changes drops that to almost noise, at just 3% more.

Testing

CI

Risk

Low. Logic didn't change; code was just moved between between async methods, getting rid of several.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #58092 to release/6.0

/cc @stephentoub

Customer Impact

Testing

Risk

Author:github-actions[bot]
Assignees:-
Labels:

area-System.Net.Http

Milestone:-

@stephentoub

Copy link
Copy Markdown
Member

cc: @danmoseley

@karelz
karelz requested a review from danmoseleyAugust 27, 2021 11:19
@karelzkarelz added this to the 6.0.0 milestone Aug 27, 2021
@danmoseley

Copy link
Copy Markdown
Contributor

Approved, as fixing significant regression to mainstream scenario with "low" risk.

@danmoseley
danmoseley requested a review from a teamAugust 27, 2021 15:22
@stephentoub
stephentoub merged commit f4e10ed into release/6.0Aug 30, 2021
@stephentoub
stephentoub deleted the backport/pr-58092-to-release/6.0 branch August 30, 2021 02:14
@ghostghost locked as resolved and limited conversation to collaborators Sep 29, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@stephentoub@danmoseley@geoffkizer@karelz