-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add CopyToAsync optimized implementations for StreamPipeReader #52159
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
See disccussion in dotnet#51147 (comment) and associated open issue dotnet#51272
1cdfd66
to
a9e712f
Compare
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeReader.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs
Outdated
Show resolved
Hide resolved
Thanks for the contribution @manandre. I'd prefer it if we didn't expose any code with the base implementation and instead optimized for calling as little functions inside of the CopyToAsync implementations inside of the I expected the calls to CopyToAsync to basically enumerate from readHead to readTail and copy that to the underlying destination (PipeWriter or Stream), and then in the Stream case, delegate completely to the Stream implementation. For the PipeWriter case, what you're doing is fine. |
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs
Show resolved
Hide resolved
fac46a9
to
347b061
Compare
src/libraries/System.IO.Pipelines/tests/StreamPipeReaderCopyToAsyncTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs
Outdated
Show resolved
Hide resolved
This change is looking great! We need to figure out the tests. |
I'm very happy with this change @manandre! |
Thanks @manandre ! I will be looking for more Pipelines work to give you 😉 |
Closes #51972
/cc @davidfowl