code review 46970043: runtime: allocate goroutine ids in batches

runtime: allocate goroutine ids in batches
Helps reduce contention on sched.goidgen.

benchmark                               old ns/op    new ns/op    delta
BenchmarkCreateGoroutines-16                  259          237   -8.49%
BenchmarkCreateGoroutinesParallel-16          127           43  -66.06%

[dvyukov, 2014-01-01 21:26:18]

Hello golang-codereviews@googlegroups.com,

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

[dave_cheney.net, 2014-01-01 21:29:18]

This change makes good sense. How did you arrive at the batching figure of 16 ?

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

https://codereview.appspot.com/46970043/diff/60001/src/pkg/runtime/proc.c#new...
src/pkg/runtime/proc.c:1787: p->goidcacheend = p->goidcache + 16;
16 needs to be a constant.

[dvyukov, 2014-01-01 21:36:32]

that's the first number that came to my mind...
i've also tried 64 and it cuts few more ns, but that won't be
noticeable on any real workload


On Thu, Jan 2, 2014 at 1:29 AM,  <dave@cheney.net> wrote:
> This change makes good sense. How did you arrive at the batching figure
> of 16 ?
>
>
> https://codereview.appspot.com/46970043/diff/60001/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
>
>
https://codereview.appspot.com/46970043/diff/60001/src/pkg/runtime/proc.c#new...
> src/pkg/runtime/proc.c:1787: p->goidcacheend = p->goidcache + 16;
> 16 needs to be a constant.
>
> https://codereview.appspot.com/46970043/

[dvyukov, 2014-01-02 08:59:42]

>
https://codereview.appspot.com/46970043/diff/60001/src/pkg/runtime/proc.c#new...
> > src/pkg/runtime/proc.c:1787: p->goidcacheend = p->goidcache + 16;
> > 16 needs to be a constant.
> >
> > https://codereview.appspot.com/46970043/


Done
PTAL
does it look better?

[dave_cheney.net, 2014-01-02 09:04:52]

On 2014/01/02 08:59:42, dvyukov wrote:
> >
>
https://codereview.appspot.com/46970043/diff/60001/src/pkg/runtime/proc.c#new...
> > > src/pkg/runtime/proc.c:1787: p->goidcacheend = p->goidcache + 16;
> > > 16 needs to be a constant.
> > >
> > > https://codereview.appspot.com/46970043/
> 
> 
> Done
> PTAL
> does it look better?

Thank you. I'll do some testing on arm as I have a quad core machine which
should show some improvements.

Although we've never guaranteed that goids are (quasi) sequential, and it isn't
exposed in the runtime, I wonder if this change constitutes a (minor) change in
observable behavior which should be at least mentioned in the Go 1.3 release
notes.

[bradfitz, 2014-01-02 18:15:47]

On Thu, Jan 2, 2014 at 1:04 AM, <dave@cheney.net> wrote:

> On 2014/01/02 08:59:42, dvyukov wrote:
>
>> >
>>
>
> https://codereview.appspot.com/46970043/diff/60001/src/
> pkg/runtime/proc.c#newcode1787
>
>> > > src/pkg/runtime/proc.c:1787: p->goidcacheend = p->goidcache + 16;
>> > > 16 needs to be a constant.
>> > >
>> > > https://codereview.appspot.com/46970043/
>>
>
>
>  Done
>> PTAL
>> does it look better?
>>
>
> Thank you. I'll do some testing on arm as I have a quad core machine
> which should show some improvements.
>
> Although we've never guaranteed that goids are (quasi) sequential, and
> it isn't exposed in the runtime, I wonder if this change constitutes a
> (minor) change in observable behavior which should be at least mentioned
> in the Go 1.3 release notes.


I wouldn't worry about it.

The number will only be off by num_procs * 16.  Even at 48 processors or
whatever big machines have today, that's still noise compared to a typical
long-running program's total goroutine creation count.

[dave_cheney.net, 2014-01-02 21:51:04]

Excellent. I'm glad you feel this way. 

> On 3 Jan 2014, at 5:15, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> 
> 
> 
> 
>> On Thu, Jan 2, 2014 at 1:04 AM, <dave@cheney.net> wrote:
>>> On 2014/01/02 08:59:42, dvyukov wrote:
>>> >
>> 
>>
https://codereview.appspot.com/46970043/diff/60001/src/pkg/runtime/proc.c#new...
>>> > > src/pkg/runtime/proc.c:1787: p->goidcacheend = p->goidcache + 16;
>>> > > 16 needs to be a constant.
>>> > >
>>> > > https://codereview.appspot.com/46970043/
>> 
>> 
>>> Done
>>> PTAL
>>> does it look better?
>> 
>> Thank you. I'll do some testing on arm as I have a quad core machine
>> which should show some improvements.
>> 
>> Although we've never guaranteed that goids are (quasi) sequential, and
>> it isn't exposed in the runtime, I wonder if this change constitutes a
>> (minor) change in observable behavior which should be at least mentioned
>> in the Go 1.3 release notes.
> 
> I wouldn't worry about it.
> 
> The number will only be off by num_procs * 16.  Even at 48 processors or
whatever big machines have today, that's still noise compared to a typical
long-running program's total goroutine creation count.
> 

[dvyukov, 2014-01-18 15:44:16]

ping

[khr, 2014-01-21 22:04:25]

On 2014/01/18 15:44:16, dvyukov wrote:
> ping

LGTM.

[dvyukov, 2014-01-22 06:34:47]

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

runtime: allocate goroutine ids in batches
Helps reduce contention on sched.goidgen.

benchmark                               old ns/op    new ns/op    delta
BenchmarkCreateGoroutines-16                  259          237   -8.49%
BenchmarkCreateGoroutinesParallel-16          127           43  -66.06%

R=golang-codereviews, dave, bradfitz, khr
CC=golang-codereviews, rsc
https://codereview.appspot.com/46970043