code review 107290044: runtime: make NumGoroutines faster

runtime: make NumGoroutines faster
Resolves TODO for not walking all goroutines in NumGoroutines.

[dvyukov, 2014-06-22 06:28:44]

Hello golang-codereviews@googlegroups.com (cc: khr@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-06-24 02:54:34]

https://codereview.appspot.com/107290044/diff/2/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/107290044/diff/2/src/pkg/runtime/proc.c#newcod...
src/pkg/runtime/proc.c:2129: n += runtime·allglen;
This math seems obfuscated to me.  How about

n = runtime.allglen;
n -= runtime.sched.ngfree;
for(p=...)
  n -= p->gfreecnt;

It's clearer what the math is doing this way.

https://codereview.appspot.com/107290044/diff/2/src/pkg/runtime/proc.c#newcod...
src/pkg/runtime/proc.c:2132: n = 1;  // at least we are running
Does this happen?  If so, why?

[dvyukov, 2014-07-15 10:50:42]

On 2014/06/24 02:54:34, khr wrote:
> https://codereview.appspot.com/107290044/diff/2/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
> 
>
https://codereview.appspot.com/107290044/diff/2/src/pkg/runtime/proc.c#newcod...
> src/pkg/runtime/proc.c:2129: n += runtime·allglen;
> This math seems obfuscated to me.  How about
> 
> n = runtime.allglen;
> n -= runtime.sched.ngfree;
> for(p=...)
>   n -= p->gfreecnt;
> 
> It's clearer what the math is doing this way.
> 
>
https://codereview.appspot.com/107290044/diff/2/src/pkg/runtime/proc.c#newcod...
> src/pkg/runtime/proc.c:2132: n = 1;  // at least we are running
> Does this happen?  If so, why?

All these variables can be changed concurrently, so the result can be
inconsistent.
For the per-P data there is no lock that we could take here at all.
Locking sched is misleading here, because it does not actually protect most that
data. I've removed it entirely.
PTAL.

[khr, 2014-07-16 22:45:01]

On 2014/07/15 10:50:42, dvyukov wrote:
> On 2014/06/24 02:54:34, khr wrote:
> > https://codereview.appspot.com/107290044/diff/2/src/pkg/runtime/proc.c
> > File src/pkg/runtime/proc.c (right):
> > 
> >
>
https://codereview.appspot.com/107290044/diff/2/src/pkg/runtime/proc.c#newcod...
> > src/pkg/runtime/proc.c:2129: n += runtime·allglen;
> > This math seems obfuscated to me.  How about
> > 
> > n = runtime.allglen;
> > n -= runtime.sched.ngfree;
> > for(p=...)
> >   n -= p->gfreecnt;
> > 
> > It's clearer what the math is doing this way.
> > 
> >
>
https://codereview.appspot.com/107290044/diff/2/src/pkg/runtime/proc.c#newcod...
> > src/pkg/runtime/proc.c:2132: n = 1;  // at least we are running
> > Does this happen?  If so, why?
> 
> All these variables can be changed concurrently, so the result can be
> inconsistent.
> For the per-P data there is no lock that we could take here at all.
> Locking sched is misleading here, because it does not actually protect most
that
> data. I've removed it entirely.
> PTAL.

LGTM.

[dvyukov, 2014-07-17 17:51:08]

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

runtime: make NumGoroutines faster
Resolves TODO for not walking all goroutines in NumGoroutines.

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

[gobot, 2014-07-17 18:25:17]

This CL appears to have broken the nacl-amd64p32 builder.
See http://build.golang.org/log/42d5e467865c9ac6a4d14f0eca1c28f088c8718d

[minux, 2014-07-17 18:33:33]

On Thu, Jul 17, 2014 at 2:25 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the nacl-amd64p32 builder.
> See http://build.golang.org/log/42d5e467865c9ac6a4d14f0eca1c28f088c8718d

This is real. my local build also reproduces it.

[dvyukov, 2014-07-18 08:58:23]

On Thu, Jul 17, 2014 at 10:33 PM, minux <minux@golang.org> wrote:
>
> On Thu, Jul 17, 2014 at 2:25 PM, <gobot@golang.org> wrote:
>>
>> This CL appears to have broken the nacl-amd64p32 builder.
>> See http://build.golang.org/log/42d5e467865c9ac6a4d14f0eca1c28f088c8718d
>
> This is real. my local build also reproduces it.

I don't see how my change can trigger that "unaligned stack size 12".
Looks like a latent bug. Anyway now there is
https://code.google.com/p/go/issues/detail?id=8379 filed for this.