code review 6273049: runtime: introduce Processors

runtime: introduce Processors
Introduce notion of P (Processors), there are exactly GOMAXPROCS Processors. Each M (Machine) has an associated P when executes Go code.
Move MCache to P.
Remove goroutine handoff (M.nextg).
Remove the atomic scheduling word, use schedlock again.
Handle locked goroutines and machines out of band. Locked goroutines are not put to runqueue, locked machines are not put to mhead.
Performance become slightly worse:
GOMAXPROCS=1 go test std
old: 48.27s
new: 49.33s
GOMAXPROCS=16 go test std
old: 52.69s
new: 53.95s
Tested: linux/darwin 386/amd64.
Ultimately more resources will be moved to P (runqueue), each P will have own mutex, common operations will be able to proceed w/o global mutex.

[dvyukov, 2012-07-01 12:36:39]

Patch Set 7 is known to pass all.bash on Linux/Darwin 386/amd64.

[dvyukov, 2012-07-04 18:36:31]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bpowers, 2012-07-05 19:01:04]

I'm not super qualified, but had a couple of nits/questions.

yours,
Bobby

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:842: schedunlock();
Is this necessary?  We end up calling it here and right after the if block.

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1354: newp[i].nextidle = runtime·sched.idlep;
since newp was just allocated and we just set runtime·sched.idlep to nil, this
doesn't do anything

[dvyukov, 2012-07-06 06:27:32]

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:842: schedunlock();
On 2012/07/05 19:01:04, bpowers wrote:
> Is this necessary?  We end up calling it here and right after the if block.

There is 'return' statement at the end of the block.

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1354: newp[i].nextidle = runtime·sched.idlep;
On 2012/07/05 19:01:04, bpowers wrote:
> since newp was just allocated and we just set runtime·sched.idlep to nil, this
> doesn't do anything

idlep is nil only on first iteration of the loop.
In general I am forming the stack of idle P's here.

[dvyukov, 2012-07-06 06:30:28]

FTR, I think current version (patch set 30) has problems with fairness. Since
mreadylocked is LIFO and always have priority over normal runqueue, a yielding
locked goroutine can starve other locked and non-locked goroutines.
However, perhaps this may be fixed in a subsequent change list.

[DMorsing, 2012-07-06 16:43:13]

This is the first time I've taken a thorough look at the go scheduler code, so
take anything I say with a grain of salt

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:56: byte waitstop;
The use of this field is subtle enough to warrant a comment explaining what it
does like in the previous version

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:71: M *kickoffm;
Needs a comment as well

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:440: } else {
else if here?

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:539: runtime·notesleep(&runtime·sched.stopped);
Can this sequence be simplified now that we're not looping here?

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:590: schedlock();
Not sure how this sequence does what the comment says it does or why it is
needed.

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:642: if(runtime·sched.gwait &&
This if statement is structured weirdly. I'd much rather prefer the comments
appear before the if statement in a block.

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1372: if(!tryentergo())
Looks to me like this could be replaced with entergo(m).

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1389: entergo(M *mp)
entergo name seems to imply that you're trying to get a goroutine to run, when
you're really trying to get a P in order to run Gs (if i understand this code
correctly)

[dvyukov, 2012-08-05 10:47:11]

Thanks!

I do not address code formatting/commenting comments, because I am still not
sure about high-level structure.

It's difficult to get to the final destination through several stable and
consistent points. I think it will be easier to do the same thing as with GC -
implement a final version in a huge CL, then commit it in parts to the extend
possible (most changes in proc.c will have to go in a single big CL anyway).

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:590: schedlock();
On 2012/07/06 16:43:13, DMorsing wrote:
> Not sure how this sequence does what the comment says it does or why it is
> needed.

See schedunlock(). newm() starts new thread, but only schedunlock() supplies it
with P. So new thread must wait for the parent thread to execute schedunlock().
This sequence does that.

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1372: if(!tryentergo())
On 2012/07/06 16:43:13, DMorsing wrote:
> Looks to me like this could be replaced with entergo(m).

Yes, I think you're right.

http://codereview.appspot.com/6273049/diff/25010/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1389: entergo(M *mp)
On 2012/07/06 16:43:13, DMorsing wrote:
> entergo name seems to imply that you're trying to get a goroutine to run, when
> you're really trying to get a P in order to run Gs (if i understand this code
> correctly)

A better name?

[dvyukov, 2012-08-06 13:45:38]

On 2012/08/05 10:47:11, dvyukov wrote:
> Thanks!
> 
> I do not address code formatting/commenting comments, because I am still not
> sure about high-level structure.
> 
> It's difficult to get to the final destination through several stable and
> consistent points. I think it will be easier to do the same thing as with GC -
> implement a final version in a huge CL, then commit it in parts to the extend
> possible (most changes in proc.c will have to go in a single big CL anyway).


... and I need a proof-of-performance prototype.
Please, disregard this CL.

[rsc, 2012-08-06 20:21:38]

removing reviewers