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

proposal: io, net: implement WriterTo for pipes #34624

Closed
Stebalien opened this issue Sep 30, 2019 · 9 comments · May be fixed by #32125
Closed

proposal: io, net: implement WriterTo for pipes #34624

Stebalien opened this issue Sep 30, 2019 · 9 comments · May be fixed by #32125
Labels
Projects
Milestone

Comments

@Stebalien
Copy link

@Stebalien Stebalien commented Sep 30, 2019

What version of Go are you using (go version)?

$ go version
go version go1.13 linux/amd64

Description

Currently, io.Copy(pipew, piper) requires an intermediate buffer and an intermediate copy. Implementing WriteTo on pipes would allow writes to be forwarded directly to the writer with no intermediate copies.

Implementation: #32125

@andybons andybons changed the title {io, net}: implement WriterTo for pipes proposal: {io, net}: implement WriterTo for pipes Sep 30, 2019
@gopherbot gopherbot added this to the Proposal milestone Sep 30, 2019
@gopherbot gopherbot added the Proposal label Sep 30, 2019
@odeke-em odeke-em changed the title proposal: {io, net}: implement WriterTo for pipes proposal: io, net: implement WriterTo for pipes Oct 2, 2019
Stebalien added a commit to Stebalien/go that referenced this issue Oct 6, 2019
Implements the WriterTo interface for io pipes to avoid intermediate buffers
when copying with `io.Copy`.

Updates golang#34624
Stebalien added a commit to Stebalien/go that referenced this issue Oct 6, 2019
Implements the WriterTo interface for net pipes to avoid intermediate buffers
when copying with `io.Copy`.

Fixes golang#34624
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 7, 2019

Change https://golang.org/cl/177977 mentions this issue: io, net: implement WriterTo for pipes

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

This seems fine in principle but I'm pretty scared by the big comment about recovering panics in the CL. Is that really necessary? Why?

@rsc rsc added this to Incoming in Proposals Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 22, 2020

This discussion seems to be waiting for an answer to my query from Nov 27, namely why is that CL so complex and what does recovering panics have to do with it?

@rsc rsc moved this from Incoming to Active in Proposals Jan 22, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 5, 2020

I understand the panic now, having looked more closely at the code. But I don't really understand why this is an important optimization. The extra code is very subtle. It avoids the intermediate copy, but how often does io.Pipe get used in performance-sensitive contexts?

It's weird, because io.Pipe already is zero-copy between the read and write ends.
This optimization only applies when you are using io.Copy(w, pr) where pr is an io.Pipe read side,
meaning there's another write end pw somewhere else, and there is a call pw.Write happening,
and the effect of all this is to end up forwarding that directly to w.Write.
But in that case, why have the io.Pipe at all?

@Stebalien

This comment has been minimized.

Copy link
Author

@Stebalien Stebalien commented Feb 5, 2020

I apologize for being slow to respond. This hasn't been high on my priority list lately (not a good reason to leave you hanging).

But in that case, why have the io.Pipe at all?

Interfaces and abstractions. For example, let's say I have some form of abstract "router" where I can ask this router to open a stream to some subsystem. If the subsystem is within the same process, it can give both sides of the connection an io.Pipe.

As you say, writes will already be zero copy with respect to reads. However, the other side will still need an intermediate buffer if they want to forward the data anywhere. With this optimization, I can proxy the local "connection" (e.g., to an HTTP response, local file, or some other subsystem) without having to copy.


On the other hand, this definitely isn't a critical optimization. I filed the PR because it seemed like an easy win but that was before the issues around panicing became apparent.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 12, 2020

It sounds like there is not a compelling reason to add this. Having extra interfaces and abstractions that amount to no-ops is not a great thing to do. In this case, instead of making the extra abstraction zero-copy, users should probably reach for removing an io.Pipe, as I mentioned above.

This seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals Feb 12, 2020
@Stebalien

This comment has been minimized.

Copy link
Author

@Stebalien Stebalien commented Feb 12, 2020

I agree with closing this, however:

Having extra interfaces and abstractions that amount to no-ops is not a great thing to do. In this case, instead of making the extra abstraction zero-copy, users should probably reach for removing an io.Pipe, as I mentioned above.

When possible and/or convenient. The motivation here was abstracting away the difference between remote and local endpoints without paying for this abstraction with additional overhead. The alternatives are to either pay for the abstraction with a copy (the current approach) or simply not abstract over this difference (leading to duplicate code for handling both cases).

@Stebalien Stebalien closed this Feb 12, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Feb 12, 2020
@rsc rsc moved this from Declined to Likely Decline in Proposals Feb 12, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 12, 2020

Sorry, I got confused by the closed proposal and set the mode wrong. We'll leave this open for one more week just to make sure it ends up in the minutes. But thanks for closing.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 26, 2020

No change in consensus, so declined.

@rsc rsc closed this Feb 26, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.