Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid allocations in ClientWebSocket.ConnectAsync in the common case #75025

Merged
merged 3 commits into from
Sep 5, 2022

Conversation

MihaZupan
Copy link
Member

ClientWebSocketOptions.Proxy is set by default, so the shared handler wasn't being used in the common case.
This PR adds a second handler field that's used when a default proxy is requested.
While we're changing this code, I also changed it to store an HttpMessageInvoker instead of SocketsHttpHandler.

For the following test case

using var cws = new ClientWebSocket();
await cws.ConnectAsync(uri, CancellationToken.None);

This saves about ~10% of the allocations (20.164 => 17.940).

Before

image
image

After

image

@MihaZupan MihaZupan added this to the 8.0.0 milestone Sep 2, 2022
@MihaZupan MihaZupan requested a review from a team September 2, 2022 20:39
@ghost
Copy link

ghost commented Sep 2, 2022

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

Issue Details

ClientWebSocketOptions.Proxy is set by default, so the shared handler wasn't being used in the common case.
This PR adds a second handler field that's used when a default proxy is requested.
While we're changing this code, I also changed it to store an HttpMessageInvoker instead of SocketsHttpHandler.

For the following test case

using var cws = new ClientWebSocket();
await cws.ConnectAsync(uri, CancellationToken.None);

This saves about ~10% of the allocations (20.164 => 17.940).

Before

image
image

After

image

Author: MihaZupan
Assignees: -
Labels:

area-System.Net

Milestone: 8.0.0

@ghost ghost assigned MihaZupan Sep 2, 2022
@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
Copy link
Member Author

Outerloop failures are still #74468

@MihaZupan MihaZupan merged commit db4a954 into dotnet:main Sep 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 5, 2022
Sign up for free to 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.

2 participants