code review 8819049: bufio: make Reader buffer transient

bufio: make Reader buffer transient

Share garbage between different bufio Readers. When a Reader
has zero buffered data, put its buffer into a pool.

This acknowledges that most bufio.Readers eventually get
read to completion, and their buffers are then no longer
needed.

benchmark               old ns/op    new ns/op    delta
BenchmarkReaderEmpty         2993         1058  -64.65%

benchmark              old allocs   new allocs    delta
BenchmarkReaderEmpty            3            2  -33.33%

benchmark               old bytes    new bytes    delta
BenchmarkReaderEmpty         4278          133  -96.89%

Update Issue 5100

[bradfitz, 2013-05-12 23:26:53]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[adg, 2013-05-12 23:55:23]

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:193: if b.r == b.w {
I think you can drop this comparison. You make it inside putBuf anyway (maybe
make it the first comparison in putBuf?)

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:222: if b.buf == nil {
ditto this one

[adg, 2013-05-12 23:57:53]

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:79: for i := range b.buf {
why zero this?

[dvyukov, 2013-05-13 04:00:32]

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:79: for i := range b.buf {
On 2013/05/12 23:57:53, adg wrote:
> why zero this?

the same question I wanted to ask

[bradfitz, 2013-05-13 04:07:59]

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:79: for i := range b.buf {
On 2013/05/13 04:00:32, dvyukov wrote:
> On 2013/05/12 23:57:53, adg wrote:
> > why zero this?
> 
> the same question I wanted to ask

Just so the behavior is observably identical as before.

Without this, it's possible for a user to use ReadSlice and extend the slice
past its length and see a different user's old buffer.  The contract permits
that, but I wanted to show that you can act the same as behavior and still be
faster.

Once https://code.google.com/p/go/issues/detail?id=5373 is fixed, this will
matter much less. I'd prefer to keep it.

[rog, 2013-05-14 16:00:33]

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:78: case b.buf = <-bufCache:
is this right when b.bufSize != defaultBufSize?
ISTM that we can return a buffer that's not of the size requested by
NewReaderSize in that case.

[bradfitz, 2013-05-14 16:19:27]

Hello golang-dev@googlegroups.com, adg@golang.org, dvyukov@google.com,
rogpeppe@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-05-14 16:19:35]

Good catch!

PTAL.



On Tue, May 14, 2013 at 9:00 AM, <rogpeppe@gmail.com> wrote:

>
> https://codereview.appspot.**com/8819049/diff/8001/src/pkg/**
>
bufio/bufio.go<https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go>
> File src/pkg/bufio/bufio.go (right):
>
> https://codereview.appspot.**com/8819049/diff/8001/src/pkg/**
>
bufio/bufio.go#newcode78<https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newcode78>
> src/pkg/bufio/bufio.go:78: case b.buf = <-bufCache:
> is this right when b.bufSize != defaultBufSize?
> ISTM that we can return a buffer that's not of the size requested by
> NewReaderSize in that case.
>
>
https://codereview.appspot.**com/8819049/<https://codereview.appspot.com/8819...
>

[gobot, 2013-05-14 22:14:36]

R=r (assigned by bradfitz)

[r, 2013-05-15 19:48:35]

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:61: if size != defaultBufSize {
why not
if size > defaultBufSize

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:76: // bufCache only holds byte slices of defaultBufSize
// bufCache holds byte slices only of length defaultBufSize

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:77: var bufCache = make(chan []byte, 8)
pull 8 out as a named (private) constant? makes it nicer and safer to change
later if we want.

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:81: func (b *Reader) allocBuf() {
should be getBuf to complement putBuf

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:111: if b.buf == nil {
looks like it should be above the if b.r > 0 case, although i doubt it actually
matters.

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:195: if b.r == b.w {
my gut tells me this is too aggressive. there are synchronization operations
involved in putbuf and allocbuf so it feels expensive, but it does reduce the
memory maybe faster. but i haven't thought it out. what's your reasoning?

also, every call is guarded by b.r==b.w. i suggest putting that check into
putBuf, which makes the code nicer but also means we can put the policy in
exactly one place.

[bradfitz, 2013-05-17 21:31:31]

Done. PTAL.

The buffer is only recycled on EOF now.

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:61: if size != defaultBufSize {
On 2013/05/15 19:48:35, r wrote:
> why not
> if size > defaultBufSize

Done.

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:76: // bufCache only holds byte slices of defaultBufSize
On 2013/05/15 19:48:35, r wrote:
> // bufCache holds byte slices only of length defaultBufSize

Done.

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:77: var bufCache = make(chan []byte, 8)
On 2013/05/15 19:48:35, r wrote:
> pull 8 out as a named (private) constant? makes it nicer and safer to change
> later if we want.

Done.  Better: added a TODO to make this a sync.Cache.

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:81: func (b *Reader) allocBuf() {
On 2013/05/15 19:48:35, r wrote:
> should be getBuf to complement putBuf

It doesn't return anything, though, so getBuf sounds wrong.  It makes the buf
exist, but it doesn't get it.

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:111: if b.buf == nil {
On 2013/05/15 19:48:35, r wrote:
> looks like it should be above the if b.r > 0 case, although i doubt it
actually
> matters.

If b.r > 0, there has to be a buf, but done.

https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:195: if b.r == b.w {
On 2013/05/15 19:48:35, r wrote:
> my gut tells me this is too aggressive. there are synchronization operations
> involved in putbuf and allocbuf so it feels expensive, but it does reduce the
> memory maybe faster. but i haven't thought it out. what's your reasoning?
> 
> also, every call is guarded by b.r==b.w. i suggest putting that check into
> putBuf, which makes the code nicer but also means we can put the policy in
> exactly one place.

I changed it to only return the buffer on io.EOF, which is the common case.

[r, 2013-05-17 21:50:30]

LGTM

[bradfitz, 2013-05-17 22:16:14]

*** Submitted as https://code.google.com/p/go/source/detail?r=bce231eb0fdd ***

bufio: make Reader buffer transient

Share garbage between different bufio Readers. When a Reader
has zero buffered data, put its buffer into a pool.

This acknowledges that most bufio.Readers eventually get
read to completion, and their buffers are then no longer
needed.

benchmark               old ns/op    new ns/op    delta
BenchmarkReaderEmpty         2993         1058  -64.65%

benchmark              old allocs   new allocs    delta
BenchmarkReaderEmpty            3            2  -33.33%

benchmark               old bytes    new bytes    delta
BenchmarkReaderEmpty         4278          133  -96.89%

Update Issue 5100

R=r
CC=adg, dvyukov, gobot, golang-dev, rogpeppe
https://codereview.appspot.com/8819049