code review 162980043: sync: release Pool memory during second and later GCs

sync: release Pool memory during second and later GCs

Pool memory was only being released during the first GC after the first Put.

Put assumes that p.local != nil means p is on the allPools list.
poolCleanup (called during each GC) removed each pool from allPools
but did not clear p.local, so each pool was cleared by exactly one GC
and then never cleared again.

This bug was introduced late in the Go 1.3 release cycle.

Fixes issue 8979.

[dvyukov, 2014-10-22 09:22:15]

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

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

[bradfitz, 2014-10-22 10:36:54]

Mention that it's been broken since
https://code.google.com/p/go/source/detail?r=ad21d6ca5d82 in the commit message.

Also, file a bug and reference that in the commit message. And tag the bug
"Release-Go1.3.4" for consideration for inclusion if/when that release is ever
made.

https://codereview.appspot.com/162980043/diff/40001/src/sync/pool_test.go
File src/sync/pool_test.go (right):

https://codereview.appspot.com/162980043/diff/40001/src/sync/pool_test.go#new...
src/sync/pool_test.go:116: time.Sleep(time.Duration(i*100+10) *
time.Millisecond)
how long does this test typically take? Skip it during test.Short if long.

https://codereview.appspot.com/162980043/diff/40001/src/sync/pool_test.go#new...
src/sync/pool_test.go:123: atomic.LoadUint32(&fin), N, try)
this atomic.LoadUint32 may not necessarily match what was tested earlier.  Use a
variable instead of two calls to atomic.LoadUint32?

[dvyukov, 2014-10-22 13:05:49]

Filed the bug, and referenced it from cl description:
Fixes issue 8979.

PTAL

https://codereview.appspot.com/162980043/diff/40001/src/sync/pool_test.go
File src/sync/pool_test.go (right):

https://codereview.appspot.com/162980043/diff/40001/src/sync/pool_test.go#new...
src/sync/pool_test.go:116: time.Sleep(time.Duration(i*100+10) *
time.Millisecond)
On 2014/10/22 10:36:54, bradfitz wrote:
> how long does this test typically take? Skip it during test.Short if long.

0.03s

the test above this one does the same

https://codereview.appspot.com/162980043/diff/40001/src/sync/pool_test.go#new...
src/sync/pool_test.go:123: atomic.LoadUint32(&fin), N, try)
On 2014/10/22 10:36:54, bradfitz wrote:
> this atomic.LoadUint32 may not necessarily match what was tested earlier.  Use
a
> variable instead of two calls to atomic.LoadUint32?

Done.

[r, 2014-10-22 13:11:18]

https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go
File src/sync/pool_test.go (right):

https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go#new...
src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) {
this is likely to be flaky and is also time-driven, so please give the builders
a hand and protect it with testing.Short

[dvyukov, 2014-10-22 13:47:05]

https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go
File src/sync/pool_test.go (right):

https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go#new...
src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) {
On 2014/10/22 13:11:18, r wrote:
> this is likely to be flaky and is also time-driven, so please give the
builders
> a hand and protect it with testing.Short

The preceding test has exactly the same logic and we we did not see flakes on
it. I would prefer to not give up positions. A non-short test is roughly the
same as no test at all.

[rsc, 2014-10-22 13:50:25]

Can you please explain what memory is being retained during GC incorrectly?
The bug says the pool retains "all memory ever put into it" but it sure looks
like lines 195-202 are clearing all the possible places that the 'memory put
into' the Pool could be kept. Where are the pointers keeping that memory alive?

I can believe that the Pool is retaining some of its own bookkeeping structures,
but that's different.

[dvyukov, 2014-10-22 14:21:34]

On 2014/10/22 13:50:25, rsc wrote:
> Can you please explain what memory is being retained during GC incorrectly?
> The bug says the pool retains "all memory ever put into it" but it sure looks
> like lines 195-202 are clearing all the possible places that the 'memory put
> into' the Pool could be kept. Where are the pointers keeping that memory
alive?
> 
> I can believe that the Pool is retaining some of its own bookkeeping
structures,
> but that's different.

The problem is that if we don't do "p.localSize = 0" the pool is not registered
in allPools again. So during the first GC we indeed clear everything. But then
the pool is no registered in allPools again, and all user objects get retained
infinitely.

[rsc, 2014-10-22 14:42:01]

https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go
File src/sync/pool_test.go (right):

https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go#new...
src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) {
Why are there two tests here? It looks like the second test is the first test
run three times. If that's true, just put a loop into the first test instead of
creating a second one.

[rsc, 2014-10-22 14:51:11]

On Wed, Oct 22, 2014 at 10:21 AM, <dvyukov@google.com> wrote:

> The problem is that if we don't do "p.localSize = 0" the pool is not
> registered in allPools again. So during the first GC we indeed clear
> everything. But then the pool is no registered in allPools again, and
> all user objects get retained infinitely.
>

Great. That's the kind of thing the CL description should say. What it says
is both untrue and incomplete. The CL description needs to make sense to
people not intimately familiar with the code. Something like this would be
better here:

    sync: release Pool memory during second and later GCs

    Pool memory was only being released during the first GC after the first
Put.

    Put assumes that p.local != nil means p is on the allPools list.
    poolCleanup (called during each GC) removed each pool from allPools
    but did not clear p.local, so each pool was cleared by exactly one GC
    and then never cleared again.

    This bug was introduced late in the Go 1.3 release cycle.

    Fixes issue XXX.

Thanks.
Russ

[dvyukov, 2014-10-22 14:57:10]

https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go
File src/sync/pool_test.go (right):

https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go#new...
src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) {
On 2014/10/22 14:42:01, rsc wrote:
> Why are there two tests here? It looks like the second test is the first test
> run three times. If that's true, just put a loop into the first test instead
of
> creating a second one.

The first test takes all elements out of the pool, and then checks that the pool
does not retain pointers to elements that were taken out.
The second test does not take all elements out of the pool, and checks that the
pool release elements eventually.
I agree that they are close, but not exactly the same.
One can argue that the second test is strictly stronger than the first one. I
don't know whether it is true for all possible implementations.

[dvyukov, 2014-10-22 14:58:50]

On 2014/10/22 14:51:11, rsc wrote:
> On Wed, Oct 22, 2014 at 10:21 AM, <mailto:dvyukov@google.com> wrote:
> 
> > The problem is that if we don't do "p.localSize = 0" the pool is not
> > registered in allPools again. So during the first GC we indeed clear
> > everything. But then the pool is no registered in allPools again, and
> > all user objects get retained infinitely.
> >
> 
> Great. That's the kind of thing the CL description should say. What it says
> is both untrue and incomplete. The CL description needs to make sense to
> people not intimately familiar with the code. Something like this would be
> better here:
> 
>     sync: release Pool memory during second and later GCs
> 
>     Pool memory was only being released during the first GC after the first
> Put.
> 
>     Put assumes that p.local != nil means p is on the allPools list.
>     poolCleanup (called during each GC) removed each pool from allPools
>     but did not clear p.local, so each pool was cleared by exactly one GC
>     and then never cleared again.
> 
>     This bug was introduced late in the Go 1.3 release cycle.
> 
>     Fixes issue XXX.
> 
> Thanks.
> Russ

Done

[rsc, 2014-10-22 15:14:32]

https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go
File src/sync/pool_test.go (right):

https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go#new...
src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) {
Can you pull the body into a helper

func testPool(t *testing.T, p *Pool, drain bool, context string) {
	var fin uint32
	const N = 100
	for i := 0; i < N; i++ {
		v := new(string)
		runtime.SetFinalizer(v, func(vv *string) {
			atomic.AddUint32(&fin, 1)
		})
		p.Put(v)
	}
	if drain {
		for i := 0; i < N; i++ {
			p.Get()
		}
	}
	var fin1 uint32
	for i := 0; i < 5; i++ {
		runtime.GC()
		time.Sleep(time.Duration(i*100+10) * time.Millisecond)
		// 1 pointer can remain on stack or elsewhere
		if fin1 = atomic.LoadUint32(&fin); fin1 >= N-1 {
			return
		}
	}
	t.Fatalf("%s: only %v out of %v resources are finalized", context, fin1, N)
}

and then make the two tests call the helper? That will make the difference much
clearer.

[dvyukov, 2014-10-22 16:05:26]

On 2014/10/22 15:14:32, rsc wrote:
> https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go
> File src/sync/pool_test.go (right):
> 
>
https://codereview.appspot.com/162980043/diff/60001/src/sync/pool_test.go#new...
> src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) {
> Can you pull the body into a helper
> 
> func testPool(t *testing.T, p *Pool, drain bool, context string) {
> 	var fin uint32
> 	const N = 100
> 	for i := 0; i < N; i++ {
> 		v := new(string)
> 		runtime.SetFinalizer(v, func(vv *string) {
> 			atomic.AddUint32(&fin, 1)
> 		})
> 		p.Put(v)
> 	}
> 	if drain {
> 		for i := 0; i < N; i++ {
> 			p.Get()
> 		}
> 	}
> 	var fin1 uint32
> 	for i := 0; i < 5; i++ {
> 		runtime.GC()
> 		time.Sleep(time.Duration(i*100+10) * time.Millisecond)
> 		// 1 pointer can remain on stack or elsewhere
> 		if fin1 = atomic.LoadUint32(&fin); fin1 >= N-1 {
> 			return
> 		}
> 	}
> 	t.Fatalf("%s: only %v out of %v resources are finalized", context, fin1, N)
> }
> 
> and then make the two tests call the helper? That will make the difference
much
> clearer.



Done
PTAL

I did not add the context parameter, as the test name will be printed on
failure.

[rsc, 2014-10-22 16:17:35]

LGTM

[dvyukov, 2014-10-22 16:23:57]

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

sync: release Pool memory during second and later GCs

Pool memory was only being released during the first GC after the first Put.

Put assumes that p.local != nil means p is on the allPools list.
poolCleanup (called during each GC) removed each pool from allPools
but did not clear p.local, so each pool was cleared by exactly one GC
and then never cleared again.

This bug was introduced late in the Go 1.3 release cycle.

Fixes issue 8979.

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