- Notifications
You must be signed in to change notification settings - Fork5.2k
Add CopyToAsync optimized implementations for StreamPipeReader#52159
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedMay 1, 2021
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 indotnet#51147 (comment) and associated open issuedotnet#51272
1cdfd66 toa9e712fCompareUh oh!
There was an error while loading.Please reload this page.
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeReader.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
davidfowl commentedMay 1, 2021
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
fac46a9 to347b061CompareUh oh!
There was an error while loading.Please reload this page.
src/libraries/System.IO.Pipelines/tests/StreamPipeReaderCopyToAsyncTests.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
davidfowl commentedMay 2, 2021
This change is looking great! We need to figure out the tests. |
davidfowl commentedMay 3, 2021
I'm very happy with this change@manandre! |
davidfowl commentedMay 5, 2021
Thanks@manandre ! I will be looking for more Pipelines work to give you 😉 |
Closes#51972
/cc@davidfowl