code review 9754044: runtime/gc: Run garbage collector on g0 stack

runtime/gc: Run garbage collector on g0 stack
instead of regular g stack. We do this so that the g stack
we're currently running on is no longer changing.  Cuts
the root set down a bit (g0 stacks are not scanned, and
we don't need to scan gc's internal state).  Also an
enabler for copyable stacks.

[khr, 2013-05-29 22:49:01]

Hello golang-dev@googlegroups.com,

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

[cshapiro1, 2013-05-29 23:56:58]

looks pretty good, just a few questions and comments

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

https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:1902: int64 t0;
Can we document this member as being the start time of the GC?  (Comment, name
change, whatever you would like.)  The name 'force' has meaning in the context
of the garbage collection, but t0 is really local to the gc function.

https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:1977: if(g == m->g0) {
Given the lone goto, is there any reason not to make this a do-while loop?

https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:1994: // now that gc is done and we're back on g stack,
run some finalizers
Must this code be executed on the G stack?

https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:2010: static void
Move this up to avoid the additional forward declaration of mgc on line 1906?

[cshapiro1, 2013-05-30 00:07:21]

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

https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:1458: // Scanning our own stack: start at &gp.
Is this case now obsolete?  Can we get away with the code on lines 1465 to 1473?

[khr1, 2013-05-30 04:03:14]

On Wed, May 29, 2013 at 4:56 PM, <cshapiro@google.com> wrote:

> looks pretty good, just a few questions and comments
>
>
> https://codereview.appspot.**com/9754044/diff/8001/src/pkg/**
>
runtime/mgc0.c<https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c>
> File src/pkg/runtime/mgc0.c (right):
>
> https://codereview.appspot.**com/9754044/diff/8001/src/pkg/**
>
runtime/mgc0.c#newcode1902<https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newcode1902>
> src/pkg/runtime/mgc0.c:1902: int64 t0;
> Can we document this member as being the start time of the GC?
> (Comment, name change, whatever you would like.)  The name 'force' has
> meaning in the context of the garbage collection, but t0 is really local
> to the gc function.
>

Sure, changed it to start_time.


>
> https://codereview.appspot.**com/9754044/diff/8001/src/pkg/**
>
runtime/mgc0.c#newcode1977<https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newcode1977>
> src/pkg/runtime/mgc0.c:1977: if(g == m->g0) {
> Given the lone goto, is there any reason not to make this a do-while
> loop?
>

The more I look at this, the more I don't understand what was intended.
 Atom, can you explain what is supposed to happen when gctrace > 1?  In
tip, it looks like when not forced and gctrace > 1, it runs a second forced
gc pass.  Is is really required to have a second gc?  Or is it ok to just
run one gc forced with the conditional (force || gctrace > 1)?  In fact, if
gctrace > 1 and N threads call gc around the same time, it looks like we
will run N+1 gcs!

Also, this second gc isn't run on a fresh stack segment like the first gc.
 Probably not a big deal, performance isn't that important when tracing gc.


> https://codereview.appspot.**com/9754044/diff/8001/src/pkg/**
>
runtime/mgc0.c#newcode1994<https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newcode1994>
> src/pkg/runtime/mgc0.c:1994: // now that gc is done and we're back on g
> stack, run some finalizers
> Must this code be executed on the G stack?
>

Yes, we can't run general Go code on the g0 stack.  It isn't a growable
stack, it won't be scanned by the next gc, etc.  Only the runtime gets to
run on g0.


>
> https://codereview.appspot.**com/9754044/diff/8001/src/pkg/**
>
runtime/mgc0.c#newcode2010<https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newcode2010>
> src/pkg/runtime/mgc0.c:2010: static void
> Move this up to avoid the additional forward declaration of mgc on line
> 1906?
>

I'd rather leave it here, so mgc and gc are right next to each other.
 Reading the code flows better that way.


>
>
https://codereview.appspot.**com/9754044/<https://codereview.appspot.com/9754...
>

[khr1, 2013-05-30 04:13:06]

On Wed, May 29, 2013 at 5:07 PM, <cshapiro@google.com> wrote:

>
> https://codereview.appspot.**com/9754044/diff/8001/src/pkg/**
>
runtime/mgc0.c<https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c>
> File src/pkg/runtime/mgc0.c (right):
>
> https://codereview.appspot.**com/9754044/diff/8001/src/pkg/**
>
runtime/mgc0.c#newcode1458<https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newcode1458>
> src/pkg/runtime/mgc0.c:1458: // Scanning our own stack: start at &gp.
> Is this case now obsolete?  Can we get away with the code on lines 1465
> to 1473?
>

Yes, you're right.  The second condition ((mp = gp->m) != nil &&
mp->helpgc) is useless as well, we never scan gchelpers which also run on
g0 (addstackroots iterates over allg, which contains only g's, not g0's).
 I've left the conditions and put a throw in the body.


>
https://codereview.appspot.**com/9754044/<https://codereview.appspot.com/9754...
>

[atom, 2013-05-30 07:25:35]

On 2013/05/30 04:03:14, khr1 wrote:
> On Wed, May 29, 2013 at 4:56 PM, <mailto:cshapiro@google.com> wrote:
> 
> >
> > https://codereview.appspot.**com/9754044/diff/8001/src/pkg/**
> >
>
runtime/mgc0.c#newcode1977<https://codereview.appspot.com/9754044/diff/8001/src/pkg/runtime/mgc0.c#newcode1977>
> > src/pkg/runtime/mgc0.c:1977: if(g == m->g0) {
> > Given the lone goto, is there any reason not to make this a do-while
> > loop?
> >
> 
> The more I look at this, the more I don't understand what was intended.
>  Atom, can you explain what is supposed to happen when gctrace > 1?

I didn't write the original code containing force=1.

[dvyukov, 2013-05-30 14:49:40]

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

https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1458: runtime·throw("can't scan our own stack\n");
remove \n in throw message

https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1459: sp = pc = 0;
is it to silence compiler?

you can do just:

if(gp == g)
    runtime·throw("can't scan our own stack");
if((mp = gp->m) != nil && mp->helpgc)
    runtime·throw("can't scan gchelper stack");

and remove the trailing else
compiler must be happy
it's now a sanity check, not function logic

https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1958: if(!force && gctrace <= 1 && mstats.heap_alloc <
mstats.next_gc) {
remove "gctrace <= 1", it can cause thousands of unnecessary GC's
I do not mind if you do not restore that weird "if(gctrace > 1 && !force)" part,
I do not understand what it means either.

https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1980: g->param = (void*)&a;
s/(void*)//

https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:2008: gc((struct gc_args*)gp->param);
s/(struct gc_args*)//

https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:2009: gp->status = Grunning;
please set gp->param=nil here, so that it does not come up somewhere else

[khr1, 2013-05-30 15:57:55]

On Thu, May 30, 2013 at 7:49 AM, <dvyukov@google.com> wrote:

>
> https://codereview.appspot.**com/9754044/diff/15001/src/**
>
pkg/runtime/mgc0.c<https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c>
> File src/pkg/runtime/mgc0.c (right):
>
> https://codereview.appspot.**com/9754044/diff/15001/src/**
>
pkg/runtime/mgc0.c#newcode1458<https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newcode1458>
> src/pkg/runtime/mgc0.c:1458: runtime·throw("can't scan our own
> stack\n");
> remove \n in throw message
>
>
done.


> https://codereview.appspot.**com/9754044/diff/15001/src/**
>
pkg/runtime/mgc0.c#newcode1459<https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newcode1459>
> src/pkg/runtime/mgc0.c:1459: sp = pc = 0;
> is it to silence compiler?
>
>
yes.


> you can do just:
>
> if(gp == g)
>     runtime·throw("can't scan our own stack");
> if((mp = gp->m) != nil && mp->helpgc)
>     runtime·throw("can't scan gchelper stack");
>
> and remove the trailing else
> compiler must be happy
> it's now a sanity check, not function logic
>

ok, did that.


>
> https://codereview.appspot.**com/9754044/diff/15001/src/**
>
pkg/runtime/mgc0.c#newcode1958<https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newcode1958>
> src/pkg/runtime/mgc0.c:1958: if(!force && gctrace <= 1 &&
> mstats.heap_alloc < mstats.next_gc) {
> remove "gctrace <= 1", it can cause thousands of unnecessary GC's
> I do not mind if you do not restore that weird "if(gctrace > 1 &&
> !force)" part, I do not understand what it means either.
>
>
So the actual docs (src/pkg/runtime/extern.go) say:  "Setting GOGCTRACE=2
emits the same summary but also repeats each collection."

I'm not sure why you would want that (want to make sure nothing is
collected on the second pass?  want to know the time when nothing needs to
be collected?), but I'll implement it.


> https://codereview.appspot.**com/9754044/diff/15001/src/**
>
pkg/runtime/mgc0.c#newcode1980<https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newcode1980>
> src/pkg/runtime/mgc0.c:1980: g->param = (void*)&a;
> s/(void*)//
>
> https://codereview.appspot.**com/9754044/diff/15001/src/**
>
pkg/runtime/mgc0.c#newcode2008<https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newcode2008>
> src/pkg/runtime/mgc0.c:2008: gc((struct gc_args*)gp->param);
> s/(struct gc_args*)//
>
> https://codereview.appspot.**com/9754044/diff/15001/src/**
>
pkg/runtime/mgc0.c#newcode2009<https://codereview.appspot.com/9754044/diff/15001/src/pkg/runtime/mgc0.c#newcode2009>
> src/pkg/runtime/mgc0.c:2009: gp->status = Grunning;
> please set gp->param=nil here, so that it does not come up somewhere
> else
>
>
did all those.


>
https://codereview.appspot.**com/9754044/<https://codereview.appspot.com/9754...
>

[rsc, 2013-05-30 16:01:27]

On Thu, May 30, 2013 at 11:57 AM, Keith Randall <khr@google.com> wrote:

> So the actual docs (src/pkg/runtime/extern.go) say:  "Setting GOGCTRACE=2
> emits the same summary but also repeats each collection."
> I'm not sure why you would want that (want to make sure nothing is
> collected on the second pass?  want to know the time when nothing needs to
> be collected?), but I'll implement it.
>

Yes, both.

Russ

[dvyukov, 2013-05-31 04:02:41]

LGTM!

[cshapiro1, 2013-05-31 04:05:18]

Minor comment...

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

https://codereview.appspot.com/9754044/diff/16002/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1991: // now that gc is done and we're back on g stack,
run some finalizers
Can we use another worth than "some" here?  After all, we will run everything on
the finalizer queue.

[khr1, 2013-05-31 17:06:39]

On Thu, May 30, 2013 at 9:05 PM, <cshapiro@google.com> wrote:

> Minor comment...
>
>
> https://codereview.appspot.**com/9754044/diff/16002/src/**
>
pkg/runtime/mgc0.c<https://codereview.appspot.com/9754044/diff/16002/src/pkg/runtime/mgc0.c>
> File src/pkg/runtime/mgc0.c (right):
>
> https://codereview.appspot.**com/9754044/diff/16002/src/**
>
pkg/runtime/mgc0.c#newcode1991<https://codereview.appspot.com/9754044/diff/16002/src/pkg/runtime/mgc0.c#newcode1991>
> src/pkg/runtime/mgc0.c:1991: // now that gc is done and we're back on g
> stack, run some finalizers
> Can we use another worth than "some" here?  After all, we will run
> everything on the finalizer queue.
>

Fixed.


>
>
https://codereview.appspot.**com/9754044/<https://codereview.appspot.com/9754...
>

[iant, 2013-06-01 00:50:44]

LGTM

[khr, 2013-06-01 03:43:36]

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

runtime/gc: Run garbage collector on g0 stack
instead of regular g stack. We do this so that the g stack
we're currently running on is no longer changing.  Cuts
the root set down a bit (g0 stacks are not scanned, and
we don't need to scan gc's internal state).  Also an
enabler for copyable stacks.

R=golang-dev, cshapiro, khr, 0xe2.0x9a.0x9b, dvyukov, rsc, iant
CC=golang-dev
https://codereview.appspot.com/9754044