-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Comments
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. |
ReadAsync is a bit wild... not jumping on that soon |
Triage: It should be solved by Stephen's dotnet/coreclr#26310 or we will use Ben's dotnet/corefx#39455. |
I'd prefer Stephen's as its more generic and wide reaching |
Though you linked to wrong issue as its a coreclr PR dotnet/coreclr#26310 rather than corefx one |
Thanks @benaadams |
Closing as it should be solved by the linked PR - dotnet/coreclr#26310 |
Not entirely; it does an |
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. |
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:
😉 |
Can you elaborate? I'm interested :) |
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)
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.
The text was updated successfully, but these errors were encountered: