Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(61)

Issue 162980043: code review 162980043: sync: release Pool memory during second and later GCs (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 6 months ago by dvyukov
Modified:
6 years, 6 months ago
Reviewers:
rsc
CC:
golang-codereviews, bradfitz, r, rsc, khr
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r 66a91d217fcb52c7011fc8103c7be748e9ab1486 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 3 : diff -r 66a91d217fcb52c7011fc8103c7be748e9ab1486 https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 4

Patch Set 4 : diff -r 66a91d217fcb52c7011fc8103c7be748e9ab1486 https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 5

Patch Set 5 : diff -r 66a91d217fcb52c7011fc8103c7be748e9ab1486 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 6 : diff -r 366de8a6a8cf77f3ae3a0ac97860afd9dbc816a9 https://dvyukov%40google.com@code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -22 lines) Patch
M src/sync/pool.go View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/sync/pool_test.go View 1 2 3 4 1 chunk +34 lines, -22 lines 0 comments Download

Messages

Total messages: 15
dvyukov
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/
6 years, 6 months ago (2014-10-22 09:22:15 UTC) #1
bradfitz
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 ...
6 years, 6 months ago (2014-10-22 10:36:54 UTC) #2
dvyukov
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 ...
6 years, 6 months ago (2014-10-22 13:05:49 UTC) #3
r
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#newcode99 src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) { this is likely to be ...
6 years, 6 months ago (2014-10-22 13:11:18 UTC) #4
dvyukov
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#newcode99 src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) { On 2014/10/22 13:11:18, r wrote: ...
6 years, 6 months ago (2014-10-22 13:47:05 UTC) #5
rsc
Can you please explain what memory is being retained during GC incorrectly? The bug says ...
6 years, 6 months ago (2014-10-22 13:50:25 UTC) #6
dvyukov
On 2014/10/22 13:50:25, rsc wrote: > Can you please explain what memory is being retained ...
6 years, 6 months ago (2014-10-22 14:21:34 UTC) #7
rsc
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#newcode99 src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) { Why are there two tests ...
6 years, 6 months ago (2014-10-22 14:42:01 UTC) #8
rsc
On Wed, Oct 22, 2014 at 10:21 AM, <dvyukov@google.com> wrote: > The problem is that ...
6 years, 6 months ago (2014-10-22 14:51:11 UTC) #9
dvyukov
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#newcode99 src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) { On 2014/10/22 14:42:01, rsc wrote: ...
6 years, 6 months ago (2014-10-22 14:57:10 UTC) #10
dvyukov
On 2014/10/22 14:51:11, rsc wrote: > On Wed, Oct 22, 2014 at 10:21 AM, <mailto:dvyukov@google.com> ...
6 years, 6 months ago (2014-10-22 14:58:50 UTC) #11
rsc
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#newcode99 src/sync/pool_test.go:99: func TestPoolRelease(t *testing.T) { Can you pull the body ...
6 years, 6 months ago (2014-10-22 15:14:32 UTC) #12
dvyukov
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#newcode99 > ...
6 years, 6 months ago (2014-10-22 16:05:26 UTC) #13
rsc
LGTM
6 years, 6 months ago (2014-10-22 16:17:35 UTC) #14
dvyukov
6 years, 6 months ago (2014-10-22 16:23:57 UTC) #15
*** 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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b