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: add interface type StringBytesWriter #25464

Closed
dchenk opened this issue May 19, 2018 · 8 comments
Closed

proposal: io: add interface type StringBytesWriter #25464

dchenk opened this issue May 19, 2018 · 8 comments
Labels
FrozenDueToAge Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@dchenk
Copy link
Contributor

dchenk commented May 19, 2018

I propose that we add to package io:

type StringBytesWriter interface {
    io.Writer
    io.StringWriter  // the unexported stringWriter should be changed
    io.ByteWriter
}

This would solve a lot of inconveniences and improve efficiency in many places where a Writer represents something that writes string (or textual) data.

I often find that it’s undesirable for some exported functions to take as arguments or return a type like *bytes.Buffer or *bufio.Writer or *strings.Builder when users of a package in these cases should only be writing data, not otherwise modifying the state of the concrete type. All of these three types (*bytes.Buffer, *bufio.Writer, and *strings.Builder) and many more outside the stdlib already implement the proposed StringBytesWriter interface: it can be much more efficient to write a single byte or directly copy a string to the Writer without having to allocate a slice.

@gopherbot gopherbot added this to the Proposal milestone May 19, 2018
@dchenk
Copy link
Contributor Author

dchenk commented May 19, 2018

An alternative name: io.TextWriter

@mvdan
Copy link
Member

mvdan commented May 19, 2018

I'm not sure I follow how exposing this interface would improve efficiency. Interfaces are satisfied implicitly, so it does not matter whether or not the io package defines them. You might say adding it is more convenient, but it should not affect performance.

I'm also not sure that exposing interfaces that combine existing exposed interfaces is that helpful. Perhaps a better proposed change would be to expose io.StringWriter, as a start.

@dchenk
Copy link
Contributor Author

dchenk commented May 19, 2018

@mvdan you'd get better efficiency when you provide external packages the interface I described for writing instead of just an io.Writer when the writer is intended for string data. The point is that, if you've already got a string and you need to write it to an io.Writer, you need to convert it to a []byte, and this is an allocation. And if you've only got an io.Writer for writing, and you want to write a single byte, you need to make a slice out of it to pass it into Write. (But, as part of the Go language itself, strings and single bytes can be copied directly to any []byte.)

@meirf
Copy link
Contributor

meirf commented May 20, 2018

@dchenk
For completeness did you consider including a rune writer, which is also implemented by *bytes.Buffer, *bufio.Writer, and *strings.Builder?

many more outside the stdlib already implement

Can you please list a couple from popular repos? It might add weight to the proposal to see such examples in the wild.

@mvdan
Copy link
Member

mvdan commented May 20, 2018

@dchenk I understand the point of writing single bytes and strings. My point is that you can do this optimization without the io package exposing the interface. For example:

package p

type StringBytesWriter interface {
    io.Writer
    WriteString(string) (n int, err error)
    io.ByteWriter
}

func DoSomething(w StringBytesWriter, foo *Foo) { ... }

But note that not even that is necessary. You could do:

func DoSomething(w io.Writer, foo *Foo) {
    if fastWriter, ok := w.(StringBytesWriter); ok {
        // faster implementation
    }
    // slower implementation
}

This is what many parts of the standard library do. This way, io.Writers still work, but passing in types that implement the more advanced interfaces still get the better performance. For example, see io.Copy's godoc:

If src implements the WriterTo interface, the copy is implemented by calling src.WriteTo(dst). Otherwise, if dst implements the ReaderFrom interface, the copy is implemented by calling dst.ReadFrom(src).

@dchenk
Copy link
Contributor Author

dchenk commented May 20, 2018

@meirf
https://godoc.org/github.com/philhofer/fwd#Writer
https://godoc.org/github.com/free/concurrent-writer/concurrent
The point is not just that there are already a lot of implementation of this interface; these types in the stdlib (bytes.Buffer, strings.Builder, bufio.Writer) are used like this in a lot of places everywhere:

someWriter.Write([]byte("some string"))

and

someWriter.Write([]byte("\n"))  // also: someWriter.Write([]byte{'\n'})

@jimmyfrasche
Copy link
Member

If you use io.WriteString it uses the WriteString method if it's there and if not it does the conversion to []byte:

io.WriteString(someWriter, "some string")

and

io.WriteString(someWriter, "\n")

@mvdan mvdan changed the title proposal: add type io.StringBytesWriter interface proposal: io: add interface type StringBytesWriter May 20, 2018
@bcmills
Copy link
Contributor

bcmills commented May 21, 2018

To be a viable proposal, this issue needs more detail.

Please include concrete examples (ideally drawn from experience reports) that illustrate the motivating problem, then show how those examples would be improved by it. (For example, if the motivating problem is a performance issue, quantify the impact on a real program: how much time is being spent today in type-assertions that this proposal would eliminate?)

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 21, 2018
@dchenk dchenk closed this as completed May 21, 2018
@golang golang locked and limited conversation to collaborators May 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

6 participants