code review 53340044: runtime: smarter slice grow

runtime: smarter slice grow
When growing slice take into account size of the allocated memory block.
Also apply the same optimization to string->[]byte conversion.
Fixes issue 6307.

benchmark                    old ns/op    new ns/op    delta
BenchmarkAppendGrowByte        4541036      4434108   -2.35%
BenchmarkAppendGrowString     59885673     44813604  -25.17%

[dvyukov, 2014-01-23 14:52:54]

Hello khr@golang.org (cc: golang-codereviews@googlegroups.com, iant@golang.org,
rsc@golang.org),

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[khr, 2014-01-24 21:59:09]

LGTM.

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/msize.c
File src/pkg/runtime/msize.c (right):

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/msize.c#ne...
src/pkg/runtime/msize.c:176: return ROUND(size, PageSize);
The ROUND may wrap around to 0.  You should check for that (unless you have an
argument for why it can't happen).

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/slice.c
File src/pkg/runtime/slice.c (right):

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/slice.c#ne...
src/pkg/runtime/slice.c:129: ret->len = x.len;
Add a comment here that no GC-triggering operations are allowed until the
memmove/memclr happen.

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/string.goc
File src/pkg/runtime/string.goc (right):

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/string.goc...
src/pkg/runtime/string.goc:292: runtime·memclr(b.array+b.len, cap-b.len);
Do we need this?
Maybe it is a security issue to disallow reading of some other old object.

[dvyukov, 2014-01-27 10:19:39]

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/msize.c
File src/pkg/runtime/msize.c (right):

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/msize.c#ne...
src/pkg/runtime/msize.c:176: return ROUND(size, PageSize);
On 2014/01/24 21:59:09, khr wrote:
> The ROUND may wrap around to 0.  You should check for that (unless you have an
> argument for why it can't happen).

added the check for overflow

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/slice.c
File src/pkg/runtime/slice.c (right):

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/slice.c#ne...
src/pkg/runtime/slice.c:129: ret->len = x.len;
On 2014/01/24 21:59:09, khr wrote:
> Add a comment here that no GC-triggering operations are allowed until the
> memmove/memclr happen.

I've added m->locks++/-- and a comment.

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/string.goc
File src/pkg/runtime/string.goc (right):

https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/string.goc...
src/pkg/runtime/string.goc:292: runtime·memclr(b.array+b.len, cap-b.len);
On 2014/01/24 21:59:09, khr wrote:
> Do we need this?
> Maybe it is a security issue to disallow reading of some other old object.

We are returning the slice with capacity larger than len. If you are reslice it
to increase capacity, you must read zeroes there. Just as for:
a := make([]byte, 0, 100)
a = a[0:100]
a must be all zeroes

[dvyukov, 2014-01-27 11:11:30]

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

runtime: smarter slice grow
When growing slice take into account size of the allocated memory block.
Also apply the same optimization to string->[]byte conversion.
Fixes issue 6307.

benchmark                    old ns/op    new ns/op    delta
BenchmarkAppendGrowByte        4541036      4434108   -2.35%
BenchmarkAppendGrowString     59885673     44813604  -25.17%

LGTM=khr
R=khr
CC=golang-codereviews, iant, rsc
https://codereview.appspot.com/53340044

[khr1, 2014-01-27 18:12:07]

On Mon, Jan 27, 2014 at 2:19 AM, <dvyukov@google.com> wrote:

>
> https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/msize.c
> File src/pkg/runtime/msize.c (right):
>
> https://codereview.appspot.com/53340044/diff/80001/src/
> pkg/runtime/msize.c#newcode176
> src/pkg/runtime/msize.c:176: return ROUND(size, PageSize);
> On 2014/01/24 21:59:09, khr wrote:
>
>> The ROUND may wrap around to 0.  You should check for that (unless you
>>
> have an
>
>> argument for why it can't happen).
>>
>
> added the check for overflow
>
>
> https://codereview.appspot.com/53340044/diff/80001/src/pkg/runtime/slice.c
> File src/pkg/runtime/slice.c (right):
>
> https://codereview.appspot.com/53340044/diff/80001/src/
> pkg/runtime/slice.c#newcode129
> src/pkg/runtime/slice.c:129: ret->len = x.len;
> On 2014/01/24 21:59:09, khr wrote:
>
>> Add a comment here that no GC-triggering operations are allowed until
>>
> the
>
>> memmove/memclr happen.
>>
>
> I've added m->locks++/-- and a comment.
>
>
> https://codereview.appspot.com/53340044/diff/80001/src/
> pkg/runtime/string.goc
> File src/pkg/runtime/string.goc (right):
>
> https://codereview.appspot.com/53340044/diff/80001/src/
> pkg/runtime/string.goc#newcode292
> src/pkg/runtime/string.goc:292: runtime·memclr(b.array+b.len,
> cap-b.len);
> On 2014/01/24 21:59:09, khr wrote:
>
>> Do we need this?
>> Maybe it is a security issue to disallow reading of some other old
>>
> object.
>
> We are returning the slice with capacity larger than len. If you are
> reslice it to increase capacity, you must read zeroes there. Just as
> for:
> a := make([]byte, 0, 100)
> a = a[0:100]
> a must be all zeroes
>
>
Ok, makes sense.


> https://codereview.appspot.com/53340044/
>