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

Issue 41860043: code review 41860043: sync: add Pool type (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 3 months ago by bradfitz
Modified:
3 years, 3 months ago
Reviewers:
r, rsc, dvyukov
CC:
golang-dev, rsc, cshapiro1, iant, r, dvyukov, khr1
Visibility:
Public.

Description

sync: add Pool type Adds the Pool type and docs, and use it in fmt. This is a temporary implementation, until Dmitry makes it fast. Uses the API proposal from Russ in http://goo.gl/cCKeb2 but adds an optional New field, as used in fmt and elsewhere. Almost all callers want that. Update Issue 4720

Patch Set 1 #

Patch Set 2 : diff -r c37a24ac952c https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r c37a24ac952c https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r c37a24ac952c https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r c37a24ac952c https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r c37a24ac952c https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r c37a24ac952c https://go.googlecode.com/hg/ #

Total comments: 13

Patch Set 8 : diff -r a0c8cab3e171 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 9 : diff -r e4cdf4d18e1e https://go.googlecode.com/hg/ #

Total comments: 8

Patch Set 10 : diff -r e4cdf4d18e1e https://go.googlecode.com/hg/ #

Total comments: 13

Patch Set 11 : diff -r 03a5f6eac06a https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 12 : diff -r 03a5f6eac06a https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 13 : diff -r 477af6a8e51f https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 14 : diff -r 477af6a8e51f https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+252 lines, -1 line) Patch
M src/pkg/runtime/mgc0.c View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +33 lines, -1 line 0 comments Download
A src/pkg/sync/pool.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +65 lines, -0 lines 0 comments Download
A src/pkg/sync/pool_test.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +154 lines, -0 lines 0 comments Download

Messages

Total messages: 54
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
3 years, 3 months ago (2013-12-13 12:31:17 UTC) #1
bradfitz
This makes fmt ~20% slower, but.... Looking at profiles, it seems I could remove the ...
3 years, 3 months ago (2013-12-13 15:10:22 UTC) #2
bradfitz
R=rsc,dvyukov On Dec 13, 2013 4:31 PM, <bradfitz@golang.org> wrote: > Reviewers: golang-dev1, > > Message: ...
3 years, 3 months ago (2013-12-13 20:41:55 UTC) #3
rsc
NOT LGTM Seriously? XORing pointers to hide them from the garbage collection? No. And anything ...
3 years, 3 months ago (2013-12-13 21:26:06 UTC) #4
bradfitz
On Sat, Dec 14, 2013 at 1:26 AM, Russ Cox <rsc@golang.org> wrote: > NOT LGTM ...
3 years, 3 months ago (2013-12-14 02:03:13 UTC) #5
cshapiro1
On Fri, Dec 13, 2013 at 6:03 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > What do you ...
3 years, 3 months ago (2013-12-14 02:10:16 UTC) #6
bradfitz
I think you're the only one arguing for knobs. Why can't the system do it ...
3 years, 3 months ago (2013-12-14 02:22:27 UTC) #7
rsc
Concentrating on P-local anything is a mistake. The P's are not the problem. The problem ...
3 years, 3 months ago (2013-12-14 02:29:49 UTC) #8
cshapiro1
On Fri, Dec 13, 2013 at 6:22 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > I think you're ...
3 years, 3 months ago (2013-12-14 02:30:51 UTC) #9
rsc
On Fri, Dec 13, 2013 at 9:30 PM, Carl Shapiro <cshapiro@google.com> wrote: > > On ...
3 years, 3 months ago (2013-12-14 02:36:33 UTC) #10
cshapiro1
On Fri, Dec 13, 2013 at 6:36 PM, Russ Cox <rsc@golang.org> wrote: > I really ...
3 years, 3 months ago (2013-12-14 02:49:00 UTC) #11
iant
On Fri, Dec 13, 2013 at 6:48 PM, Carl Shapiro <cshapiro@google.com> wrote: > > Without ...
3 years, 3 months ago (2013-12-14 02:55:52 UTC) #12
rsc
On Fri, Dec 13, 2013 at 9:48 PM, Carl Shapiro <cshapiro@google.com> wrote: > The garbage ...
3 years, 3 months ago (2013-12-14 03:12:20 UTC) #13
bradfitz
Hello golang-dev@googlegroups.com, rsc@golang.org, cshapiro@google.com, iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
3 years, 3 months ago (2013-12-14 04:05:15 UTC) #14
bradfitz
Fmt performance is back to normal now. PTAL On Dec 14, 2013 8:05 AM, <bradfitz@golang.org> ...
3 years, 3 months ago (2013-12-14 04:39:16 UTC) #15
cshapiro1
On Fri, Dec 13, 2013 at 6:55 PM, Ian Lance Taylor <iant@golang.org> wrote: > On ...
3 years, 3 months ago (2013-12-16 21:09:14 UTC) #16
cshapiro1
On Fri, Dec 13, 2013 at 7:12 PM, Russ Cox <rsc@golang.org> wrote: > I don't ...
3 years, 3 months ago (2013-12-16 21:14:58 UTC) #17
iant
On Mon, Dec 16, 2013 at 1:14 PM, Carl Shapiro <cshapiro@google.com> wrote: > > On ...
3 years, 3 months ago (2013-12-16 21:32:54 UTC) #18
rsc
On Mon, Dec 16, 2013 at 4:14 PM, Carl Shapiro <cshapiro@google.com> wrote: > On Fri, ...
3 years, 3 months ago (2013-12-16 22:08:48 UTC) #19
cshapiro1
On Mon, Dec 16, 2013 at 1:32 PM, Ian Lance Taylor <iant@golang.org> wrote: > It ...
3 years, 3 months ago (2013-12-16 22:45:05 UTC) #20
bradfitz
Ping. I'd like to start using this and removing all the grubby buffered channels. On ...
3 years, 3 months ago (2013-12-17 22:50:40 UTC) #21
iant
It's convenient to have the fmt code to see what it looks like, but probably ...
3 years, 3 months ago (2013-12-17 23:21:17 UTC) #22
bradfitz
All but the last comment seem fine. The code is as I believe Russ asked ...
3 years, 3 months ago (2013-12-17 23:38:44 UTC) #23
bradfitz
Actually the code can leak registrations in the runtime as-is. Will fix. We should only ...
3 years, 3 months ago (2013-12-17 23:41:28 UTC) #24
bradfitz
Hello golang-dev@googlegroups.com, rsc@golang.org, cshapiro@google.com, iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
3 years, 3 months ago (2013-12-18 00:42:10 UTC) #25
bradfitz
https://codereview.appspot.com/41860043/diff/110001/src/pkg/fmt/print.go File src/pkg/fmt/print.go (right): https://codereview.appspot.com/41860043/diff/110001/src/pkg/fmt/print.go#newcode127 src/pkg/fmt/print.go:127: var ppFree = &sync.Pool{ On 2013/12/17 23:21:18, iant wrote: ...
3 years, 3 months ago (2013-12-18 00:42:23 UTC) #26
rsc
https://codereview.appspot.com/41860043/diff/130001/src/pkg/sync/pool_test.go File src/pkg/sync/pool_test.go (right): https://codereview.appspot.com/41860043/diff/130001/src/pkg/sync/pool_test.go#newcode20 src/pkg/sync/pool_test.go:20: if g := p.Get(); g != "b" { i ...
3 years, 3 months ago (2013-12-18 02:01:29 UTC) #27
bradfitz
Hello golang-dev@googlegroups.com, rsc@golang.org, cshapiro@google.com, iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
3 years, 3 months ago (2013-12-18 02:15:41 UTC) #28
bradfitz
https://codereview.appspot.com/41860043/diff/130001/src/pkg/sync/pool_test.go File src/pkg/sync/pool_test.go (right): https://codereview.appspot.com/41860043/diff/130001/src/pkg/sync/pool_test.go#newcode20 src/pkg/sync/pool_test.go:20: if g := p.Get(); g != "b" { On ...
3 years, 3 months ago (2013-12-18 02:15:49 UTC) #29
r
https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go File src/pkg/sync/pool.go (right): https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newcode10 src/pkg/sync/pool.go:10: // at its own discretion. // A Pool is ...
3 years, 3 months ago (2013-12-18 05:12:15 UTC) #30
bradfitz
PTAL https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go File src/pkg/sync/pool.go (right): https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newcode10 src/pkg/sync/pool.go:10: // at its own discretion. On 2013/12/18 05:12:16, ...
3 years, 3 months ago (2013-12-18 05:33:18 UTC) #31
bradfitz
Although, > If the Pool holds the only reference when this happens, the item will ...
3 years, 3 months ago (2013-12-18 05:36:45 UTC) #32
r
s/will be deallocated/might be deallocated/ ? or just delete the sentence i suppose. trying to ...
3 years, 3 months ago (2013-12-18 06:50:11 UTC) #33
dvyukov
What about the previously proposed Put/Take names? It makes it clear that Take mutates state ...
3 years, 3 months ago (2013-12-18 11:41:14 UTC) #34
dvyukov
Evicting all cached elements before looks good to me. Russ correctly noted that evicting anything ...
3 years, 3 months ago (2013-12-18 11:50:38 UTC) #35
dvyukov
I am pretty sure that the Pool will need Proc-local caches in the final version. ...
3 years, 3 months ago (2013-12-18 11:53:45 UTC) #36
bradfitz
On Wed, Dec 18, 2013 at 3:41 AM, <dvyukov@google.com> wrote: > What about the previously ...
3 years, 3 months ago (2013-12-18 12:01:26 UTC) #37
dvyukov
LGTM https://codereview.appspot.com/41860043/diff/170001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/41860043/diff/170001/src/pkg/runtime/mgc0.c#newcode45 src/pkg/runtime/mgc0.c:45: struct { static struct https://codereview.appspot.com/41860043/diff/170001/src/pkg/runtime/mgc0.c#newcode45 src/pkg/runtime/mgc0.c:45: struct { ...
3 years, 3 months ago (2013-12-18 12:09:37 UTC) #38
r
Put and Take might be better names for this. -rob On Wed, Dec 18, 2013 ...
3 years, 3 months ago (2013-12-18 14:44:27 UTC) #39
r
But Put and Get are probably OK too. -rob
3 years, 3 months ago (2013-12-18 14:45:19 UTC) #40
rsc
Please let's leave the names the way they were originally - Get and Put.
3 years, 3 months ago (2013-12-18 17:06:28 UTC) #41
bradfitz
Hello golang-dev@googlegroups.com, rsc@golang.org, cshapiro@google.com, iant@golang.org, r@golang.org, dvyukov@google.com (cc: golang-dev@googlegroups.com), Please take another look.
3 years, 3 months ago (2013-12-18 17:31:15 UTC) #42
bradfitz
https://codereview.appspot.com/41860043/diff/170001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/41860043/diff/170001/src/pkg/runtime/mgc0.c#newcode45 src/pkg/runtime/mgc0.c:45: struct { On 2013/12/18 12:09:38, dvyukov wrote: > static ...
3 years, 3 months ago (2013-12-18 17:31:24 UTC) #43
rsc
LGTM but wait for r https://codereview.appspot.com/41860043/diff/190001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/41860043/diff/190001/src/pkg/runtime/mgc0.c#newcode60 src/pkg/runtime/mgc0.c:60: void static void https://codereview.appspot.com/41860043/diff/190001/src/pkg/sync/pool_test.go ...
3 years, 3 months ago (2013-12-18 17:42:52 UTC) #44
bradfitz
Done. https://codereview.appspot.com/41860043/diff/190001/src/pkg/sync/pool_test.go File src/pkg/sync/pool_test.go (right): https://codereview.appspot.com/41860043/diff/190001/src/pkg/sync/pool_test.go#newcode74 src/pkg/sync/pool_test.go:74: t.Skip("skipping on non-amd64, relies on GC preciseness") On ...
3 years, 3 months ago (2013-12-18 17:49:57 UTC) #45
dvyukov
On Wed, Dec 18, 2013 at 9:49 PM, <bradfitz@golang.org> wrote: > Done. > > > ...
3 years, 3 months ago (2013-12-18 17:51:05 UTC) #46
khr1
In the spec, we may want to allow for the possibility that Get() will return ...
3 years, 3 months ago (2013-12-18 18:28:24 UTC) #47
r
Good point. -rob
3 years, 3 months ago (2013-12-18 18:40:06 UTC) #48
r
Please put all uses of the package into separate CLs.
3 years, 3 months ago (2013-12-18 18:40:11 UTC) #49
bradfitz
Hello golang-dev@googlegroups.com, rsc@golang.org, cshapiro@google.com, iant@golang.org, r@golang.org, dvyukov@google.com, khr@google.com (cc: golang-dev@googlegroups.com), Please take another look.
3 years, 3 months ago (2013-12-18 18:43:49 UTC) #50
r
https://codereview.appspot.com/41860043/diff/210001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/41860043/diff/210001/src/pkg/runtime/mgc0.c#newcode45 src/pkg/runtime/mgc0.c:45: static struct // Support for sync.Pool. https://codereview.appspot.com/41860043/diff/210001/src/pkg/sync/pool.go File src/pkg/sync/pool.go ...
3 years, 3 months ago (2013-12-18 18:43:53 UTC) #51
bradfitz
PTAL On Wed, Dec 18, 2013 at 10:43 PM, <r@golang.org> wrote: > > https://codereview.appspot.com/41860043/diff/210001/src/pkg/runtime/mgc0.c > ...
3 years, 3 months ago (2013-12-18 18:46:43 UTC) #52
r
LGTM https://codereview.appspot.com/41860043/diff/230001/src/pkg/sync/pool.go File src/pkg/sync/pool.go (right): https://codereview.appspot.com/41860043/diff/230001/src/pkg/sync/pool.go#newcode46 src/pkg/sync/pool.go:46: // the return values from Get. Callers should ...
3 years, 3 months ago (2013-12-18 18:57:07 UTC) #53
bradfitz
3 years, 3 months ago (2013-12-18 19:08:38 UTC) #54
*** Submitted as https://code.google.com/p/go/source/detail?r=2d9fe00d6ce1 ***

sync: add Pool type

Adds the Pool type and docs, and use it in fmt.
This is a temporary implementation, until Dmitry
makes it fast.

Uses the API proposal from Russ in http://goo.gl/cCKeb2 but
adds an optional New field, as used in fmt and elsewhere.
Almost all callers want that.

Update Issue 4720

R=golang-dev, rsc, cshapiro, iant, r, dvyukov, khr
CC=golang-dev
https://codereview.appspot.com/41860043
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted