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.

Sign up for GitHub

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

Jump to bottom

StreamPipeReader does not amortize ValueTask ReadAsync calls #30169

Closed
benaadams opened this issue Jul 7, 2019 · 5 comments · Fixed by #68457
Closed

StreamPipeReader does not amortize ValueTask ReadAsync calls #30169

benaadams opened this issue Jul 7, 2019 · 5 comments · Fixed by #68457
Assignees
@davidfowl
Labels
area-System.IO.Pipelines
Milestone
7.0.0

Comments

@benaadams
Copy link
Member

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

Using the ValueTask overloads for StreamPipeReader read allocates a AsyncStateMachineBox per read (when data is not immediately available)

image

However it could use TryRead for the sync-path and allocating a IValueTaskSource object the first time that fails to back the async read, and then reuse it each time the read needs to go async again.

Related: dotnet/aspnetcore#11940
@davidfowl davidfowl changed the title StreamPipeReaders don't amortize their ValueTask ReadAsync calls PipeReaderStream does not amortize their ValueTask ReadAsync calls Jul 7, 2019
@davidfowl davidfowl changed the title PipeReaderStream does not amortize their ValueTask ReadAsync calls PipeReaderStream does not amortize ValueTask ReadAsync calls Jul 7, 2019
@benaadams benaadams changed the title PipeReaderStream does not amortize ValueTask ReadAsync calls StreamPipeReader does not amortize ValueTask ReadAsync calls Jul 13, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@BrennanConroy BrennanConroy removed the untriaged New issue has not been triaged by the area owner label Jul 8, 2020
@davidfowl davidfowl modified the milestones: Future, 6.0.0 Apr 19, 2021
@davidfowl
Copy link
Member

This is a candidate for state machine pooling

@danmoseley
Copy link
Member

@davidfowl @halter73 @jkotalik this is marked 6.0. please either fix or change milestone by aug 17.

@davidfowl davidfowl self-assigned this Aug 7, 2021
@davidfowl
Copy link
Member

I'll take this one.

@stephentoub
Copy link
Member

Moving to .NET 7

@stephentoub stephentoub modified the milestones: 6.0.0, 7.0.0 Aug 30, 2021
@davidfowl
Copy link
Member

Yep I'll do it in 7

@davidfowl davidfowl mentioned this issue Apr 24, 2022
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 24, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 27, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@davidfowl davidfowl

Labels
area-System.IO.Pipelines
Projects
None yet
Milestone
7.0.0
7 participants
@davidfowl @benaadams @stephentoub @maryamariyan @danmoseley @BrennanConroy @msftgits