code review 8164043: bytes: don't grow Buffer if capacity is available

bytes: don't grow Buffer if capacity is available

Also added a new benchmark from the same test:

benchmark                           old ns/op    new ns/op    delta
BenchmarkBufferNotEmptyWriteRead      2643698       709189  -73.17%

Fixes Issue 5154

[bradfitz, 2013-03-29 18:22:42]

Hello golang-dev@googlegroups.com,

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

[r, 2013-03-29 18:27:49]

LGTM

https://codereview.appspot.com/8164043/diff/5001/src/pkg/bytes/export_test.go
File src/pkg/bytes/export_test.go (right):

https://codereview.appspot.com/8164043/diff/5001/src/pkg/bytes/export_test.go...
src/pkg/bytes/export_test.go:13: }
func (b *Buffer) Cap() int {
   return cap(b.buf)
}

[r, 2013-03-29 18:28:34]

let gri have a look too before you submit. there may be a prettier way
but i couldn't think of one

[bradfitz, 2013-03-29 18:30:35]

Hello golang-dev@googlegroups.com, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-03-29 18:30:37]

https://codereview.appspot.com/8164043/diff/5001/src/pkg/bytes/export_test.go
File src/pkg/bytes/export_test.go (right):

https://codereview.appspot.com/8164043/diff/5001/src/pkg/bytes/export_test.go...
src/pkg/bytes/export_test.go:13: }
On 2013/03/29 18:27:49, r wrote:
> func (b *Buffer) Cap() int {
>    return cap(b.buf)
> }

Done.

[gri, 2013-03-29 19:26:02]

LGTM

nice!

[bradfitz, 2013-03-29 19:39:25]

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

bytes: don't grow Buffer if capacity is available

Also added a new benchmark from the same test:

benchmark                           old ns/op    new ns/op    delta
BenchmarkBufferNotEmptyWriteRead      2643698       709189  -73.17%

Fixes Issue 5154

R=golang-dev, r, gri
CC=golang-dev
https://codereview.appspot.com/8164043

[khr1, 2013-03-29 19:47:49]

I would think you'd want to make sure that you weren't copying too much.
 If you're asking for one byte and there's one byte available at the start
of the buffer, you probably don't want to copy down N bytes only to free up
that one byte at the end.  Better to grow the buffer now so that next time
there is more space.

So copy if the buffer is at least half empty?  Something like "m+n <=
cap(b.buf) && m <= cap(b.buf)/2"


On Fri, Mar 29, 2013 at 12:39 PM, <bradfitz@golang.org> wrote:

> *** Submitted as
>
https://code.google.com/p/go/**source/detail?r=7b47ca7337ca<https://code.goog...
>
>
> bytes: don't grow Buffer if capacity is available
>
> Also added a new benchmark from the same test:
>
> benchmark                           old ns/op    new ns/op    delta
> BenchmarkBufferNotEmptyWriteRe**ad      2643698       709189  -73.17%
>
> Fixes Issue 5154
>
> R=golang-dev, r, gri
> CC=golang-dev
>
https://codereview.appspot.**com/8164043<https://codereview.appspot.com/8164043>
>
>
>
>
https://codereview.appspot.**com/8164043/<https://codereview.appspot.com/8164...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[bradfitz, 2013-03-29 19:51:53]

Yeah, that was my hesitation before too, and I just realized after
submitting that the original bug report said that too.

Will send a CL.

On Fri, Mar 29, 2013 at 12:47 PM, Keith Randall <khr@google.com> wrote:

> I would think you'd want to make sure that you weren't copying too much.
>  If you're asking for one byte and there's one byte available at the start
> of the buffer, you probably don't want to copy down N bytes only to free up
> that one byte at the end.  Better to grow the buffer now so that next time
> there is more space.
>
> So copy if the buffer is at least half empty?  Something like "m+n <=
> cap(b.buf) && m <= cap(b.buf)/2"
>
>
> On Fri, Mar 29, 2013 at 12:39 PM, <bradfitz@golang.org> wrote:
>
>> *** Submitted as
>>
https://code.google.com/p/go/**source/detail?r=7b47ca7337ca<https://code.goog...
>>
>>
>> bytes: don't grow Buffer if capacity is available
>>
>> Also added a new benchmark from the same test:
>>
>> benchmark                           old ns/op    new ns/op    delta
>> BenchmarkBufferNotEmptyWriteRe**ad      2643698       709189  -73.17%
>>
>> Fixes Issue 5154
>>
>> R=golang-dev, r, gri
>> CC=golang-dev
>>
https://codereview.appspot.**com/8164043<https://codereview.appspot.com/8164043>
>>
>>
>>
>>
https://codereview.appspot.**com/8164043/<https://codereview.appspot.com/8164...
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
>> .
>> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
>> .
>>
>>
>>
>

[robryk, 2013-03-29 20:03:23]

> On Fri, Mar 29, 2013 at 12:47 PM, Keith Randall <mailto:khr@google.com> wrote:
> 
> > I would think you'd want to make sure that you weren't copying too much.
> >  If you're asking for one byte and there's one byte available at the start
> > of the buffer, you probably don't want to copy down N bytes only to free up
> > that one byte at the end.  Better to grow the buffer now so that next time
> > there is more space.
> >
> > So copy if the buffer is at least half empty?  Something like "m+n <=
> > cap(b.buf) && m <= cap(b.buf)/2"

m+n <= cap(b.buf)/2 should be ok: we will then rewrite every byte inserted into
the buffer at most once (we will only copy bytes from its second half into its
first half; and it never shrinks) and we will grow the buffer to at most twice
the size required.

[bradfitz, 2013-03-29 20:04:59]

On Fri, Mar 29, 2013 at 1:03 PM, <robryk@gmail.com> wrote:

> On Fri, Mar 29, 2013 at 12:47 PM, Keith Randall
>>
> <mailto:khr@google.com> wrote:
>
>  > I would think you'd want to make sure that you weren't copying too
>>
> much.
>
>> >  If you're asking for one byte and there's one byte available at the
>>
> start
>
>> > of the buffer, you probably don't want to copy down N bytes only to
>>
> free up
>
>> > that one byte at the end.  Better to grow the buffer now so that
>>
> next time
>
>> > there is more space.
>> >
>> > So copy if the buffer is at least half empty?  Something like "m+n
>>
> <=
>
>> > cap(b.buf) && m <= cap(b.buf)/2"
>>
>
> m+n <= cap(b.buf)/2 should be ok: we will then rewrite every byte
> inserted into the buffer at most once (we will only copy bytes from its
> second half into its first half; and it never shrinks) and we will grow
> the buffer to at most twice the size required.
>

yeah, that's what I went with in https://codereview.appspot.com/8173043/