code review 41170044: runtime: zero 2-word memory blocks in-place

runtime: zero 2-word memory blocks in-place
Currently for 2-word blocks we set the flag to clear the flag. Makes no sense.
In particular on 32-bits we call memclr always.

[dvyukov, 2013-12-31 09:49:20]

Hello golang-codereviews@googlegroups.com,

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

[dave_cheney.net, 2013-12-31 10:34:49]

Thanks Dmitry. Do you have any benchmark data on 1 word vs 2 words ? Does
this value change depending on the platform, ie should this be an enum in
runtime/arch_{arm,amd64,386}.h ?


On Tue, Dec 31, 2013 at 8:49 PM, <dvyukov@google.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
>
>
> Description:
> runtime: zero 2-word memory blocks in-place
> Currently for 2-word blocks we set the flag to clear the flag. Makes no
> sense.
> In particular on 32-bits we call memclr always.
>
> Please review this at https://codereview.appspot.com/41170044/
>
> Affected files (+8, -4 lines):
>   M src/pkg/runtime/malloc.goc
>   M src/pkg/runtime/mgc0.c
>
>
> Index: src/pkg/runtime/malloc.goc
> ===================================================================
> --- a/src/pkg/runtime/malloc.goc
> +++ b/src/pkg/runtime/malloc.goc
> @@ -75,7 +75,7 @@
>                 if(!(flag & FlagNoZero)) {
>                         v->next = nil;
>                         // block is zeroed iff second word is zero ...
> -                       if(size > sizeof(uintptr) && ((uintptr*)v)[1] != 0)
> +                       if(size > 2*sizeof(uintptr) && ((uintptr*)v)[1] !=
> 0)
>                                 runtime·memclr((byte*)v, size);
>                 }
>                 c->local_cachealloc += size;
> @@ -207,8 +207,10 @@
>         } else {
>                 // Small object.
>                 size = runtime·class_to_size[sizeclass];
> -               if(size > sizeof(uintptr))
> +               if(size > 2*sizeof(uintptr))
>                         ((uintptr*)v)[1] = (uintptr)0xfeedfeedfeedfeedll;
>       // mark as "needs to be zeroed"
> +               else if(size > sizeof(uintptr))
> +                       ((uintptr*)v)[1] = 0;   // zeroize the second word
>                 // Must mark v freed before calling MCache_Free:
>                 // it might coalesce v and other blocks into a bigger span
>                 // and change the bitmap further.
> Index: src/pkg/runtime/mgc0.c
> ===================================================================
> --- a/src/pkg/runtime/mgc0.c
> +++ b/src/pkg/runtime/mgc0.c
> @@ -1851,9 +1851,11 @@
>                                 *(byte*)type_data = 0;
>                                 break;
>                         }
> -                       if(size > sizeof(uintptr))
> +                       if(size > 2*sizeof(uintptr))
>                                 ((uintptr*)p)[1] =
> (uintptr)0xdeaddeaddeaddeadll;       // mark as "needs to be zeroed"
> -
> +                       else if(size > sizeof(uintptr))
> +                               ((uintptr*)p)[1] = 0;   // zeroize the
> second word
> +
>                         end->next = (MLink*)p;
>                         end = (MLink*)p;
>                         nfree++;
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[dvyukov, 2013-12-31 10:54:47]

On Tue, Dec 31, 2013 at 2:34 PM, Dave Cheney <dave@cheney.net> wrote:
> Thanks Dmitry. Do you have any benchmark data on 1 word vs 2 words ?

Other parts of the system are that slow, that this change does not
provide visible change. Yet.


> Does
> this value change depending on the platform, ie should this be an enum in
> runtime/arch_{arm,amd64,386}.h ?

This change does not need such tuning.
If we decide to inline memclr for small blocks, then that change will
need tuning.



> On Tue, Dec 31, 2013 at 8:49 PM, <dvyukov@google.com> wrote:
>>
>> Reviewers: golang-codereviews,
>>
>> Message:
>> Hello golang-codereviews@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://dvyukov%40google.com@code.google.com/p/go/
>>
>>
>> Description:
>> runtime: zero 2-word memory blocks in-place
>> Currently for 2-word blocks we set the flag to clear the flag. Makes no
>> sense.
>> In particular on 32-bits we call memclr always.
>>
>> Please review this at https://codereview.appspot.com/41170044/
>>
>> Affected files (+8, -4 lines):
>>   M src/pkg/runtime/malloc.goc
>>   M src/pkg/runtime/mgc0.c
>>
>>
>> Index: src/pkg/runtime/malloc.goc
>> ===================================================================
>> --- a/src/pkg/runtime/malloc.goc
>> +++ b/src/pkg/runtime/malloc.goc
>> @@ -75,7 +75,7 @@
>>                 if(!(flag & FlagNoZero)) {
>>                         v->next = nil;
>>                         // block is zeroed iff second word is zero ...
>> -                       if(size > sizeof(uintptr) && ((uintptr*)v)[1] !=
>> 0)
>> +                       if(size > 2*sizeof(uintptr) && ((uintptr*)v)[1] !=
>> 0)
>>                                 runtime·memclr((byte*)v, size);
>>                 }
>>                 c->local_cachealloc += size;
>> @@ -207,8 +207,10 @@
>>         } else {
>>                 // Small object.
>>                 size = runtime·class_to_size[sizeclass];
>> -               if(size > sizeof(uintptr))
>> +               if(size > 2*sizeof(uintptr))
>>                         ((uintptr*)v)[1] = (uintptr)0xfeedfeedfeedfeedll;
>> // mark as "needs to be zeroed"
>> +               else if(size > sizeof(uintptr))
>> +                       ((uintptr*)v)[1] = 0;   // zeroize the second word
>>                 // Must mark v freed before calling MCache_Free:
>>                 // it might coalesce v and other blocks into a bigger span
>>                 // and change the bitmap further.
>> Index: src/pkg/runtime/mgc0.c
>> ===================================================================
>> --- a/src/pkg/runtime/mgc0.c
>> +++ b/src/pkg/runtime/mgc0.c
>> @@ -1851,9 +1851,11 @@
>>                                 *(byte*)type_data = 0;
>>                                 break;
>>                         }
>> -                       if(size > sizeof(uintptr))
>> +                       if(size > 2*sizeof(uintptr))
>>                                 ((uintptr*)p)[1] =
>> (uintptr)0xdeaddeaddeaddeadll;       // mark as "needs to be zeroed"
>> -
>> +                       else if(size > sizeof(uintptr))
>> +                               ((uintptr*)p)[1] = 0;   // zeroize the
>> second word
>> +
>>                         end->next = (MLink*)p;
>>                         end = (MLink*)p;
>>                         nfree++;
>>
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[dvyukov, 2014-01-18 15:42:44]

ping

[iant, 2014-01-19 05:37:49]

LGTM

https://codereview.appspot.com/41170044/diff/60001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/41170044/diff/60001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:213: ((uintptr*)v)[1] = 0;	// zeroize the second word
"zeroize" is not an English word.

I would actually drop the comment entirely, or rewrite it to be longer.  By
itself this comment simply restates what the code is doing.  It's only
meaningful if you want to write a large comment explaining why this is
necessary.

https://codereview.appspot.com/41170044/diff/60001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/41170044/diff/60001/src/pkg/runtime/mgc0.c#new...
src/pkg/runtime/mgc0.c:1857: ((uintptr*)p)[1] = 0;	// zeroize the second word
Same comment about the comment.

[dvyukov, 2014-01-21 06:51:40]

https://codereview.appspot.com/41170044/diff/60001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/41170044/diff/60001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:213: ((uintptr*)v)[1] = 0;	// zeroize the second word
On 2014/01/19 05:37:50, iant wrote:
> "zeroize" is not an English word.
> 
> I would actually drop the comment entirely, or rewrite it to be longer.  By
> itself this comment simply restates what the code is doing.  It's only
> meaningful if you want to write a large comment explaining why this is
> necessary.

Done.

https://codereview.appspot.com/41170044/diff/60001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/41170044/diff/60001/src/pkg/runtime/mgc0.c#new...
src/pkg/runtime/mgc0.c:1857: ((uintptr*)p)[1] = 0;	// zeroize the second word
On 2014/01/19 05:37:50, iant wrote:
> Same comment about the comment.

Done.

[dvyukov, 2014-01-21 06:55:21]

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

runtime: zero 2-word memory blocks in-place
Currently for 2-word blocks we set the flag to clear the flag. Makes no sense.
In particular on 32-bits we call memclr always.

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