code review 123640043: runtime: convert common scheduler functions to Go

runtime: convert common scheduler functions to Go
These are required for chans, semaphores, timers, etc.

[dvyukov, 2014-08-20 16:34:07]

Hello golang-codereviews@googlegroups.com (cc: khr@golang.org, rlh@golang.org,
rsc@golang.org),

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

[dvyukov, 2014-08-20 16:43:41]

Keith, let's get this is asap. It will simplify your chans change; sync
semaphores, async semaphores, timers and probably something else depends on it.

[rsc, 2014-08-20 18:21:40]

R=khr
looks good to me

https://codereview.appspot.com/123640043/diff/100001/src/pkg/runtime/proc.go
File src/pkg/runtime/proc.go (right):

https://codereview.appspot.com/123640043/diff/100001/src/pkg/runtime/proc.go#...
src/pkg/runtime/proc.go:10: gStatusidle = iota
s/idle/Idle/

https://codereview.appspot.com/123640043/diff/100001/src/pkg/runtime/proc.go#...
src/pkg/runtime/proc.go:27: // reason must be a static string so that it is
null-terminated.
This seems like a mistake. Change waitreason to be a Go string and use
gostringnocopy from C and %S to print it.

[khr, 2014-08-20 18:25:01]

What is nrelease for?  It isn't used in this CL.

I think it is a lot simpler to just keep one SudoG per G.  The stack copier then
knows where it is, for example.  I'm not particularly worried about the space -
if there are lots of goroutines they are probably mostly blocked on something,
which means most of them need a sudog anyway.

https://codereview.appspot.com/123640043/diff/100001/src/pkg/runtime/proc.go
File src/pkg/runtime/proc.go (right):

https://codereview.appspot.com/123640043/diff/100001/src/pkg/runtime/proc.go#...
src/pkg/runtime/proc.go:79: c.sudogcache = s
We could clear the sudogcache at every GC.

[dvyukov, 2014-08-21 05:45:33]

On 2014/08/20 18:25:01, khr wrote:
> What is nrelease for?  It isn't used in this CL.

It's for sync semaphores (https://codereview.appspot.com/130340043/). They also
use SudoG's for blocking.



> I think it is a lot simpler to just keep one SudoG per G.  The stack copier
then
> knows where it is, for example.  I'm not particularly worried about the space
-
> if there are lots of goroutines they are probably mostly blocked on something,
> which means most of them need a sudog anyway.

Selects need N sudog's per G. Also goroutines can be blocked on network IO,
which does not need sudog.


 
> https://codereview.appspot.com/123640043/diff/100001/src/pkg/runtime/proc.go
> File src/pkg/runtime/proc.go (right):
> 
>
https://codereview.appspot.com/123640043/diff/100001/src/pkg/runtime/proc.go#...
> src/pkg/runtime/proc.go:79: c.sudogcache = s
> We could clear the sudogcache at every GC.

I do it in mgc0.c

[dvyukov, 2014-08-21 12:17:54]

PTAL

https://codereview.appspot.com/123640043/diff/100001/src/pkg/runtime/proc.go
File src/pkg/runtime/proc.go (right):

https://codereview.appspot.com/123640043/diff/100001/src/pkg/runtime/proc.go#...
src/pkg/runtime/proc.go:27: // reason must be a static string so that it is
null-terminated.
On 2014/08/20 18:21:39, rsc wrote:
> This seems like a mistake. Change waitreason to be a Go string and use
> gostringnocopy from C and %S to print it.

Done

[khr, 2014-08-21 16:26:08]

LGTM.

[dvyukov, 2014-08-21 16:41:15]

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

runtime: convert common scheduler functions to Go
These are required for chans, semaphores, timers, etc.

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

[josharian, 2014-08-21 17:51:22]

vet doesn't like the amd64 runtime·gocputicks:

pkg/runtime/asm_amd64.s:958: [amd64] invalid MOVL of ret+0(FP); int64 is 8-byte
value
pkg/runtime/asm_amd64.s:959: [amd64] invalid offset ret+4(FP); expected
ret+0(FP)

[gobot, 2014-08-21 17:59:34]

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