code review 6223050: runtime: faster GC mark phase

runtime: faster GC mark phase
Also bump MaxGcproc to 8.

benchmark             old ns/op    new ns/op    delta
Parser               3796323000   3763880000   -0.85%
Parser-2             3591752500   3518560250   -2.04%
Parser-4             3423825250   3334955250   -2.60%
Parser-8             3304585500   3267014750   -1.14%
Parser-16            3313615750   3286160500   -0.83%

Tree                  984128500    942501166   -4.23%
Tree-2                932564444    883266222   -5.29%
Tree-4                835831000    799912777   -4.30%
Tree-8                819238500    789717333   -3.73%
Tree-16               880837833    837840055   -5.13%

Tree2                 604698100    579716900   -4.13%
Tree2-2               372414500    356765200   -4.20%
Tree2-4               187488100    177455900   -5.56%
Tree2-8               136315300    102086700  -25.11%
Tree2-16               93725900     76705800  -22.18%

ParserPause           157441210    166202783   +5.56%
ParserPause-2          93842650     85199900   -9.21%
ParserPause-4          56844404     53535684   -5.82%
ParserPause-8          35739446     30767613  -16.15%
ParserPause-16         32718255     27212441  -16.83%

TreePause              29610557     29787725   +0.60%
TreePause-2            24001659     20674421  -13.86%
TreePause-4            15114887     12842781  -15.03%
TreePause-8            13128725     10741747  -22.22%
TreePause-16           16131360     12506901  -22.47%

Tree2Pause           2673350920   2651045280   -0.83%
Tree2Pause-2         1796999200   1709350040   -4.88%
Tree2Pause-4         1163553320   1090706480   -6.67%
Tree2Pause-8          987032520    858916360  -25.11%
Tree2Pause-16         864758560    809567480   -6.81%

ParserLastPause       280537000    289047000   +3.03%
ParserLastPause-2     183030000    166748000   -8.90%
ParserLastPause-4     105817000     91552000  -13.48%
ParserLastPause-8      65127000     53288000  -18.18%
ParserLastPause-16     45258000     38334000  -15.30%

TreeLastPause          45072000     51449000  +12.39%
TreeLastPause-2        39269000     37866000   -3.57%
TreeLastPause-4        23564000     20649000  -12.37%
TreeLastPause-8        20881000     15807000  -24.30%
TreeLastPause-16       23297000     17309000  -25.70%

Tree2LastPause       6046912000   5797120000   -4.13%
Tree2LastPause-2     3724034000   3567592000   -4.20%
Tree2LastPause-4     1874831000   1774524000   -5.65%
Tree2LastPause-8     1363108000   1020809000  -12.79%
Tree2LastPause-16     937208000    767019000  -22.18%

[dvyukov, 2012-05-23 13:58:24]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

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

[dvyukov, 2012-05-23 14:46:54]

I've also collected additional stats on tree benchmark.
BEFORE:
GOMAXPROCS=1:  exec time 16690ms, GC 5846ms (35.30%), mark 3602ms (21.58%),
sweep 2244ms (13.44%)
GOMAXPROCS=2:  exec time 14956ms, GC 3559ms (23.79%), mark 2244ms (15.00%),
sweep 1314ms ( 8.79%)
GOMAXPROCS=4:  exec time 14113ms, GC 2625ms (18.60%), mark 1626ms (11.52%),
sweep 999ms  ( 7.70%)
GOMAXPROCS=8:  exec time 13787ms, GC 1962ms (14.23%), mark 1096ms ( 7.95%),
sweep 865ms  ( 6.27%)
GOMAXPROCS=16: exec time 14927ms, GC 2702ms (18.10%), mark 1614ms (10.81%),
sweep 1088ms ( 7.29%)

AFTER:
GOMAXPROCS=1:  exec time 16564ms, GC 5869ms (35.43%), mark 3646ms (22.10%),
sweep 2222ms (13.41%)
GOMAXPROCS=2:  exec time 15432ms, GC 3970ms (25.72%), mark 2340ms (15.16%),
sweep 1629ms (10.55%)
GOMAXPROCS=4:  exec time 14076ms, GC 2537ms (18.20%), mark 1408ms (10.00%),
sweep 1128ms ( 8.20%)
GOMAXPROCS=8:  exec time 13334ms, GC 1799ms (13.49%), mark 1058ms ( 7.93%),
sweep 741ms  ( 5.56%)
GOMAXPROCS=16: exec time 13162ms, GC 1718ms (13.50%), mark 1010ms ( 7.67%),
sweep 707ms  ( 5.37%)

That's on Intel Xeon E5620 2.40GHz, 8 HT cores. It does not slow down on 16
threads now. But I think we better set MaxGcproc=8 for now, especially taking
into account that I tested only on 8 HT cores machine.

[atom, 2012-05-23 18:29:25]

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:261: // If another proc wants a pointer, give it some.
Is there a reason why this was moved here?

[dvyukov, 2012-05-23 18:36:49]

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:261: // If another proc wants a pointer, give it some.
On 2012/05/23 18:29:25, atom wrote:
> Is there a reason why this was moved here?

I wrote this 6+ month ago, so I am not sure :)
I think that the reason was that a thread may not get to that point below for a
long time. It was at the end of the loop, I moved it to the middle of the loop;
so it's kind of compromise between affecting hot-path and responsiveness.

[atom, 2012-05-23 19:17:39]

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:261: // If another proc wants a pointer, give it some.
On 2012/05/23 18:36:50, dvyukov wrote:
> On 2012/05/23 18:29:25, atom wrote:
> > Is there a reason why this was moved here?
> 
> I wrote this 6+ month ago, so I am not sure :)
> I think that the reason was that a thread may not get to that point below for
a
> long time. It was at the end of the loop, I moved it to the middle of the
loop;
> so it's kind of compromise between affecting hot-path and responsiveness.

If (nobj==4) then the test (nobj>4) will always fail unless you increment nobj.
You moved the code into a place where you don't know whether nobj will be
incremented. Maybe it should be placed below nobj++.

In any case, my patch (http://codereview.appspot.com/6114046/) contains a
rewrite of function scanblock(), so when it gets merged some of your changes
will be lost.

[rsc, 2012-05-23 19:20:41]

On Wed, May 23, 2012 at 3:17 PM,  <0xE2.0x9A.0x9B@gmail.com> wrote:
> In any case, my patch (http://codereview.appspot.com/6114046/) contains
> a rewrite of function scanblock(), so when it gets merged some of your
> changes will be lost.

I'm a little worried about this statement.
Part of getting Dmitriy's patch in first means that it's your job to merge it
into your patch, without losing the changes, at least without losing the
important ones.

Russ

[rsc, 2012-05-23 19:59:04]

LGTM

It is interesting to me that the Parser pause times went down so noticeably
(~15%) but the overall Parser run time did not.  Do you understand why that is?

Is this the last of the CLs?

Thanks.
Russ

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:261: // If another proc wants a pointer, give it some.
I don't understand: why is nobj==4 a concern?  This code was above the nobj++
before, and it still is.  Is nobj==4 a case that happens that I'm missing?

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:609: default:
Please unindent these cases, like they were before.  The standard switch style
in this code is

switch(x) {
case 1:
     indented()
case 2:
     indented()
}

The words switch and case should be aligned.

I know it's annoying.  I actually looked into building a cfmt out of parts I had
lying around, but it seemed like overkill and a distraction.

[dvyukov, 2012-05-24 06:32:21]

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:261: // If another proc wants a pointer, give it some.
On 2012/05/23 19:59:04, rsc wrote:
> I don't understand: why is nobj==4 a concern?  This code was above the nobj++
> before, and it still is.  Is nobj==4 a case that happens that I'm missing?

I also do not see what's wrong with nobj==4.
I've just made it checked more frequently (in the hope to avoid at least some of
corner cases, and it seems to not affect performance in this new place). And
also reordered checks.

[dvyukov, 2012-05-24 06:38:29]

>
http://codereview.appspot.com/6223050/diff/12001/src/pkg/runtime/mgc0.c#newco...
> src/pkg/runtime/mgc0.c:609: default:
> Please unindent these cases, like they were before.  The standard switch style
> in this code is
> 
> switch(x) {
> case 1:
>      indented()
> case 2:
>      indented()
> }
> 
> The words switch and case should be aligned.
> 
> I know it's annoying.  I actually looked into building a cfmt out of parts I
had
> lying around, but it seemed like overkill and a distraction.

Done.
I guess it was some evil IDE. Moving spaces around for 20 yrs, I am OK to move a
bit more :)

[dvyukov, 2012-05-24 06:49:07]

On 2012/05/23 19:59:04, rsc wrote:
> LGTM
> 
> It is interesting to me that the Parser pause times went down so noticeably
> (~15%) but the overall Parser run time did not.  Do you understand why that
is?

Whether it's good or not, parser is not garbage benchmark anymore. Here are new
stats for GOMAXPROCS=16:
exec time 26517ms, GC 1397ms (5.27%), mark 891ms (3.36%), sweep 506ms (1.91%),
total GCs 42
GC takes only 5% of time. If you cut 20% out of 5%, that only 1%.
Taking into account that parser still mostly generates garbage, I think
bandwidth component of GC problem may be considered solved.


> Is this the last of the CLs?

Yes, I've finished porting that big CL. Some micro-tuning is still possible, and
there is that "concurrent sweep" thing. But I want to take a break with GC work.
From that GC stats above, sweep takes 2% of time, and mean sweep pause is 12ms.
I think that scheduler improvements will have much higher impact of bandwidth
and latency.

[dvyukov, 2012-05-24 06:59:01]

*** Submitted as http://code.google.com/p/go/source/detail?r=0ed15bfbe0f4 ***

runtime: faster GC mark phase
Also bump MaxGcproc to 8.

benchmark             old ns/op    new ns/op    delta
Parser               3796323000   3763880000   -0.85%
Parser-2             3591752500   3518560250   -2.04%
Parser-4             3423825250   3334955250   -2.60%
Parser-8             3304585500   3267014750   -1.14%
Parser-16            3313615750   3286160500   -0.83%

Tree                  984128500    942501166   -4.23%
Tree-2                932564444    883266222   -5.29%
Tree-4                835831000    799912777   -4.30%
Tree-8                819238500    789717333   -3.73%
Tree-16               880837833    837840055   -5.13%

Tree2                 604698100    579716900   -4.13%
Tree2-2               372414500    356765200   -4.20%
Tree2-4               187488100    177455900   -5.56%
Tree2-8               136315300    102086700  -25.11%
Tree2-16               93725900     76705800  -22.18%

ParserPause           157441210    166202783   +5.56%
ParserPause-2          93842650     85199900   -9.21%
ParserPause-4          56844404     53535684   -5.82%
ParserPause-8          35739446     30767613  -16.15%
ParserPause-16         32718255     27212441  -16.83%

TreePause              29610557     29787725   +0.60%
TreePause-2            24001659     20674421  -13.86%
TreePause-4            15114887     12842781  -15.03%
TreePause-8            13128725     10741747  -22.22%
TreePause-16           16131360     12506901  -22.47%

Tree2Pause           2673350920   2651045280   -0.83%
Tree2Pause-2         1796999200   1709350040   -4.88%
Tree2Pause-4         1163553320   1090706480   -6.67%
Tree2Pause-8          987032520    858916360  -25.11%
Tree2Pause-16         864758560    809567480   -6.81%

ParserLastPause       280537000    289047000   +3.03%
ParserLastPause-2     183030000    166748000   -8.90%
ParserLastPause-4     105817000     91552000  -13.48%
ParserLastPause-8      65127000     53288000  -18.18%
ParserLastPause-16     45258000     38334000  -15.30%

TreeLastPause          45072000     51449000  +12.39%
TreeLastPause-2        39269000     37866000   -3.57%
TreeLastPause-4        23564000     20649000  -12.37%
TreeLastPause-8        20881000     15807000  -24.30%
TreeLastPause-16       23297000     17309000  -25.70%

Tree2LastPause       6046912000   5797120000   -4.13%
Tree2LastPause-2     3724034000   3567592000   -4.20%
Tree2LastPause-4     1874831000   1774524000   -5.65%
Tree2LastPause-8     1363108000   1020809000  -12.79%
Tree2LastPause-16     937208000    767019000  -22.18%

R=rsc, 0xe2.0x9a.0x9b
CC=golang-dev
http://codereview.appspot.com/6223050

[rsc, 2012-05-24 14:00:58]

Thank you so much for seeing this through. I am more than happy to put
off concurrent sweep for now.

Russ