code review 86020043: sync: less agressive local caching in Pool

sync: less agressive local caching in Pool
Currently Pool can cache up to 15 elements per P, and these elements are not accesible to other Ps.
If a Pool caches large objects, say 2MB, and GOMAXPROCS is set to a large value, say 32,
then the Pool can waste up to 960MB.
The new caching policy caches at most 1 per-P element, the rest is shared between Ps.

Get/Put performance is unchanged. Nested Get/Put performance is 57% worse.
However, overall scalability of nested Get/Put is significantly improved,
so the new policy starts winning under contention.

benchmark                     old ns/op     new ns/op     delta
BenchmarkPool                 27.4          26.7          -2.55%
BenchmarkPool-4               6.63          6.59          -0.60%
BenchmarkPool-16              1.98          1.87          -5.56%
BenchmarkPool-64              1.93          1.86          -3.63%
BenchmarkPoolOverlflow        3970          6235          +57.05%
BenchmarkPoolOverlflow-4      10935         1668          -84.75%
BenchmarkPoolOverlflow-16     13419         520           -96.12%
BenchmarkPoolOverlflow-64     10295         380           -96.31%

[dvyukov, 2014-04-14 14:31:24]

Hello golang-codereviews@googlegroups.com,

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

[dvyukov, 2014-04-14 14:32:07]

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

Please take another look.

[dvyukov, 2014-04-14 14:34:13]

I am still running production tests, but for now this policy looks good.
With the current caching policy the Pool can be unusable with large objects.
What are the chances of pushing this into Go1.3.
Since the Pool is new in 1.3, I would consider the risks as low (nobody yet
depends on it).

[rsc, 2014-04-14 14:43:54]

LGTM but it needs to go in today so that it will be in the beta

https://codereview.appspot.com/86020043/diff/60002/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/86020043/diff/60002/src/pkg/runtime/mgc0.c#new...
src/pkg/runtime/mgc0.c:111: reflect·call(poolcleanup, (byte*)&i, 0, 0);
not sure why you pass &i here. if there are no args, doesn't nil work?

https://codereview.appspot.com/86020043/diff/60002/src/pkg/sync/pool.go
File src/pkg/sync/pool.go (right):

https://codereview.appspot.com/86020043/diff/60002/src/pkg/sync/pool.go#newco...
src/pkg/sync/pool.go:186: func poolCleanup() {
// This function is called with the world stopped, at the beginning of a garbage
collection.
// It must not allocate and probably should not call any runtime functions.

https://codereview.appspot.com/86020043/diff/60002/src/pkg/sync/pool.go#newco...
src/pkg/sync/pool.go:215: return
(*poolLocal)(unsafe.Pointer(uintptr(unsafe.Pointer(l)) +
unsafe.Sizeof(*l)*uintptr(i))) // uh...
this is needlessly complex.

return (*[10000]poolLocal)(unsafe.Pointer(l))[i]

or even better make the *poolLocal in the struct an unsafe.Pointer and this is
just

return (*[10000]poolLocal)(l)[i]

[dvyukov, 2014-04-14 16:48:09]

https://codereview.appspot.com/86020043/diff/60002/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/86020043/diff/60002/src/pkg/runtime/mgc0.c#new...
src/pkg/runtime/mgc0.c:111: reflect·call(poolcleanup, (byte*)&i, 0, 0);
On 2014/04/14 14:43:55, rsc wrote:
> not sure why you pass &i here. if there are no args, doesn't nil work?

It was crashing before with nil. Now it's not crashing.
Done.

https://codereview.appspot.com/86020043/diff/60002/src/pkg/sync/pool.go
File src/pkg/sync/pool.go (right):

https://codereview.appspot.com/86020043/diff/60002/src/pkg/sync/pool.go#newco...
src/pkg/sync/pool.go:186: func poolCleanup() {
On 2014/04/14 14:43:55, rsc wrote:
> // This function is called with the world stopped, at the beginning of a
garbage
> collection.
> // It must not allocate and probably should not call any runtime functions.

Done.

https://codereview.appspot.com/86020043/diff/60002/src/pkg/sync/pool.go#newco...
src/pkg/sync/pool.go:215: return
(*poolLocal)(unsafe.Pointer(uintptr(unsafe.Pointer(l)) +
unsafe.Sizeof(*l)*uintptr(i))) // uh...
On 2014/04/14 14:43:55, rsc wrote:
> this is needlessly complex.
> 
> return (*[10000]poolLocal)(unsafe.Pointer(l))[i]
> 
> or even better make the *poolLocal in the struct an unsafe.Pointer and this is
> just
> 
> return (*[10000]poolLocal)(l)[i]
> 

Done.

[dvyukov, 2014-04-14 17:13:36]

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

sync: less agressive local caching in Pool
Currently Pool can cache up to 15 elements per P, and these elements are not
accesible to other Ps.
If a Pool caches large objects, say 2MB, and GOMAXPROCS is set to a large value,
say 32,
then the Pool can waste up to 960MB.
The new caching policy caches at most 1 per-P element, the rest is shared
between Ps.

Get/Put performance is unchanged. Nested Get/Put performance is 57% worse.
However, overall scalability of nested Get/Put is significantly improved,
so the new policy starts winning under contention.

benchmark                     old ns/op     new ns/op     delta
BenchmarkPool                 27.4          26.7          -2.55%
BenchmarkPool-4               6.63          6.59          -0.60%
BenchmarkPool-16              1.98          1.87          -5.56%
BenchmarkPool-64              1.93          1.86          -3.63%
BenchmarkPoolOverlflow        3970          6235          +57.05%
BenchmarkPoolOverlflow-4      10935         1668          -84.75%
BenchmarkPoolOverlflow-16     13419         520           -96.12%
BenchmarkPoolOverlflow-64     10295         380           -96.31%

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

[gobot, 2014-04-14 22:46:00]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/0828cd3aae07c3907ac4d7e96deca4f37dea9550