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

io: implement WriterTo on result of MultiReader #50842

Closed
Jorropo opened this issue Jan 27, 2022 · 8 comments
Closed

io: implement WriterTo on result of MultiReader #50842

Jorropo opened this issue Jan 27, 2022 · 8 comments

Comments

@Jorropo
Copy link
Member

Jorropo commented Jan 27, 2022

I have a program where I use io#MultiReader to concatenate a bytes#Buffer with a *os#File.

However doing that I loose zero copying on the file.

I added io#WriterTo to io#MultiReader and in my case that makes the copy loop of my app about 120 times faster. I am performing zero-copies of big file, this explain such jump.

Anyway I think you shouldn't loose zero copy on MultiReader.

@gopherbot
Copy link

Change https://golang.org/cl/381194 mentions this issue: io: add WriterTo support to MultiReader

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 27, 2022
@rsc rsc changed the title proposal: io: Add support for WriterTo to MultiReader proposal: io: implement WriterTo on result of MultiReader Feb 2, 2022
@rsc rsc moved this from Incoming to Active in Proposals (old) Feb 2, 2022
@rsc
Copy link
Contributor

rsc commented Feb 2, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Feb 9, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Feb 9, 2022
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Feb 16, 2022
@rsc
Copy link
Contributor

rsc commented Feb 16, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: io: implement WriterTo on result of MultiReader io: implement WriterTo on result of MultiReader Feb 16, 2022
@rsc rsc modified the milestones: Proposal, Backlog Feb 16, 2022
@Jorropo
Copy link
Member Author

Jorropo commented Feb 16, 2022

@rsc thx for the update, I have a patch implementing that, should I wait for the next merge window or I open a PR right now ?
(or I leave someone else implementing that ?)

@ianlancetaylor
Copy link
Contributor

Please go ahead and send a pull request whenever is convenient for you. It won't be reviewed until after the 1.18 release is out, though. Thanks.

@gopherbot
Copy link

Change https://go.dev/cl/388455 mentions this issue: io: add WriterTo to MultiReader

Jorropo added a commit to Jorropo/go that referenced this issue Mar 5, 2022
Fixes golang#50842.

Third version, previous: https://golang.org/cl/388455

This patch allows to zerocopy using MultiReader.
This is done by MultiReader implementing WriterTo.

Each sub reader is copied using usual io copy helper and thus use
WriterTo or ReadFrom with reflection.

There is a special case for when a subreader is a MultiReader.
Instead of using copyBuffer which would call multiReader.WriteTo,
multiReader.writeToWithBuffer is used instead, the difference
is that the temporary copy buffer is passed along, saving
allocations for nested MultiReaders.

The workflow looks like this:
- multiReader.WriteTo (allocates 32k buffer)
  - multiReader.writeToWithBuffer
    - for each subReader:
      - is instance of multiReader ?
        - yes, call multiReader.writeToWithBuffer
        - no, call copyBuffer(writer, currentReader, buffer)
          - does currentReader implements WriterTo ?
           - yes, use use currentReader.WriteTo
           - no, does writer implement ReadFrom ?
             - yes, use writer.ReadFrom
             - no, copy using Read / Write with buffer

This can be improved by lazy allocating the 32k buffer.
For example a MultiReader of such types:
  MultiReader(
    bytes.Reader, // WriterTo-able
    bytes.Reader, // WriterTo-able
    bytes.Reader, // WriterTo-able
  )

Doesn't need any allocation, all copy can be done using bytes.Reader's
internal data slice. However currently we still allocate a 32k buffer
for nothing.

This optimisation has been omited for a future patch because of high
complexity costs for a non obvious performance cost (it need a benchmark).
This patch at least is on par with the previous multiReader.Read
workflow allocation wise.
@gopherbot
Copy link

Change https://go.dev/cl/390215 mentions this issue: io: add WriterTo to MultiReader

Jorropo added a commit to Jorropo/go that referenced this issue Mar 7, 2022
Third version, previous: https://golang.org/cl/388455

This patch allows to zerocopy using MultiReader.
This is done by MultiReader implementing WriterTo.

Each sub reader is copied using usual io copy helper and thus use
WriterTo or ReadFrom with reflection.

There is a special case for when a subreader is a MultiReader.
Instead of using copyBuffer which would call multiReader.WriteTo,
multiReader.writeToWithBuffer is used instead, the difference
is that the temporary copy buffer is passed along, saving
allocations for nested MultiReaders.

The workflow looks like this:
- multiReader.WriteTo (allocates 32k buffer)
  - multiReader.writeToWithBuffer
    - for each subReader:
      - is instance of multiReader ?
        - yes, call multiReader.writeToWithBuffer
        - no, call copyBuffer(writer, currentReader, buffer)
          - does currentReader implements WriterTo ?
           - yes, use use currentReader.WriteTo
           - no, does writer implement ReadFrom ?
             - yes, use writer.ReadFrom
             - no, copy using Read / Write with buffer

This can be improved by lazy allocating the 32k buffer.
For example a MultiReader of such types:
  MultiReader(
    bytes.Reader, // WriterTo-able
    bytes.Reader, // WriterTo-able
    bytes.Reader, // WriterTo-able
  )

Doesn't need any allocation, all copy can be done using bytes.Reader's
internal data slice. However currently we still allocate a 32k buffer
for nothing.

This optimisation has been omited for a future patch because of high
complexity costs for a non obvious performance cost (it need a benchmark).
This patch at least is on par with the previous multiReader.Read
workflow allocation wise.

Fixes golang#50842
@dmitshur dmitshur modified the milestones: Backlog, Go1.19 Mar 8, 2022
@golang golang locked and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.