bufio: set default buffer size from *bytes.Reader

[josharian]

I just came across some code that does this rather natural thing:

r := bufio.NewReader(bytes.NewReader(buf))
//  use r to do bufio-ish things

However, len(buf) is typically low, much smaller than the default buffer size of 4096. I think that bufio.NewReader should check whether r is a *bytes.Reader, and if so, use r.Len() to size the buffer (assuming r.Len() is less than 4096).

Implementation is easy, and would be a good first contribution for someone.

Any objections?

cc @griesemer @bradfitz @ianlancetaylor per golang.org/s/owners

[bradfitz]

Sounds reasonable. The dependency is already there anyway.

We could in theory do even better though and use the same underlying array so bufio doesn't need to allocate its own of any size.

[josharian]

In theory yes but I worry about Hyrum’s Law and people modifying the buffer underfoot. Maybe we should start there early in cycle and fall back to the less aggressive optimization if needed.

[qingyunha]

Should strings.Reader add here ?

[josharian]

@qingyunha i don’t think there’s a bufio->strings dependency yet, and I’d be reluctant to add one.

[bcmills]

If the optimization is based on r.Len(), would it make sense to check for a Len() int method in general rather than *bytes.Reader in particular? (Or are we worried about Len() int methods with other semantics?)

[bradfitz]

Or are we worried about Len() int methods with other semantics?

We have been in the past. This same issue came up earlier for http.NewRequest's type switch:

go/src/net/http/request.go

Line 874 in 1519bc4

case *bytes.Buffer:

and we decided just looking for a method wasn't enough.

[bradfitz]

(But if we wanted, we could use a combination of reflect + interface assertion to get the strings.Reader length without importing strings, and still verify it's only pkg strings and not something else with Len)

[bradfitz]

The comment from Bradford Lamson-Scribner has been deleted since I started replying, but FWIW, you can obtain the bytes.Reader's underlying buffer like: https://play.golang.org/p/t0o_t_lO3ck

That kinda violates the WriterTo contract at least in spirit, but limited shenanigans are generally permitted within the standard library when sufficiently documented & tested. Better than unsafe at least.

[bradford-hamilton]

@bradfitz thank you! I saw @josharian post this issue, and was interested in putting together a CL. Not long after posting, I realized it could be a great first contribution for someone (maybe depending on which implementation is preferred) and sort of took my hat out of the ring. I will keep 👀 on this though, if no one else gains interest - happy to work on it.