code review 10136043: runtime: refactor mallocgc

runtime: refactor mallocgc
Make it accept type, combine flags.
Several reasons for the change:
1. mallocgc and settype must be atomic wrt GC
2. settype is called from only one place now
3. it will help performance (eventually settype
functionality must be combined with markallocated)
4. flags are easier to read now (no mallocgc(sz, 0, 1, 0) anymore)

[dvyukov, 2013-06-09 17:08:58]

Hello golang-dev@googlegroups.com,

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

[iant, 2013-06-10 17:31:20]

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

https://codereview.appspot.com/10136043/diff/5001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:769: m->locks++;  // disable preemption to not expose
the memory block w/o type info to GC
This means that the call to mallocgc will not do a GC.  That doesn't seem right,
here or below.

[dvyukov, 2013-06-10 17:38:29]

On Mon, Jun 10, 2013 at 9:31 PM,  <iant@golang.org> wrote:
>
> https://codereview.appspot.com/10136043/diff/5001/src/pkg/runtime/malloc.goc
> File src/pkg/runtime/malloc.goc (right):
>
>
https://codereview.appspot.com/10136043/diff/5001/src/pkg/runtime/malloc.goc#...
> src/pkg/runtime/malloc.goc:769: m->locks++;  // disable preemption to
>
> not expose the memory block w/o type info to GC
> This means that the call to mallocgc will not do a GC.  That doesn't
> seem right, here or below.

This is a very good point.

[dvyukov, 2013-06-11 11:32:09]

On 2013/06/10 17:38:29, dvyukov wrote:
> On Mon, Jun 10, 2013 at 9:31 PM,  <mailto:iant@golang.org> wrote:
> >
> > https://codereview.appspot.com/10136043/diff/5001/src/pkg/runtime/malloc.goc
> > File src/pkg/runtime/malloc.goc (right):
> >
> >
>
https://codereview.appspot.com/10136043/diff/5001/src/pkg/runtime/malloc.goc#...
> > src/pkg/runtime/malloc.goc:769: m->locks++;  // disable preemption to
> >
> > not expose the memory block w/o type info to GC
> > This means that the call to mallocgc will not do a GC.  That doesn't
> > seem right, here or below.
> 
> This is a very good point.


I think mallocgc() deserves a more serious refactoring then:
1. We really must be setting the type in only 1 place.
2. Later markallocated() and settype() will be combined into a single operation,
so it has to be in mallocgc().
3. It's a bit difficult to decode mallocgc(sz, 0, 1, 0) when reading the code.

What do you think about the following interface?

void* mallocgc(uintptr size, Type *t, uint32 flag);

If Type is not known, it's nil.
Flags will be extended to:

enum
{
	// flags to malloc
	FlagNoPointers = 1<<0,	// no pointers here
	FlagNoProfiling = 1<<1,	// must not profile
	FlagNoGC = 1<<2,	// must not free or scan for pointers
	FlagNoZeroize = 1<<3,	// don't zeroize memory
	FlagNoGCInvoke = 1<<4   // don't invoke GC
};

[iant, 2013-06-11 15:36:28]

On Tue, Jun 11, 2013 at 4:32 AM,  <dvyukov@google.com> wrote:
>
> What do you think about the following interface?
>
> void* mallocgc(uintptr size, Type *t, uint32 flag);
>
> If Type is not known, it's nil.
> Flags will be extended to:
>
> enum
> {
>         // flags to malloc
>         FlagNoPointers = 1<<0,  // no pointers here
>         FlagNoProfiling = 1<<1, // must not profile
>         FlagNoGC = 1<<2,        // must not free or scan for pointers
>         FlagNoZeroize = 1<<3,   // don't zeroize memory
>         FlagNoGCInvoke = 1<<4   // don't invoke GC
> };

Seems fine to me.  I would call the last one FlagNoInvokeGC.

Ian

[dvyukov, 2013-06-12 11:39:46]

On Tue, Jun 11, 2013 at 7:36 PM, Ian Lance Taylor <iant@golang.org> wrote:
> On Tue, Jun 11, 2013 at 4:32 AM,  <dvyukov@google.com> wrote:
>>
>> What do you think about the following interface?
>>
>> void* mallocgc(uintptr size, Type *t, uint32 flag);
>>
>> If Type is not known, it's nil.
>> Flags will be extended to:
>>
>> enum
>> {
>>         // flags to malloc
>>         FlagNoPointers = 1<<0,  // no pointers here
>>         FlagNoProfiling = 1<<1, // must not profile
>>         FlagNoGC = 1<<2,        // must not free or scan for pointers
>>         FlagNoZeroize = 1<<3,   // don't zeroize memory
>>         FlagNoGCInvoke = 1<<4   // don't invoke GC
>> };
>
> Seems fine to me.

Russ, what do you think about mallocgc(uintptr size, Type *t, uint32 flag)?

> I would call the last one FlagNoInvokeGC.

Ack

[dvyukov, 2013-06-14 20:24:47]

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

Please take another look.

[dvyukov, 2013-06-14 20:25:10]

On 2013/06/11 15:36:28, iant wrote:
> On Tue, Jun 11, 2013 at 4:32 AM,  <mailto:dvyukov@google.com> wrote:
> >
> > What do you think about the following interface?
> >
> > void* mallocgc(uintptr size, Type *t, uint32 flag);
> >
> > If Type is not known, it's nil.
> > Flags will be extended to:
> >
> > enum
> > {
> >         // flags to malloc
> >         FlagNoPointers = 1<<0,  // no pointers here
> >         FlagNoProfiling = 1<<1, // must not profile
> >         FlagNoGC = 1<<2,        // must not free or scan for pointers
> >         FlagNoZeroize = 1<<3,   // don't zeroize memory
> >         FlagNoGCInvoke = 1<<4   // don't invoke GC
> > };
> 
> Seems fine to me.  I would call the last one FlagNoInvokeGC.


Done
PTAL

[ioe, 2013-06-14 22:57:26]

small nit.

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

https://codereview.appspot.com/10136043/diff/27001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:30: runtime·mallocgc(uintptr size, uintptr typ,
uint32 flag)
mallocgc() with argument "typ" set to "0" seems pretty common. It might be
useful to have a wrapper function for it. Esp. since this case has very special
semantics ("will be freed with runtime·free").

[dvyukov, 2013-06-15 11:58:41]

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

https://codereview.appspot.com/10136043/diff/27001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:30: runtime·mallocgc(uintptr size, uintptr typ,
uint32 flag)
On 2013/06/14 22:57:26, ioe wrote:
> mallocgc() with argument "typ" set to "0" seems pretty common. It might be
> useful to have a wrapper function for it. Esp. since this case has very
special
> semantics ("will be freed with runtime·free").

We already have two: runtime·mal() and runtime·malloc().
Note that typ==0 does not imply the very special semantics. The opposite is
true.

[rsc, 2013-07-02 00:38:02]

Merging the final two bools in mallogc into the flag seems fine to me.

[dave_cheney.net, 2013-07-02 00:59:30]

A while back I was working on a CL that reduced the number of
arguments in mallocgc and friends. At the time there were three
arguments, two bools and a flag. By combining them all into a single
flags argument I saw a small performance improvement.

+1 to this CL.

On Tue, Jul 2, 2013 at 10:38 AM, Russ Cox <rsc@golang.org> wrote:
> Merging the final two bools in mallogc into the flag seems fine to me.
>
> --
>
> ---
> 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.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[dvyukov, 2013-07-17 10:04:34]

ping

[khr, 2013-07-19 19:38:57]

On 2013/07/17 10:04:34, dvyukov wrote:
> ping

LGTM

[rsc, 2013-07-19 19:58:01]

NOT LGTM

[rsc, 2013-07-19 19:58:23]

There is enough broken in the runtime. Please wait on this CL until other
things have settled down.

[bradfitz, 2013-07-20 05:21:27]

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

https://codereview.appspot.com/10136043/diff/45001/src/pkg/runtime/malloc.h#n...
src/pkg/runtime/malloc.h:468: // flags to malloc
// flags to mallocgc

https://codereview.appspot.com/10136043/diff/45001/src/pkg/runtime/malloc.h#n...
src/pkg/runtime/malloc.h:472: FlagNoZeroize	= 1<<3, // don't zeroize memory
I didn't know "zeroize" was a real word until just now.  IMO, "FlagNoZero" and
"don't zero memory" sounds better.

[r, 2013-07-20 07:18:11]

If possible lease find a way the flag isn't negative true, or we end up
with if FlagNoZero is not false don't do this and I get all confused.

-rob



On Sat, Jul 20, 2013 at 3:21 PM, <bradfitz@golang.org> wrote:

>
> https://codereview.appspot.**com/10136043/diff/45001/src/**
>
pkg/runtime/malloc.h<https://codereview.appspot.com/10136043/diff/45001/src/pkg/runtime/malloc.h>
> File src/pkg/runtime/malloc.h (right):
>
> https://codereview.appspot.**com/10136043/diff/45001/src/**
>
pkg/runtime/malloc.h#**newcode468<https://codereview.appspot.com/10136043/diff/45001/src/pkg/runtime/malloc.h#newcode468>
> src/pkg/runtime/malloc.h:468: // flags to malloc
> // flags to mallocgc
>
> https://codereview.appspot.**com/10136043/diff/45001/src/**
>
pkg/runtime/malloc.h#**newcode472<https://codereview.appspot.com/10136043/diff/45001/src/pkg/runtime/malloc.h#newcode472>
> src/pkg/runtime/malloc.h:472: FlagNoZeroize     = 1<<3, // don't zeroize
> memory
> I didn't know "zeroize" was a real word until just now.  IMO,
> "FlagNoZero" and "don't zero memory" sounds better.
>
>
https://codereview.appspot.**com/10136043/<https://codereview.appspot.com/101...
>
> --
>
> ---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...
> .
>
>
>

[dvyukov, 2013-07-20 10:56:10]

On Sat, Jul 20, 2013 at 11:17 AM, Rob Pike <r@golang.org> wrote:
> If possible lease find a way the flag isn't negative true, or we end up with
> if FlagNoZero is not false don't do this and I get all confused.


I understand your concern.
But this is like "don't disturb" or "don't clean my room".
E.g. FlagNoGC is passed in very special circumstances where GC will
crash. FlagNoProfile is passed for very special blocks. FlagNoZero is
passed as an explicit optimization, and you want to see it, because by
default you assume that the memory is zero.
By default and in most cases it "does everything".

It could make sense to switch the meaning if we pass all flags as
separate arguments (it would be too verbose for common case). But
since we pass a bitmask generated with bitwise or, it's better to have
them inverted.

[dvyukov, 2013-07-20 10:57:16]

On Sat, Jul 20, 2013 at 9:21 AM,  <bradfitz@golang.org> wrote:
>
> https://codereview.appspot.com/10136043/diff/45001/src/pkg/runtime/malloc.h
> File src/pkg/runtime/malloc.h (right):
>
>
https://codereview.appspot.com/10136043/diff/45001/src/pkg/runtime/malloc.h#n...
> src/pkg/runtime/malloc.h:468: // flags to malloc
> // flags to mallocgc
>
>
https://codereview.appspot.com/10136043/diff/45001/src/pkg/runtime/malloc.h#n...
> src/pkg/runtime/malloc.h:472: FlagNoZeroize     = 1<<3, // don't zeroize
> memory
> I didn't know "zeroize" was a real word until just now.  IMO,
> "FlagNoZero" and "don't zero memory" sounds better.


Will do.

[r, 2013-07-20 21:25:46]

FlagNoZero is fine in this case then. I was just expressing a common
problem with naming.

-rob

[rsc, 2013-07-22 18:52:36]

LGTM

You buried the lede in the CL description. I would not have kept pushing this
off you had made it more obvious that this is a bug fix.

runtime: make mallocgc+settype atomic

Refactor mallocgc to accept type word and flags.
1. mallocgc and settype must be atomic wrt GC.
2. settype is called from only one place now.
3. Eventually settype can be combined with markallocated.
4. No more boolean arguments.

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

https://codereview.appspot.com/10136043/diff/45001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:28: // If the block will be freed with
runtime·free(), typ must be 0.
s/()//

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

https://codereview.appspot.com/10136043/diff/45001/src/pkg/runtime/malloc.h#n...
src/pkg/runtime/malloc.h:472: FlagNoZeroize	= 1<<3, // don't zeroize memory
On 2013/07/20 05:21:28, bradfitz wrote:
> I didn't know "zeroize" was a real word until just now.  IMO, "FlagNoZero" and
> "don't zero memory" sounds better.

FlagNoClear, please

[rsc, 2013-07-22 18:52:46]

LGTM after flag name change

[dvyukov, 2013-07-26 17:17:35]

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

runtime: refactor mallocgc
Make it accept type, combine flags.
Several reasons for the change:
1. mallocgc and settype must be atomic wrt GC
2. settype is called from only one place now
3. it will help performance (eventually settype
functionality must be combined with markallocated)
4. flags are easier to read now (no mallocgc(sz, 0, 1, 0) anymore)

R=golang-dev, iant, nightlyone, rsc, dave, khr, bradfitz, r
CC=golang-dev
https://codereview.appspot.com/10136043