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

WebSocket doesn't amortize its ValueTask ReadAsync calls #30167

Closed
benaadams opened this issue Jul 7, 2019 · 11 comments
Closed

WebSocket doesn't amortize its ValueTask ReadAsync calls #30167

benaadams opened this issue Jul 7, 2019 · 11 comments
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Milestone

Comments

@benaadams
Copy link
Member

In usage a WebSocket normally has very many ReadAsync calls made on it. (Use case WebSockets/SignalR over TLS)

Using the ValueTask overloads for WebSocket read allocates 2x AsyncStateMachineBox per read (when data is not immediately available)

image

However it could allocate once; backing with an IValueTaskSource objects that get allocated for the first async and reused when the read goes async again.

@stephentoub
Copy link
Member

It could, as can anything in theory that returns ValueTask (we don't need an issue for each). It's non-trivial and significantly complicates the implementation. Which is why it hasn't been done thus far.

@benaadams
Copy link
Member Author

ReadAsync is a bit wild... not jumping on that soon

@karelz
Copy link
Member

karelz commented Oct 1, 2019

Triage: It should be solved by Stephen's dotnet/coreclr#26310 or we will use Ben's dotnet/corefx#39455.

@benaadams
Copy link
Member Author

I'd prefer Stephen's as its more generic and wide reaching

@benaadams
Copy link
Member Author

Though you linked to wrong issue as its a coreclr PR dotnet/coreclr#26310 rather than corefx one

@scalablecory
Copy link
Contributor

Thanks @benaadams

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@karelz
Copy link
Member

karelz commented May 6, 2020

Closing as it should be solved by the linked PR - dotnet/coreclr#26310

@karelz karelz closed this as completed May 6, 2020
@benaadams
Copy link
Member Author

Not entirely; it does an .AsTask() in one of the paths converting a ValueTask to a Task; which could be avoided with some refactoring (though isn't completely straightforward)

@stephentoub
Copy link
Member

which could be avoided with some refactoring

What kind of refactoring are you thinking about? The challenge here I've faced whenever I've sat down to address this (and then sighed and walked away) is the potential consumption by not only the consumer of the receive but also a potentially concurrent close.

@benaadams
Copy link
Member Author

What kind of refactoring are you thinking about?

I did attempt it; it was on the basis that close could only happen once so I reversed the dependency, however the knock on effects turned out to be a lot more wide reaching than I was anticipating so I never finished it 😢

Leading me to the assessment:

isn't completely straightforward

😉

@stephentoub
Copy link
Member

I did attempt it; it was on the basis that close could only happen once so I reversed the dependency

Can you elaborate? I'm interested :)

@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

5 participants