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: default ByteReader from Reader #37344

Closed
pierrec opened this issue Feb 20, 2020 · 12 comments
Closed

proposal: io: default ByteReader from Reader #37344

pierrec opened this issue Feb 20, 2020 · 12 comments

Comments

@pierrec
Copy link

pierrec commented Feb 20, 2020

Background & Motivation

When using some APIs such as ReadUvarint, sometimes the need arise to provide an io.ByteReader when only an io.Reader is available. Currently, there are 2 main ways this is addressed:

  • wrap the Reader with a new type which implements io.ByteReader
  • use bufio.Reader that already implements io.ByteReader

The downsides of the current solutions are:

  • multiple implementations of the same type
  • instantiating a large object for a simple use

A quick look at the standard library shows that the second option is usually the preferred method:
compress/bzip2
compress/flate
compress/lzw
encoding/xml
image/gif

Goals

Simplify usage of APIs requiring io.ByteReader's.
Reduce memory footprint when needing an io.ByteReader from an io.Reader.

Design

Provide a new type that implements the wrapper. Something like below.

// byteReader turns an io.byteReader into an io.ByteReader.
type byteReader struct {
	io.Reader
}

// New returns an io.ByteReader from an io.byteReader.
// If r is already a ByteReader, it is returned.
func New(r io.Reader) io.ByteReader {
	if r, ok := r.(io.ByteReader); ok {
		return r
	}
	return &byteReader{r}
}

func (r *byteReader) ReadByte() (byte, error) {
	var buf [1]byte
	n, err := r.Reader.Read(buf[:])
	if err != nil {
		return 0, err
	}
	if n == 0 {
		return 0, io.ErrNoProgress
	}
	return buf[0], nil
}

To be decided

The package where this type would live: io, ioutil other?

Backwards Compatibility

This proposal is backward compatible.

@gopherbot gopherbot added this to the Proposal milestone Feb 20, 2020
@OneOfOne
Copy link
Contributor

Reading one byte at a time without caching would be extremely slow for any real io, just something to consider.

@ianlancetaylor
Copy link
Contributor

As you show, you can already do this yourself. So the only question is whether this should be in the standard library. Is this really something that comes up often enough to add to the standard library? https://golang.org/doc/faq#x_in_std

@pierrec
Copy link
Author

pierrec commented Feb 21, 2020

@OneOfOne yes that is true, hence why it falls back to the underlying Reader implementation of ByteReader if it exists.

@ianlancetaylor indeed. And I filed this proposal after thinking about this and coming to the conclusion that this could go with the ioutil.NopCloser, which I do use from time to time as it is convenient albeit not too often. Such would be the case for the ByteReader.

Not sure how this comes up in practice outside of the standard library, I havent investigated this yet. I could if this helps.

@ianlancetaylor
Copy link
Contributor

NopCloser was added to the standard library because external packages kept repeating that code. It made sense to consolidate a single version.

It's not clear to me that external package keep repeating the creation of an io.ByteReader. For most uses a bufio.Reader works well and is more efficient. The case where people want the less efficient, albeit smaller, io.ByteReader seems uncommon to me. But I might change my mind if there are many existing examples where programs have chosen to implement a small io.ByteReader.

@rsc
Copy link
Contributor

rsc commented Feb 26, 2020

The reason that io.ByteReader exists at all is that some code wants to read one byte at a time, and that really can't be done well without an underlying larger buffer. That is, Read of 1-byte slices is so suboptimal to be worth defining this added interface. This is why Uvarint takes an io.ByteReader in the first place instead of an io.Reader.

Having a "default" implementation that converts io.Reader to io.ByteReader by making 1-byte reads is going to have all the slowness we were avoiding by not doing that in the implementations.

In general having a ReadByte method is an assertion "I have an efficient way to read 1 byte at a time". This would not be making that claim accurately.

We could document this better in the io.ByteReader docs. The right answer for your case is to add a buffer, likely using bytes.Buffer, as high up in the call stack as possible, so that the buffer is amortized across as many calls as possible.

@pierrec
Copy link
Author

pierrec commented Feb 27, 2020

@rsc thank you it makes sense.
Adding a note to the io.ByteReader interface would probably clarify its usage, something along the lines of:

// ByteReader is the interface that wraps the ReadByte method.
//
// ReadByte reads and returns the next byte from the input or
// any error encountered. If ReadByte returns an error, no input
// byte was consumed, and the returned byte value is undefined.
//
// When ReadByte is provided, it defines an efficient way of reading 
// bytes one at a time from an io.Reader, usually by using an underlying
// large buffer.
type ByteReader interface {
	ReadByte() (byte, error)
}

@gopherbot
Copy link

Change https://golang.org/cl/221380 mentions this issue: io: document that ReadByte is for efficiency

@ianlancetaylor
Copy link
Contributor

As @robpike says on CL 221380:

This feels out of place, a programming lesson in the middle of a list of simple I/O interfaces.

It's a tutorial about when this might be useful, not documentation about what it does.
This is information for an example maybe, or a blog post, but doesn't belong here.

@rsc rsc changed the title proposal: default implementation of io.ByteReader from io.Reader proposal: io: default implementation of ByteReader from Reader Mar 4, 2020
@rsc rsc changed the title proposal: io: default implementation of ByteReader from Reader proposal: io: default ByteReader from Reader Mar 4, 2020
@rsc
Copy link
Contributor

rsc commented Mar 11, 2020

@robpike said he would take a stab at a CL for wording an appropriate comment.

@rsc
Copy link
Contributor

rsc commented Mar 11, 2020

Based on the discussion above, however, the proposal to add a helper wrapper that does 1-byte reads seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Mar 11, 2020
@gopherbot
Copy link

Change https://golang.org/cl/223097 mentions this issue: io: add a comment about how to turn a Reader into BytReader

gopherbot pushed a commit that referenced this issue Mar 12, 2020
Offered as an alternative to CL 221380, which was more
tutorial than necessary.

Update #37344

Change-Id: Ide673b0b97983c2c2319a9311dc3d0a10567e6c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/223097
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@rsc
Copy link
Contributor

rsc commented Mar 25, 2020

No change in consensus, and Rob's CL landed, so declining.

@rsc rsc closed this as completed Mar 25, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Mar 25, 2020
@golang golang locked and limited conversation to collaborators Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants