code review 41860043: sync: add Pool type

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

[bradfitz, 2013-12-13 12:31:17]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-12-13 15:10:22]

This makes fmt ~20% slower, but....

Looking at profiles, it seems I could remove the Pool's internal Mutex and
just protect its state with the runtime.mheap mutex which I'm currently
also grabbing anyway to read numgc..  (using two unexported runtime
functions: one to lock + return numgc, and one to unlock)  But I figured
that would offend people.  So this is the simple version that's slightly
slower for some code (like fmt) but Dmitry will make faster later, once the
API+docs are in.

If people prefer, I can easily do the grosser, speed-parity 1-mutex version
for this first round.

Note that fmt's malloc tests still pass.  (an earlier unmailed version of
this CL initially failed them)



On Fri, Dec 13, 2013 at 4:31 AM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> 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
>
> Please review this at https://codereview.appspot.com/41860043/
>
> Affected files (+162, -39 lines):
>   M src/pkg/fmt/print.go
>   M src/pkg/fmt/scan.go
>   M src/pkg/runtime/mgc0.c
>   M src/pkg/sync/export_test.go
>   A src/pkg/sync/pool.go
>   A src/pkg/sync/pool_test.go
>
>
>

[bradfitz, 2013-12-13 20:41:55]

R=rsc,dvyukov
 On Dec 13, 2013 4:31 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> 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
>
> Please review this at https://codereview.appspot.com/41860043/
>
> Affected files (+162, -39 lines):
>   M src/pkg/fmt/print.go
>   M src/pkg/fmt/scan.go
>   M src/pkg/runtime/mgc0.c
>   M src/pkg/sync/export_test.go
>   A src/pkg/sync/pool.go
>   A src/pkg/sync/pool_test.go
>
>
>

[rsc, 2013-12-13 21:26:06]

NOT LGTM

Seriously? XORing pointers to hide them from the garbage collection? No.
And anything grosser is out too. Especially since this is the "slow"
version.

My proposal included a very simple implementation sketch. Why not use that?

type Pool struct {
    next *Pool // for use by runtime
    list []interface{} // offset known to runtime
    mu sync.Mutex
}

func (p *Pool) Get() interface{} {
    p.mu.Lock()
    var x interface{}
    if n := len(p.list); n > 0 {
        x = p.list[n-1]
        p.list = p.list[:n-1]
    }
    p.mu.Unlock()
    return x
}

func runtime_registerPool(*Pool)

func (p *Pool) Put(x interface{}) {
    p.mu.Lock()
    p.list = append(p.list, x)
    if len(p.list) == 1 {
        runtime_registerPool(p)
    }
    p.mu.Unlock()
}

in runtime:

struct {
    Lock;
    void *head;
} pools;

void
sync.runtime_registerPool(void **p)
{
    runtime.lock(&pools);
    p[0] = pools.head;
    pools.head = p;
    runtime.unlock(&pools);
}

void
clearpools(void)
{
    void **p, **next;

    for(p=pools.head; p != nil; p = next) {
        next = p[0];
        p[0] = nil; // next
        p[1] = nil; // slice
        p[2] = nil;
        p[3] = nil;
    }
}

and call clearpools during gc after stoptheworld.

This all assumes we want the API, which I'm still not sure of. But at least
we should be using clean code.

Russ

[bradfitz, 2013-12-14 02:03:13]

On Sat, Dec 14, 2013 at 1:26 AM, Russ Cox <rsc@golang.org> wrote:

> NOT LGTM
>
> Seriously? XORing pointers to hide them from the garbage collection? No.
>

Hah, right?  :-)  I considered 1 of 3 CLs to send, but I didn't like the
odds for any of them so I went for gross shock value.  Seems like it
worked.  (sorry)  It was also a little side experiment (incomplete) to see
whether it could be done without the runtime's help, and how quickly.


> This all assumes we want the API, which I'm still not sure of. But at
> least we should be using clean code.
>

Dmitry wanted me to get the API in and agreed upon before he worked on
resurrecting one of his P-local patches.

What will it take to conclude this "Thinking" stage of the bug?  Reduction
of code? (removing all those buffered chans)  Performance?  Pretty
implementation?  User star count?  If there's new thoughts, I've haven't
heard them.

I really want to see this get in for Go 1.3 (or at least be enlightened why
it should not) but I don't know how to make progress.

What do you want to see?

[cshapiro1, 2013-12-14 02:10:16]

On Fri, Dec 13, 2013 at 6:03 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> What do you want to see?


How about a proper cache datatype that provides a way for the user to
specify an eviction policy that does not involve the garbage collector?

[bradfitz, 2013-12-14 02:22:27]

I think you're the only one arguing for knobs.

Why can't the system do it automatically or learn over time?
 On Dec 14, 2013 6:10 AM, "Carl Shapiro" <cshapiro@google.com> wrote:

>
> On Fri, Dec 13, 2013 at 6:03 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
>> What do you want to see?
>
>
> How about a proper cache datatype that provides a way for the user to
> specify an eviction policy that does not involve the garbage collector?
>
>

[rsc, 2013-12-14 02:29:49]

Concentrating on P-local anything is a mistake. The P's are not the
problem. The problem is getting the garbage collection semantics right. The
code I sent is at least simple and clear.

Russ

[cshapiro1, 2013-12-14 02:30:51]

On Fri, Dec 13, 2013 at 6:22 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> I think you're the only one arguing for knobs.
>
This seems like a strawman to me.  You are creating a cache.  You have a
set of assumptions about why you should cache an object rather than
re-materialize it.

>  Why can't the system do it automatically or learn over time?
>
And just what signals is it going to learn from, exactly?  How do these
signals correlate with the activity of the garbage collector?

It looks to me like you are just passing the buck onto the garbage
collector.

[rsc, 2013-12-14 02:36:33]

On Fri, Dec 13, 2013 at 9:30 PM, Carl Shapiro <cshapiro@google.com> wrote:

>
> On Fri, Dec 13, 2013 at 6:22 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
>> I think you're the only one arguing for knobs.
>>
> This seems like a strawman to me.  You are creating a cache.  You have a
> set of assumptions about why you should cache an object rather than
> re-materialize it.
>
>>  Why can't the system do it automatically or learn over time?
>>
> And just what signals is it going to learn from, exactly?  How do these
> signals correlate with the activity of the garbage collector?
>
> It looks to me like you are just passing the buck onto the garbage
> collector.
>

At least the garbage collector has some inside information. I understand
your objection to the proposed policy - drained cache at full collection -
in light of possible changes to the collector. But until the day comes when
there are partial collections, the proposed policy seems fine. Even more
importantly, it seems fine to define that the caches are drained by the
system without saying exactly how. Let the system do something sensible
instead of "passing the buck" to all programmers. When the collector
changes, the policy can change too, as long as it is system-defined and not
programmer-defined.

I really don't understand the motivation to let the user control the
eviction policy. To me it sounds the same as letting the user call free.
Not wanting to do that is the whole reason we have a runtime, including a
garbage collector.

Russ

[cshapiro1, 2013-12-14 02:49:00]

On Fri, Dec 13, 2013 at 6:36 PM, Russ Cox <rsc@golang.org> wrote:

> I really don't understand the motivation to let the user control the
> eviction policy. To me it sounds the same as letting the user call free.
> Not wanting to do that is the whole reason we have a runtime, including a
> garbage collector.


You would be hard pressed to show me a system that uses a garbage collector
to implement a time or space efficient cache eviction policy.  Why is Go an
exception?

The garbage collector does not know the cost of re-materializing objects in
the cache.  The user does.  The garbage collector does not know how many
bytes of memory are retained by a pointer in the cache.  The user does.

Without knowing these things, you cannot make a sensible retain versus
release decision.  If an object, say, was read from NFS, it would be better
to grow the heap than evict the object from cache.  You can extrapolate
down to smaller sized objects too.  How is the user supposed to reason
about this?

[iant, 2013-12-14 02:55:52]

On Fri, Dec 13, 2013 at 6:48 PM, Carl Shapiro <cshapiro@google.com> wrote:
>
> Without knowing these things, you cannot make a sensible retain versus
> release decision.  If an object, say, was read from NFS, it would be better
> to grow the heap than evict the object from cache.  You can extrapolate down
> to smaller sized objects too.  How is the user supposed to reason about
> this?

A case like that would not use this data structure.

Clearly some caches need tuning.  That doesn't mean that we shouldn't
also have a simple one.

Ian

[rsc, 2013-12-14 03:12:20]

On Fri, Dec 13, 2013 at 9:48 PM, Carl Shapiro <cshapiro@google.com> wrote:

> The garbage collector does not know the cost of re-materializing objects
> in the cache.  The user does.  The garbage collector does not know how many
> bytes of memory are retained by a pointer in the cache.  The user does.
>

I don't believe we are talking about the same thing at all. As Ian wrote, a
cache in an NFS implementation needs something else entirely. The data
structure we're talking about in this CL is a free list of otherwise-unused
interchangeable objects, not a cache of computed values to be discarded on
demand. That's why Brad and Andrew talked me out of calling it sync.Cache
and insisted on sync.Pool.

Russ

[bradfitz, 2013-12-14 04:05:15]

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

Please take another look.

[bradfitz, 2013-12-14 04:39:16]

Fmt performance is back to normal now.
PTAL
 On Dec 14, 2013 8:05 AM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, rsc@golang.org, cshapiro@google.com,
> iant@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> https://codereview.appspot.com/41860043/
>

[cshapiro1, 2013-12-16 21:09:14]

On Fri, Dec 13, 2013 at 6:55 PM, Ian Lance Taylor <iant@golang.org> wrote:

> On Fri, Dec 13, 2013 at 6:48 PM, Carl Shapiro <cshapiro@google.com> wrote:
> >
> > Without knowing these things, you cannot make a sensible retain versus
> > release decision.  If an object, say, was read from NFS, it would be
> better
> > to grow the heap than evict the object from cache.  You can extrapolate
> down
> > to smaller sized objects too.  How is the user supposed to reason about
> > this?
>
> A case like that would not use this data structure.
>

What would use this data structure?


> Clearly some caches need tuning.  That doesn't mean that we shouldn't
> also have a simple one.


There are no tuning parameters in this data structure.  Everything is
delegated to the garbage collector.

[cshapiro1, 2013-12-16 21:14:58]

On Fri, Dec 13, 2013 at 7:12 PM, Russ Cox <rsc@golang.org> wrote:

> I don't believe we are talking about the same thing at all. As Ian wrote,
> a cache in an NFS implementation needs something else entirely. The data
> structure we're talking about in this CL is a free list of otherwise-unused
> interchangeable objects, not a cache of computed values to be discarded on
> demand. That's why Brad and Andrew talked me out of calling it sync.Cache
> and insisted on sync.Pool.


Whatever you call this, the garbage collector is being used as an eviction
signal.  You did not follow up to my question about whether there is a
system which has successfully implemented such an eviction signal.  Do you
know of such a system?  I do not.  As far as I know many have tried and
none have succeeded.  Why is go different?

[iant, 2013-12-16 21:32:54]

On Mon, Dec 16, 2013 at 1:14 PM, Carl Shapiro <cshapiro@google.com> wrote:
>
> On Fri, Dec 13, 2013 at 7:12 PM, Russ Cox <rsc@golang.org> wrote:
>>
>> I don't believe we are talking about the same thing at all. As Ian wrote,
>> a cache in an NFS implementation needs something else entirely. The data
>> structure we're talking about in this CL is a free list of otherwise-unused
>> interchangeable objects, not a cache of computed values to be discarded on
>> demand. That's why Brad and Andrew talked me out of calling it sync.Cache
>> and insisted on sync.Pool.
>
>
> Whatever you call this, the garbage collector is being used as an eviction
> signal.  You did not follow up to my question about whether there is a
> system which has successfully implemented such an eviction signal.  Do you
> know of such a system?  I do not.  As far as I know many have tried and none
> have succeeded.  Why is go different?

It seems to me that systems that build a cache using weak pointers
will behave much like this proposal.  I could well be missing
something, but what?

Ian

[rsc, 2013-12-16 22:08:48]

On Mon, Dec 16, 2013 at 4:14 PM, Carl Shapiro <cshapiro@google.com> wrote:

> On Fri, Dec 13, 2013 at 7:12 PM, Russ Cox <rsc@golang.org> wrote:
>
>> I don't believe we are talking about the same thing at all. As Ian wrote,
>> a cache in an NFS implementation needs something else entirely. The data
>> structure we're talking about in this CL is a free list of otherwise-unused
>> interchangeable objects, not a cache of computed values to be discarded on
>> demand. That's why Brad and Andrew talked me out of calling it sync.Cache
>> and insisted on sync.Pool.
>
>
> Whatever you call this, the garbage collector is being used as an eviction
> signal.  You did not follow up to my question about whether there is a
> system which has successfully implemented such an eviction signal.  Do you
> know of such a system?  I do not.  As far as I know many have tried and
> none have succeeded.  Why is go different?
>

I answered your question about eviction signals by pointing out that this
is not a cache. It is a free list. "Eviction" is for caches. A cache holds
a set of non-interchangeable objects, each of which represents a different
computation or effort that might be repeated if an eviction is necessary.
In a cache, it is important that the eviction policy predict the future
needs as well as possible so that the impact of an eviction is put off for
as long as possible. A free list holds a set of interchangeable,
indistinguishable pieces of memory, each of which is just sitting waiting
to be used for bypassing a memory allocation and holds no other value.
"Eviction" is not something that makes sense for free lists.

As I explained in the other thread, this is the picture I see:

The benefit of a free list is to let the programmer arrange memory reuse
without waiting for a garbage collection. The costs are (1) to preserve
type safety, the reuse must be for an object of the same type as the
original allocation, and (2) keeping allocations in a free list makes it
impossible for the garbage collector to reclaim the memory for use as some
other type, which would ordinary happen.

That is, putting something on a free list enables limited reuse now, before
the next collection, but it inhibits more general reuse (as another type)
after the next collection. The way to resolve this is to empty the free
list at the time of collection, so that the collector then sees the memory
formerly on the free list as unreferenced and makes it available for new
allocations, whether as the original type or a new one.

On that other thread, you objected that the solution becomes less clear
when there are partial collections and such. I understand that, but I
observe that there are no partial collections today nor are there concrete
plans to introduce them. The objection is thus irrelevant to the current
implementation. If a future implementation adds partial collections, we
will have to revise the policy, but that is fine. The policy is internal to
the implementation. (Looking ahead, as Ian points out, the policy being
proposed can be implemented using weak pointers, although it does not
require them. Therefore it seems to me that we can at least take
inspiration about the semantics of the policy in a partial collector from
the semantics of the policies used by weak pointers.)

I am unaware of systems that have built free lists that work this way - not
caches, but free lists. I would certainly be interested to learn about them
and from their experiences. If you know of any that have tried, please
point them out.

Thanks.
Russ

[cshapiro1, 2013-12-16 22:45:05]

On Mon, Dec 16, 2013 at 1:32 PM, Ian Lance Taylor <iant@golang.org> wrote:

> It seems to me that systems that build a cache using weak pointers
> will behave much like this proposal.  I could well be missing
> something, but what?
>

What systems are these?  How well is it working for them?  I could be
wrong, but I suspect the answer is "not very well".  Weak pointers are not
designed to be the basis for a cache.

Many people, with very good intentions, try to write caches using some form
of weak pointer.  However, the behavior of the cache is very unstable as
the behavior of the garbage collector is inherently unpredictable.  This is
not what people expect from a system-provided facility.  I can show you
systems in production at Google that have this problem.  One went
non-linear just a few days ago.

If we are trying to do an end run around slow allocation performance, there
are better approaches that are easier for users to understand.  For
example, the benefit of caching an object for re-use versus re-allocating
the object can be quantified in terms of a time savings.  The decision to
retain versus reallocate can be made by evaluating the periodic demands on
the cache.  Timers and hints from the user about the size of the data
pointed are sufficient to implement such a system.  There is no signal for
the garbage collector needed.

[bradfitz, 2013-12-17 22:50:40]

Ping.

I'd like to start using this and removing all the grubby buffered channels.



On Fri, Dec 13, 2013 at 8:05 PM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, rsc@golang.org, cshapiro@google.com,
> iant@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> https://codereview.appspot.com/41860043/
>

[iant, 2013-12-17 23:21:17]

It's convenient to have the fmt code to see what it looks like, but probably the
changes to fmt should be in a separate CL to follow this one.

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#newc...
src/pkg/fmt/print.go:127: var ppFree = &sync.Pool{
Seems that this could be sync.Pool, without the &.

https://codereview.appspot.com/41860043/diff/110001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

https://codereview.appspot.com/41860043/diff/110001/src/pkg/fmt/scan.go#newco...
src/pkg/fmt/scan.go:384: var ssFree = &sync.Pool{
s/&// ?

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

https://codereview.appspot.com/41860043/diff/110001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:50: void sync·runtime_registerPool(void **p)
Newline after "void".

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

https://codereview.appspot.com/41860043/diff/110001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:9: // The Pool is drained automatically by the runtime.
We don't want to say anything about GC, but I think we can be slightly clearer. 
How about "The runtime may remove elements from the pool at its own discretion."

https://codereview.appspot.com/41860043/diff/110001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:21: func runtime_registerPool(*Pool)
I think we can write a type safe API here.  We don't have to have the runtime
really free the pools.  What if we write something along the lines of

func init() {
    runtime_registerClean(clean)
}

func clean() {
    poolsLock.Lock()
    for p := pools; p != nil; p = p.next {
        p.mu.Lock()
        p.list = nil
        p.mu.Unlock()
    }
    poolsLock.Unlock()
}

Alternatively, move more of the code, including the object handling, into the
runtime package.

[bradfitz, 2013-12-17 23:38:44]

All but the last comment seem fine.

The code is as I believe Russ asked for. I'm not inclined to change it back
and forth until there's some authoritative decision.
On Dec 17, 2013 3:21 PM, <iant@golang.org> wrote:

> It's convenient to have the fmt code to see what it looks like, but
> probably the changes to fmt should be in a separate CL to follow this
> one.
>
>
> 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{
> Seems that this could be sync.Pool, without the &.
>
> https://codereview.appspot.com/41860043/diff/110001/src/pkg/fmt/scan.go
> File src/pkg/fmt/scan.go (right):
>
> https://codereview.appspot.com/41860043/diff/110001/src/
> pkg/fmt/scan.go#newcode384
> src/pkg/fmt/scan.go:384: var ssFree = &sync.Pool{
> s/&// ?
>
> https://codereview.appspot.com/41860043/diff/110001/src/pkg/runtime/mgc0.c
> File src/pkg/runtime/mgc0.c (right):
>
> https://codereview.appspot.com/41860043/diff/110001/src/
> pkg/runtime/mgc0.c#newcode50
> src/pkg/runtime/mgc0.c:50: void sync·runtime_registerPool(void **p)
> Newline after "void".
>
> https://codereview.appspot.com/41860043/diff/110001/src/pkg/sync/pool.go
> File src/pkg/sync/pool.go (right):
>
> https://codereview.appspot.com/41860043/diff/110001/src/
> pkg/sync/pool.go#newcode9
> src/pkg/sync/pool.go:9: // The Pool is drained automatically by the
> runtime.
> We don't want to say anything about GC, but I think we can be slightly
> clearer.  How about "The runtime may remove elements from the pool at
> its own discretion."
>
> https://codereview.appspot.com/41860043/diff/110001/src/
> pkg/sync/pool.go#newcode21
> src/pkg/sync/pool.go:21: func runtime_registerPool(*Pool)
> I think we can write a type safe API here.  We don't have to have the
> runtime really free the pools.  What if we write something along the
> lines of
>
> func init() {
>     runtime_registerClean(clean)
> }
>
> func clean() {
>     poolsLock.Lock()
>     for p := pools; p != nil; p = p.next {
>         p.mu.Lock()
>         p.list = nil
>         p.mu.Unlock()
>     }
>     poolsLock.Unlock()
> }
>
> Alternatively, move more of the code, including the object handling,
> into the runtime package.
>
> https://codereview.appspot.com/41860043/
>

[bradfitz, 2013-12-17 23:41:28]

Actually the code can leak registrations in the runtime as-is. Will fix.

We should only register with runtime on the transition from nil to non-nil
freelist, then clear the runtime's registration list on GC.
 On Dec 17, 2013 3:38 PM, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:

> All but the last comment seem fine.
>
> The code is as I believe Russ asked for. I'm not inclined to change it
> back and forth until there's some authoritative decision.
> On Dec 17, 2013 3:21 PM, <iant@golang.org> wrote:
>
>> It's convenient to have the fmt code to see what it looks like, but
>> probably the changes to fmt should be in a separate CL to follow this
>> one.
>>
>>
>> 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{
>> Seems that this could be sync.Pool, without the &.
>>
>> https://codereview.appspot.com/41860043/diff/110001/src/pkg/fmt/scan.go
>> File src/pkg/fmt/scan.go (right):
>>
>> https://codereview.appspot.com/41860043/diff/110001/src/
>> pkg/fmt/scan.go#newcode384
>> src/pkg/fmt/scan.go:384: var ssFree = &sync.Pool{
>> s/&// ?
>>
>> https://codereview.appspot.com/41860043/diff/110001/src/
>> pkg/runtime/mgc0.c
>> File src/pkg/runtime/mgc0.c (right):
>>
>> https://codereview.appspot.com/41860043/diff/110001/src/
>> pkg/runtime/mgc0.c#newcode50
>> src/pkg/runtime/mgc0.c:50: void sync·runtime_registerPool(void **p)
>> Newline after "void".
>>
>> https://codereview.appspot.com/41860043/diff/110001/src/pkg/sync/pool.go
>> File src/pkg/sync/pool.go (right):
>>
>> https://codereview.appspot.com/41860043/diff/110001/src/
>> pkg/sync/pool.go#newcode9
>> src/pkg/sync/pool.go:9: // The Pool is drained automatically by the
>> runtime.
>> We don't want to say anything about GC, but I think we can be slightly
>> clearer.  How about "The runtime may remove elements from the pool at
>> its own discretion."
>>
>> https://codereview.appspot.com/41860043/diff/110001/src/
>> pkg/sync/pool.go#newcode21
>> src/pkg/sync/pool.go:21: func runtime_registerPool(*Pool)
>> I think we can write a type safe API here.  We don't have to have the
>> runtime really free the pools.  What if we write something along the
>> lines of
>>
>> func init() {
>>     runtime_registerClean(clean)
>> }
>>
>> func clean() {
>>     poolsLock.Lock()
>>     for p := pools; p != nil; p = p.next {
>>         p.mu.Lock()
>>         p.list = nil
>>         p.mu.Unlock()
>>     }
>>     poolsLock.Unlock()
>> }
>>
>> Alternatively, move more of the code, including the object handling,
>> into the runtime package.
>>
>> https://codereview.appspot.com/41860043/
>>
>

[bradfitz, 2013-12-18 00:42:10]

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

Please take another look.

[bradfitz, 2013-12-18 00:42:23]

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#newc...
src/pkg/fmt/print.go:127: var ppFree = &sync.Pool{
On 2013/12/17 23:21:18, iant wrote:
> Seems that this could be sync.Pool, without the &.

Done.

https://codereview.appspot.com/41860043/diff/110001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

https://codereview.appspot.com/41860043/diff/110001/src/pkg/fmt/scan.go#newco...
src/pkg/fmt/scan.go:384: var ssFree = &sync.Pool{
On 2013/12/17 23:21:18, iant wrote:
> s/&// ?

Done.

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

https://codereview.appspot.com/41860043/diff/110001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:50: void sync·runtime_registerPool(void **p)
On 2013/12/17 23:21:18, iant wrote:
> Newline after "void".

Done.

https://codereview.appspot.com/41860043/diff/110001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:69: }
changed this to zero out pools.head too

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

https://codereview.appspot.com/41860043/diff/110001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:9: // The Pool is drained automatically by the runtime.
On 2013/12/17 23:21:18, iant wrote:
> We don't want to say anything about GC, but I think we can be slightly
clearer. 
> How about "The runtime may remove elements from the pool at its own
discretion."

Done.

https://codereview.appspot.com/41860043/diff/110001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:14: reg  bool
removed this

https://codereview.appspot.com/41860043/diff/110001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:21: func runtime_registerPool(*Pool)
On 2013/12/17 23:21:18, iant wrote:
> I think we can write a type safe API here.  We don't have to have the runtime
> really free the pools.  What if we write something along the lines of
> 
> func init() {
>     runtime_registerClean(clean)
> }
> 
> func clean() {
>     poolsLock.Lock()
>     for p := pools; p != nil; p = p.next {
>         p.mu.Lock()
>         p.list = nil
>         p.mu.Unlock()
>     }
>     poolsLock.Unlock()
> }
> 
> Alternatively, move more of the code, including the object handling, into the
> runtime package.

Waiting on this.

https://codereview.appspot.com/41860043/diff/110001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:27: if !p.reg {
now registering the pool instead on the transition from a nil to non-nil slice

[rsc, 2013-12-18 02:01:29]

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...
src/pkg/sync/pool_test.go:20: if g := p.Get(); g != "b" {
i think you must accept nil here too, or else garbage collection will make this
test flaky.

[bradfitz, 2013-12-18 02:15:41]

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

Please take another look.

[bradfitz, 2013-12-18 02:15:49]

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...
src/pkg/sync/pool_test.go:20: if g := p.Get(); g != "b" {
On 2013/12/18 02:01:30, rsc wrote:
> i think you must accept nil here too, or else garbage collection will make
this
> test flaky.

Fixed with: 	defer debug.SetGCPercent(debug.SetGCPercent(-1))

[r, 2013-12-18 05:12:15]

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#newc...
src/pkg/sync/pool.go:10: // at its own discretion.
// A Pool is a set of temporary objects that may be individually saved and
retrieved.
// Any item stored in the Pool may be removed automatically by the
// implementation at any time without notification.
// If the Pool holds the only reference when this happens, the item will be
// deallocated.
// A Pool is safe for use by multiple goroutines simultaneously.

https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:33: // Get removes and returns a value from the pool.
// Get selects an arbitrary item from the Pool, removes it from the Pool, and
returns it to the caller.

https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:34: // If the pool is empty, Get returns nil, or the return
If the Pool is empty or the item is nil, ...

https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:40: x = p.list[n-1]
p.list[n-1] = nil  // Just to be safe

[bradfitz, 2013-12-18 05:33:18]

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#newc...
src/pkg/sync/pool.go:10: // at its own discretion.
On 2013/12/18 05:12:16, r wrote:
> // A Pool is a set of temporary objects that may be individually saved and
> retrieved.
> // Any item stored in the Pool may be removed automatically by the
> // implementation at any time without notification.
> // If the Pool holds the only reference when this happens, the item will be
> // deallocated.
> // A Pool is safe for use by multiple goroutines simultaneously.

Done.

https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:33: // Get removes and returns a value from the pool.
On 2013/12/18 05:12:16, r wrote:
> // Get selects an arbitrary item from the Pool, removes it from the Pool, and
> returns it to the caller.

Done.

https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:34: // If the pool is empty, Get returns nil, or the return
On 2013/12/18 05:12:16, r wrote:
> If the Pool is empty or the item is nil, ...

Done.

https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:40: x = p.list[n-1]
On 2013/12/18 05:12:16, r wrote:
> p.list[n-1] = nil  // Just to be safe

Done.

[bradfitz, 2013-12-18 05:36:45]

Although,

> If the Pool holds the only reference when this happens, the item will be
> deallocated.

Not necessarily.  The Pool could even choose to not store it but not GC.  I
think you're implying too much of a promise there (that it's either pooled
or collected).  We should promise nothing.

Also, it's only stated indirectly that Put could choose not to do a put.
 (But "may be removed automatically by the implementation at any time
without notification" also means "immediately after a call to Put").




On Wed, Dec 18, 2013 at 9:33 AM, <bradfitz@golang.org> wrote:

> 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, r wrote:
>
>> // A Pool is a set of temporary objects that may be individually saved
>>
> and
>
>> retrieved.
>> // Any item stored in the Pool may be removed automatically by the
>> // implementation at any time without notification.
>> // If the Pool holds the only reference when this happens, the item
>>
> will be
>
>> // deallocated.
>> // A Pool is safe for use by multiple goroutines simultaneously.
>>
>
> Done.
>
>
> https://codereview.appspot.com/41860043/diff/150001/src/
> pkg/sync/pool.go#newcode33
> src/pkg/sync/pool.go:33: // Get removes and returns a value from the
> pool.
> On 2013/12/18 05:12:16, r wrote:
>
>> // Get selects an arbitrary item from the Pool, removes it from the
>>
> Pool, and
>
>> returns it to the caller.
>>
>
> Done.
>
>
> https://codereview.appspot.com/41860043/diff/150001/src/
> pkg/sync/pool.go#newcode34
> src/pkg/sync/pool.go:34: // If the pool is empty, Get returns nil, or
> the return
> On 2013/12/18 05:12:16, r wrote:
>
>> If the Pool is empty or the item is nil, ...
>>
>
> Done.
>
>
> https://codereview.appspot.com/41860043/diff/150001/src/
> pkg/sync/pool.go#newcode40
> src/pkg/sync/pool.go:40: x = p.list[n-1]
> On 2013/12/18 05:12:16, r wrote:
>
>> p.list[n-1] = nil  // Just to be safe
>>
>
> Done.
>
> https://codereview.appspot.com/41860043/
>

[r, 2013-12-18 06:50:11]

s/will be deallocated/might be deallocated/ ? or just delete the
sentence i suppose.

trying to get the right promises is important


On Tue, Dec 17, 2013 at 9:36 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Although,
>
>> If the Pool holds the only reference when this happens, the item will be
>> deallocated.
>
> Not necessarily.  The Pool could even choose to not store it but not GC.  I
> think you're implying too much of a promise there (that it's either pooled
> or collected).  We should promise nothing.
>
> Also, it's only stated indirectly that Put could choose not to do a put.
> (But "may be removed automatically by the implementation at any time without
> notification" also means "immediately after a call to Put").
>
>
>
>
> On Wed, Dec 18, 2013 at 9:33 AM, <bradfitz@golang.org> wrote:
>>
>> 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#newc...
>> src/pkg/sync/pool.go:10: // at its own discretion.
>> On 2013/12/18 05:12:16, r wrote:
>>>
>>> // A Pool is a set of temporary objects that may be individually saved
>>
>> and
>>>
>>> retrieved.
>>> // Any item stored in the Pool may be removed automatically by the
>>> // implementation at any time without notification.
>>> // If the Pool holds the only reference when this happens, the item
>>
>> will be
>>>
>>> // deallocated.
>>> // A Pool is safe for use by multiple goroutines simultaneously.
>>
>>
>> Done.
>>
>>
>>
>>
https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newc...
>> src/pkg/sync/pool.go:33: // Get removes and returns a value from the
>> pool.
>> On 2013/12/18 05:12:16, r wrote:
>>>
>>> // Get selects an arbitrary item from the Pool, removes it from the
>>
>> Pool, and
>>>
>>> returns it to the caller.
>>
>>
>> Done.
>>
>>
>>
>>
https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newc...
>> src/pkg/sync/pool.go:34: // If the pool is empty, Get returns nil, or
>> the return
>> On 2013/12/18 05:12:16, r wrote:
>>>
>>> If the Pool is empty or the item is nil, ...
>>
>>
>> Done.
>>
>>
>>
>>
https://codereview.appspot.com/41860043/diff/150001/src/pkg/sync/pool.go#newc...
>> src/pkg/sync/pool.go:40: x = p.list[n-1]
>> On 2013/12/18 05:12:16, r wrote:
>>>
>>> p.list[n-1] = nil  // Just to be safe
>>
>>
>> Done.
>>
>> https://codereview.appspot.com/41860043/
>
>

[dvyukov, 2013-12-18 11:41:14]

What about the previously proposed Put/Take names?
It makes it clear that Take mutates state of the Pool (as opposed to "getter");
avoids the name with bad reputation; Get usually pairs with Set.

[dvyukov, 2013-12-18 11:50:38]

Evicting all cached elements before looks good to me.
Russ correctly noted that evicting anything before GC makes no sense -- the
elements can't be reused for anything else anyway.
We could do a partial eviction before GC, but I think it does not worth it. (1)
Cost of allocating new elements after GC is very low, (2) it may be too
conservative and waste memory.

If/when we have partial GC, we may need to reevaluate the policy. But note that
the elements won't be collected by both generation-0 collection (they are
referenced from Pool which is most likely in last-level generation) and by
goroutine-local collection (once again they are referenced by Pool which is most
likely not-goroutine-local). So the best policy may be "evict everything before
full GC" again.

This, of course, won't work for a key-value timed cache. But that's a different
story.

[dvyukov, 2013-12-18 11:53:45]

I am pretty sure that the Pool will need Proc-local caches in the final version.
Eviction policy addresses only the "memory" part of the problem, but not the
"speed" part of the problem. The centralized mutex-protected Pool will be slower
than malloc/GC (both are parallel). So users will have to do the trade-off
decision, and eventually we will see manually partitioned Pools.

[bradfitz, 2013-12-18 12:01:26]

On Wed, Dec 18, 2013 at 3:41 AM, <dvyukov@google.com> wrote:

> What about the previously proposed Put/Take names?
> It makes it clear that Take mutates state of the Pool (as opposed to
> "getter"); avoids the name with bad reputation; Get usually pairs with
> Set.
>

But Pool also has an optional constructor.  Then Take sounds weird, whereas
Get is such a flexibly meaningless word, it always works.

Note that fmt already used the names Get (taking or making) and Put.

I agree Set isn't right... it implies either one value, or one value per
some sort of key.

[dvyukov, 2013-12-18 12:09:37]

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#ne...
src/pkg/runtime/mgc0.c:45: struct {
static struct

https://codereview.appspot.com/41860043/diff/170001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:45: struct {
{ on next line

https://codereview.appspot.com/41860043/diff/170001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:47: void *head;
pin * to type in struct defs

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

https://codereview.appspot.com/41860043/diff/170001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:13: // A Pool is safe for use by multiple goroutines
simultaneously.
New is not safe to mutate concurrently with Get.

https://codereview.appspot.com/41860043/diff/170001/src/pkg/sync/pool_test.go
File src/pkg/sync/pool_test.go (right):

https://codereview.appspot.com/41860043/diff/170001/src/pkg/sync/pool_test.go...
src/pkg/sync/pool_test.go:40: 
Also add the following tests:


 39 func TestCacheNew(t *testing.T) {
  40         i := 0
  41         c := Cache{New: func() interface{} {
  42                 i++
  43                 return i
  44         }}
  45         if v := c.Take(); v == nil || v.(int) != 1 {
  46                 t.Fatalf("expect 1 cache, got %v", v)
  47         }
  48         if v := c.Take(); v == nil || v.(int) != 2 {
  49                 t.Fatalf("expect 2 cache, got %v", v)
  50         }
  51         c.Put(42)
  52         if v := c.Take(); v == nil || v.(int) != 42 {
  53                 t.Fatalf("expect 42, got %v", v)
  54         }
  55         if v := c.Take(); v == nil || v.(int) != 3 {
  56                 t.Fatalf("expect 2 cache, got %v", v)
  57         }
  58 }
  59 
    79 
  80 func TestCacheGC(t *testing.T) {
  81         // Test that Cache does not hold pointers to previously cached
resources
  82         if runtime.GOARCH != "amd64" {
  83                 t.Skip("skipping on non-amd64, relies GC preciseness")
  84         }
  85         var c Cache
  86         var fin uint32
  87         const N = 100
  88         for i := 0; i < N; i++ {
  89                 v := new(int)
  90                 runtime.SetFinalizer(v, func(vv *int) {
  91                         atomic.AddUint32(&fin, 1)
  92                 })
  93                 c.Put(v)
  94         }
  95         for i := 0; i < N; i++ {
  96                 c.Take()
  97         }
  98         for i := 0; i < 5; i++ {
  99                 runtime.GC()
 100                 time.Sleep(time.Millisecond)
 101                 // 1 pointer can remain on stack or elsewhere
 102                 if atomic.LoadUint32(&fin) >= N-1 {
 103                         return
 104                 }
 105         }
 106         t.Fatalf("only %v out of %v resources are finalized",
atomic.LoadUint32(&fin), N)
 107 }
 108 
 109 func TestCacheStress(t *testing.T) {
 110         const P = 10
 111         N := int(1e6)
 112         if testing.Short() {
 113                 N /= 100
 114         }
 115         var c Cache
 116         done := make(chan bool)
 117         for i := 0; i < P; i++ {
 118                 go func() {
 119                         var v interface{} = 0
 120                         for j := 0; j < N; j++ {
 121                                 if v == nil {
 122                                         v = 0
 123                                 }
 124                                 c.Put(v)
 125                                 v = c.Take()
 126                                 if v != nil && v.(int) != 0 {
 127                                         t.Fatalf("expect 0, got %v", v)
 128                                 }
 129                         }
 130                         done <- true
 131                 }()
 132         }
 133         for i := 0; i < P; i++ {
 134                 <-done
 135         }
 136 }

https://codereview.appspot.com/41860043/diff/170001/src/pkg/sync/pool_test.go...
src/pkg/sync/pool_test.go:50: defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(2))
Benchmarks must just create k*GOMAXPROCS goroutines,
you control the exact number with go test -cpu=1,2,3,4

delete BenchmarkPool1 and s/BenchmarkPool2/BenchmarkPool/

https://codereview.appspot.com/41860043/diff/170001/src/pkg/sync/pool_test.go...
src/pkg/sync/pool_test.go:57: for i := 0; i < b.N; i += 2 {
this static load balancing does not work good in practice
some goroutines will finish earlier, then you will benchmark some part of
execution with less goroutines; the result will be meaningless

see e.g. BenchmarkSelectUncontended

[r, 2013-12-18 14:44:27]

Put and Take might be better names for this.

-rob

On Wed, Dec 18, 2013 at 3:41 AM,  <dvyukov@google.com> wrote:
> What about the previously proposed Put/Take names?
> It makes it clear that Take mutates state of the Pool (as opposed to
> "getter"); avoids the name with bad reputation; Get usually pairs with
> Set.
>
>
> https://codereview.appspot.com/41860043/

[r, 2013-12-18 14:45:19]

But Put and Get are probably OK too.

-rob

[rsc, 2013-12-18 17:06:28]

Please let's leave the names the way they were originally - Get and Put.

[bradfitz, 2013-12-18 17:31:15]

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.

[bradfitz, 2013-12-18 17:31:24]

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#ne...
src/pkg/runtime/mgc0.c:45: struct {
On 2013/12/18 12:09:38, dvyukov wrote:
> static struct

Done.

https://codereview.appspot.com/41860043/diff/170001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:45: struct {
On 2013/12/18 12:09:38, dvyukov wrote:
> { on next line

that is not consistent with the rest of this file

https://codereview.appspot.com/41860043/diff/170001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:47: void *head;
On 2013/12/18 12:09:38, dvyukov wrote:
> pin * to type in struct defs

Done.

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

https://codereview.appspot.com/41860043/diff/170001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:13: // A Pool is safe for use by multiple goroutines
simultaneously.
On 2013/12/18 12:09:38, dvyukov wrote:
> New is not safe to mutate concurrently with Get.

Done.

https://codereview.appspot.com/41860043/diff/170001/src/pkg/sync/pool_test.go
File src/pkg/sync/pool_test.go (right):

https://codereview.appspot.com/41860043/diff/170001/src/pkg/sync/pool_test.go...
src/pkg/sync/pool_test.go:40: 
On 2013/12/18 12:09:38, dvyukov wrote:
> Also add the following tests:
> 
> 
>  39 func TestCacheNew(t *testing.T) {
>   40         i := 0
>   41         c := Cache{New: func() interface{} {
>   42                 i++
>   43                 return i
>   44         }}
>   45         if v := c.Take(); v == nil || v.(int) != 1 {
>   46                 t.Fatalf("expect 1 cache, got %v", v)
>   47         }
>   48         if v := c.Take(); v == nil || v.(int) != 2 {
>   49                 t.Fatalf("expect 2 cache, got %v", v)
>   50         }
>   51         c.Put(42)
>   52         if v := c.Take(); v == nil || v.(int) != 42 {
>   53                 t.Fatalf("expect 42, got %v", v)
>   54         }
>   55         if v := c.Take(); v == nil || v.(int) != 3 {
>   56                 t.Fatalf("expect 2 cache, got %v", v)
>   57         }
>   58 }
>   59 
>     79 
>   80 func TestCacheGC(t *testing.T) {
>   81         // Test that Cache does not hold pointers to previously cached
> resources
>   82         if runtime.GOARCH != "amd64" {
>   83                 t.Skip("skipping on non-amd64, relies GC preciseness")
>   84         }
>   85         var c Cache
>   86         var fin uint32
>   87         const N = 100
>   88         for i := 0; i < N; i++ {
>   89                 v := new(int)
>   90                 runtime.SetFinalizer(v, func(vv *int) {
>   91                         atomic.AddUint32(&fin, 1)
>   92                 })
>   93                 c.Put(v)
>   94         }
>   95         for i := 0; i < N; i++ {
>   96                 c.Take()
>   97         }
>   98         for i := 0; i < 5; i++ {
>   99                 runtime.GC()
>  100                 time.Sleep(time.Millisecond)
>  101                 // 1 pointer can remain on stack or elsewhere
>  102                 if atomic.LoadUint32(&fin) >= N-1 {
>  103                         return
>  104                 }
>  105         }
>  106         t.Fatalf("only %v out of %v resources are finalized",
> atomic.LoadUint32(&fin), N)
>  107 }
>  108 
>  109 func TestCacheStress(t *testing.T) {
>  110         const P = 10
>  111         N := int(1e6)
>  112         if testing.Short() {
>  113                 N /= 100
>  114         }
>  115         var c Cache
>  116         done := make(chan bool)
>  117         for i := 0; i < P; i++ {
>  118                 go func() {
>  119                         var v interface{} = 0
>  120                         for j := 0; j < N; j++ {
>  121                                 if v == nil {
>  122                                         v = 0
>  123                                 }
>  124                                 c.Put(v)
>  125                                 v = c.Take()
>  126                                 if v != nil && v.(int) != 0 {
>  127                                         t.Fatalf("expect 0, got %v", v)
>  128                                 }
>  129                         }
>  130                         done <- true
>  131                 }()
>  132         }
>  133         for i := 0; i < P; i++ {
>  134                 <-done
>  135         }
>  136 }

Done.

https://codereview.appspot.com/41860043/diff/170001/src/pkg/sync/pool_test.go...
src/pkg/sync/pool_test.go:57: for i := 0; i < b.N; i += 2 {
On 2013/12/18 12:09:38, dvyukov wrote:
> this static load balancing does not work good in practice
> some goroutines will finish earlier, then you will benchmark some part of
> execution with less goroutines; the result will be meaningless
> 
> see e.g. BenchmarkSelectUncontended

Done, I think.  PTAL.

[rsc, 2013-12-18 17:42:52]

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#ne...
src/pkg/runtime/mgc0.c:60: void
static void

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...
src/pkg/sync/pool_test.go:74: t.Skip("skipping on non-amd64, relies on GC
preciseness")
does this really fail on non-amd64?

The only lack of precision we have left is interface values and temporaries
whose addresses are taken but do not escape. I do not see any of those here.

[bradfitz, 2013-12-18 17:49:57]

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...
src/pkg/sync/pool_test.go:74: t.Skip("skipping on non-amd64, relies on GC
preciseness")
On 2013/12/18 17:42:53, rsc wrote:
> does this really fail on non-amd64?
> 
> The only lack of precision we have left is interface values and temporaries
> whose addresses are taken but do not escape. I do not see any of those here.

I copied this from Dmitry's suggested code.

I'll remove this and see what happens for now.  We can always add it back if
things are flaky.

[dvyukov, 2013-12-18 17:51:05]

On Wed, Dec 18, 2013 at 9:49 PM,  <bradfitz@golang.org> wrote:
> 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...
> src/pkg/sync/pool_test.go:74: t.Skip("skipping on non-amd64, relies on
> GC preciseness")
> On 2013/12/18 17:42:53, rsc wrote:
>>
>> does this really fail on non-amd64?
>
>
>> The only lack of precision we have left is interface values and
>
> temporaries
>>
>> whose addresses are taken but do not escape. I do not see any of those
>
> here.
>
> I copied this from Dmitry's suggested code.

That code was written long time ago.

> I'll remove this and see what happens for now.  We can always add it
> back if things are flaky.
>
> https://codereview.appspot.com/41860043/

[khr1, 2013-12-18 18:28:24]

In the spec, we may want to allow for the possibility that Get() will
return nil or call New() even when the pool isn't empty.  For an
implementation with a per-P cache, we might rather call New() or force the
caller to alloc instead of searching all the other Ps for the last few
unused objects.


On Wed, Dec 18, 2013 at 9:50 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Wed, Dec 18, 2013 at 9:49 PM,  <bradfitz@golang.org> wrote:
> > 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...
> > src/pkg/sync/pool_test.go:74: t.Skip("skipping on non-amd64, relies on
> > GC preciseness")
> > On 2013/12/18 17:42:53, rsc wrote:
> >>
> >> does this really fail on non-amd64?
> >
> >
> >> The only lack of precision we have left is interface values and
> >
> > temporaries
> >>
> >> whose addresses are taken but do not escape. I do not see any of those
> >
> > here.
> >
> > I copied this from Dmitry's suggested code.
>
> That code was written long time ago.
>
> > I'll remove this and see what happens for now.  We can always add it
> > back if things are flaky.
> >
> > https://codereview.appspot.com/41860043/
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[r, 2013-12-18 18:40:06]

Good point.

-rob

[r, 2013-12-18 18:40:11]

Please put all uses of the package into separate CLs.

[bradfitz, 2013-12-18 18:43:49]

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.

[r, 2013-12-18 18:43:53]

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#ne...
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 (right):

https://codereview.appspot.com/41860043/diff/210001/src/pkg/sync/pool.go#newc...
src/pkg/sync/pool.go:44: // If the pool is empty and p.New is not set, Get
returns nil.
// By default, Get returns nil if the pool is empty.
// However, if p.New is non-nil, Get will use it to create values to be returned
// if the pool is empty or under other unspecified conditions.

[bradfitz, 2013-12-18 18:46:43]

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
> 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 (right):
>
> https://codereview.appspot.com/41860043/diff/210001/src/
> pkg/sync/pool.go#newcode44
> src/pkg/sync/pool.go:44: // If the pool is empty and p.New is not set,
> Get returns nil.
> // By default, Get returns nil if the pool is empty.
> // However, if p.New is non-nil, Get will use it to create values to be
> returned
> // if the pool is empty or under other unspecified conditions.
>
> https://codereview.appspot.com/41860043/
>

[r, 2013-12-18 18:57:07]

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#newc...
src/pkg/sync/pool.go:46: // the return values from Get.
Callers should not assume any relation between values passed to Put and the
values returned by Get.

[bradfitz, 2013-12-18 19:08:38]

*** 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