code review 57680046: runtime: introduce MSpan.needzero instead of writing to...

runtime: introduce MSpan.needzero instead of writing to span data

This cleans up the code significantly, and it avoids any
possible problems with madvise zeroing out some but
not all of the data.

Fixes issue 6400.

[rsc, 2014-02-12 18:32:04]

Hello dvyukov (cc: golang-codereviews@googlegroups.com),

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

[dave_cheney.net, 2014-02-12 18:38:21]

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

https://codereview.appspot.com/57680046/diff/60001/src/pkg/runtime/mheap.c#ne...
src/pkg/runtime/mheap.c:106: if(zeroed && s->needzero)
The current code reads as 'if zeroed, and needs to be zeroed, then zeroy it'.
Could you rename zeroed to be needzero ?

[rsc, 2014-02-12 20:05:00]

PTAL

[dave_cheney.net, 2014-02-12 20:09:14]

LGTM. Much clearer.


On Thu, Feb 13, 2014 at 7:05 AM, <rsc@golang.org> wrote:

> PTAL
>
>
> https://codereview.appspot.com/57680046/
>

[dvyukov, 2014-02-12 20:17:08]

I will take a look tomorrow

[rsc, 2014-02-12 22:09:15]

On Wed, Feb 12, 2014 at 4:57 PM, <iant@golang.org> wrote:

> LGTM
>
>
> https://codereview.appspot.com/57680046/diff/90001/src/
> pkg/runtime/malloc.h
> File src/pkg/runtime/malloc.h (right):
>
> https://codereview.appspot.com/57680046/diff/90001/src/
> pkg/runtime/malloc.h#newcode69
> src/pkg/runtime/malloc.h:69: // the second word of the object is zero.
> The spans in the
> Please update comment.
>
> https://codereview.appspot.com/57680046/diff/90001/src/pkg/runtime/mgc0.c
> File src/pkg/runtime/mgc0.c (right):
>
> https://codereview.appspot.com/57680046/diff/90001/src/
> pkg/runtime/mgc0.c#newcode1301
> src/pkg/runtime/mgc0.c:1301: runtime·printf("sweep %d %d\n",
> s->sweepgen, sg);
> Unrelated change?
>

It now appears that way; will remove, thanks. Waiting for Dmitriy to fix
sweeping though.

[dvyukov, 2014-02-13 07:09:35]

On 2014/02/12 22:09:15, rsc wrote:
> On Wed, Feb 12, 2014 at 4:57 PM, <mailto:iant@golang.org> wrote:
> 
> > LGTM
> >
> >
> > https://codereview.appspot.com/57680046/diff/90001/src/
> > pkg/runtime/malloc.h
> > File src/pkg/runtime/malloc.h (right):
> >
> > https://codereview.appspot.com/57680046/diff/90001/src/
> > pkg/runtime/malloc.h#newcode69
> > src/pkg/runtime/malloc.h:69: // the second word of the object is zero.
> > The spans in the
> > Please update comment.
> >
> > https://codereview.appspot.com/57680046/diff/90001/src/pkg/runtime/mgc0.c
> > File src/pkg/runtime/mgc0.c (right):
> >
> > https://codereview.appspot.com/57680046/diff/90001/src/
> > pkg/runtime/mgc0.c#newcode1301
> > src/pkg/runtime/mgc0.c:1301: runtime·printf("sweep %d %d\n",
> > s->sweepgen, sg);
> > Unrelated change?
> >
> 
> It now appears that way; will remove, thanks. Waiting for Dmitriy to fix
> sweeping though.

I've sent sweeping fix.

runtime·free also requires s->needzero=1 before MHeap_Free call.
I would put s->needzero=1 in the beginning of MHeap_Free -- one less possibility
for bugs.

[rsc, 2014-02-13 16:10:35]

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

runtime: introduce MSpan.needzero instead of writing to span data

This cleans up the code significantly, and it avoids any
possible problems with madvise zeroing out some but
not all of the data.

Fixes issue 6400.

LGTM=dave
R=dvyukov, dave
CC=golang-codereviews
https://codereview.appspot.com/57680046

[iant, 2014-02-13 17:00:03]

On Wed, Feb 12, 2014 at 1:57 PM,  <iant@golang.org> wrote:
>
>
> https://codereview.appspot.com/57680046/diff/90001/src/pkg/runtime/malloc.h
> File src/pkg/runtime/malloc.h (right):
>
>
https://codereview.appspot.com/57680046/diff/90001/src/pkg/runtime/malloc.h#n...
> src/pkg/runtime/malloc.h:69: // the second word of the object is zero.
> The spans in the
> Please update comment.

I think you missed this suggestion.

Ian

[gobot, 2014-02-13 17:07:40]

This CL appears to have broken the openbsd-amd64-rootbsd builder.

[rsc, 2014-02-13 18:32:15]

https://codereview.appspot.com/57680046/diff/90001/src/pkg/runtime/malloc.h
File src/pkg/runtime/malloc.h (right):

https://codereview.appspot.com/57680046/diff/90001/src/pkg/runtime/malloc.h#n...
src/pkg/runtime/malloc.h:69: // the second word of the object is zero.  The
spans in the
On 2014/02/12 21:57:36, iant wrote:
> Please update comment.

New CL sent, sorry.