code review 44050044: encoding/gob: use sync.Pool

encoding/gob: use sync.Pool

Update Issue 4720

[bradfitz, 2013-12-19 00:17:02]

Hello golang-dev@googlegroups.com,

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

[r, 2013-12-19 00:19:54]

Not sure about this one. You're putting a mutex into something that is locked at
the top. Given the way this package is used, that seems misguided.

[r, 2013-12-19 00:21:11]

Never mind, the mutexes will all happen on the same thread.

Still, I don't see this change as much of a win.

[rsc, 2013-12-19 00:25:34]

What is the improvement here? This is a local free list, not a global one.
That is, it gets thrown away with the Decoder, it doesn't live forever
anyway. The sync.Pool seems to be useful because it solves the problem of
when the throw away a global free list. But that's not a problem here.

Russ

[bradfitz, 2013-12-19 00:28:46]

Oh, whoops, I thought this was a longer-lived thing.  I don't use
encoding/gob.

I at least think the new code is nicer, not having to implement the
freelist stack, but I agree the mutex (even if local/uncontended, and
potentially going partially away the future) is unnecessary

I can abandon.



On Wed, Dec 18, 2013 at 4:25 PM, Russ Cox <rsc@golang.org> wrote:

> What is the improvement here? This is a local free list, not a global one.
> That is, it gets thrown away with the Decoder, it doesn't live forever
> anyway. The sync.Pool seems to be useful because it solves the problem of
> when the throw away a global free list. But that's not a problem here.
>
> Russ
>

[dvyukov, 2013-12-19 07:28:15]

Why it is a local cache? To avoid contention? Can it be made global?

[r, 2013-12-19 15:13:57]

To avoid contention. To reduce memory usage. Because it makes sense to
be local. Because it's simple.

The package is designed to support RPC services. Since a server might
have thousands of long-running clients, a per-client cache makes a lot
more sense than a global one.

Please abandon this CL.

-rob

[dvyukov, 2013-12-19 15:19:08]

On 2013/12/19 15:13:57, r wrote:
> To avoid contention. To reduce memory usage. Because it makes sense to
> be local. Because it's simple.
> 
> The package is designed to support RPC services. Since a server might
> have thousands of long-running clients, a per-client cache makes a lot
> more sense than a global one.

I do not understand.
A global cache will reduce memory usage. Lots of local caches will increase
memory usage.
The Pool must not have any contention in final version. It's intended to be used
as global caches.

[r, 2013-12-19 15:23:34]

But it's not the final version yet and it will add contention. The
current code is fine, at least for now.

Please abandon this CL.

-rob

[rsc, 2013-12-19 16:20:58]

not lgtm

sync.Pool is not for short-lived free lists, especially ones that are
already managed inside of another data structure. The free list inside the
Decoder only lives as long as the Decoder does. There is no need for what
sync.Pool contributes.

sync.Pool solves a real problem for global free lists. Since this case is
not a global free list, sync.Pool has very little to contribute here.

By far my largest reservation about sync.Pool was that it would infect all
our code, that people would use it in places where it does not belong, both
for existing free lists that are working fine and to introduce new free
lists that are not necessary. If this happens, we will delete sync.Pool.

Russ

[bradfitz, 2013-12-19 17:22:47]

On Thu, Dec 19, 2013 at 8:20 AM, Russ Cox <rsc@golang.org> wrote:

> not lgtm
>
> sync.Pool is not for short-lived free lists, especially ones that are
> already managed inside of another data structure. The free list inside the
> Decoder only lives as long as the Decoder does. There is no need for what
> sync.Pool contributes.
>
> sync.Pool solves a real problem for global free lists. Since this case is
> not a global free list, sync.Pool has very little to contribute here.
>
> By far my largest reservation about sync.Pool was that it would infect all
> our code, that people would use it in places where it does not belong, both
> for existing free lists that are working fine and to introduce new free
> lists that are not necessary. If this happens, we will delete sync.Pool.
>

Yes, I already closed this yesterday.  I didn't use the hg tool to send
**abandoned** email, though.

As I said in the other thread, this change was a mistake on my part.  I
agree sync.Pool is not good here for the same reasons others have pointed
out.  I thought this code was something else, and I don't intend to
over-use sync.Pool.

[rsc, 2013-12-19 17:58:51]

On Thu, Dec 19, 2013 at 12:22 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> On Thu, Dec 19, 2013 at 8:20 AM, Russ Cox <rsc@golang.org> wrote:
>
>> not lgtm
>>
>> sync.Pool is not for short-lived free lists, especially ones that are
>> already managed inside of another data structure. The free list inside the
>> Decoder only lives as long as the Decoder does. There is no need for what
>> sync.Pool contributes.
>>
>> sync.Pool solves a real problem for global free lists. Since this case is
>> not a global free list, sync.Pool has very little to contribute here.
>>
>> By far my largest reservation about sync.Pool was that it would infect
>> all our code, that people would use it in places where it does not belong,
>> both for existing free lists that are working fine and to introduce new
>> free lists that are not necessary. If this happens, we will delete
>> sync.Pool.
>>
>
> Yes, I already closed this yesterday.  I didn't use the hg tool to send
> **abandoned** email, though.
>

I know you're on the same page. I was replying to Dmitry. Thanks.

Russ

[dvyukov, 2013-12-20 06:24:23]

On 2013/12/19 16:20:58, rsc wrote:
> not lgtm
> 
> sync.Pool is not for short-lived free lists, especially ones that are
> already managed inside of another data structure. The free list inside the
> Decoder only lives as long as the Decoder does. There is no need for what
> sync.Pool contributes.
> 
> sync.Pool solves a real problem for global free lists. Since this case is
> not a global free list, sync.Pool has very little to contribute here.


Are we talking about current non-final non-scalable Pool implementation? Or the
final, scalable one? Or both?

The plan here, as I see it:
1. Make the Pool scalable
2. Replace the local pools in gob with a global one

[rsc, 2013-12-20 17:07:32]

On Fri, Dec 20, 2013 at 1:24 AM, <dvyukov@google.com> wrote:

> On 2013/12/19 16:20:58, rsc wrote:
>
>> not lgtm
>>
>
>  sync.Pool is not for short-lived free lists, especially ones that are
>> already managed inside of another data structure. The free list inside
>>
> the
>
>> Decoder only lives as long as the Decoder does. There is no need for
>>
> what
>
>> sync.Pool contributes.
>>
>
>  sync.Pool solves a real problem for global free lists. Since this case
>>
> is
>
>> not a global free list, sync.Pool has very little to contribute here.
>>
>
>
> Are we talking about current non-final non-scalable Pool implementation?
> Or the final, scalable one? Or both?
>
> The plan here, as I see it:
> 1. Make the Pool scalable
> 2. Replace the local pools in gob with a global one
>

I don't see any need to replace the local free lists with a global one.
That's exactly the overuse I want to avoid, exactly the overuse that will
cause us to take sync.Pool back out of the library.

As I explained above, the most significant feature of sync.Pool is that it
lets the runtime influence when memory is released. That is not necessary
in this case, as I explained above. The memory will be released when the
Decoder is released.

Russ

[dvyukov, 2013-12-23 06:32:37]

On 2013/12/20 17:07:32, rsc wrote:
> On Fri, Dec 20, 2013 at 1:24 AM, <mailto:dvyukov@google.com> wrote:
> 
> > On 2013/12/19 16:20:58, rsc wrote:
> >
> >> not lgtm
> >>
> >
> >  sync.Pool is not for short-lived free lists, especially ones that are
> >> already managed inside of another data structure. The free list inside
> >>
> > the
> >
> >> Decoder only lives as long as the Decoder does. There is no need for
> >>
> > what
> >
> >> sync.Pool contributes.
> >>
> >
> >  sync.Pool solves a real problem for global free lists. Since this case
> >>
> > is
> >
> >> not a global free list, sync.Pool has very little to contribute here.
> >>
> >
> >
> > Are we talking about current non-final non-scalable Pool implementation?
> > Or the final, scalable one? Or both?
> >
> > The plan here, as I see it:
> > 1. Make the Pool scalable
> > 2. Replace the local pools in gob with a global one
> >
> 
> I don't see any need to replace the local free lists with a global one.
> That's exactly the overuse I want to avoid, exactly the overuse that will
> cause us to take sync.Pool back out of the library.
> 
> As I explained above, the most significant feature of sync.Pool is that it
> lets the runtime influence when memory is released. That is not necessary
> in this case, as I explained above. The memory will be released when the
> Decoder is released.


I do not understand why you call it overuse.
If we assume that this code needs pooling, then a global sync.Pool looks like
what doctor said.
The current ad-hoc pooling solution simply does not work in some cases, and in
some cases it works poorly. If you have a small object with some nesting, then
each operation will allocate several objects with no pooling at all. In this
case the ad-hoc pool adds code and slows down execution giving nothing in
return. In some intermediate cases you can have some limited benefit from the
pool, but still significantly less than from a global sync.Pool.
At the same time I do not see any reason to not use sync.Pool.
So why not delete the ad-hoc code and replace it with a better standard
component?

[dvyukov, 2013-12-29 18:16:21]

On 2013/12/23 06:32:37, dvyukov wrote:
> On 2013/12/20 17:07:32, rsc wrote:
> > On Fri, Dec 20, 2013 at 1:24 AM, <mailto:dvyukov@google.com> wrote:
> > 
> > > On 2013/12/19 16:20:58, rsc wrote:
> > >
> > >> not lgtm
> > >>
> > >
> > >  sync.Pool is not for short-lived free lists, especially ones that are
> > >> already managed inside of another data structure. The free list inside
> > >>
> > > the
> > >
> > >> Decoder only lives as long as the Decoder does. There is no need for
> > >>
> > > what
> > >
> > >> sync.Pool contributes.
> > >>
> > >
> > >  sync.Pool solves a real problem for global free lists. Since this case
> > >>
> > > is
> > >
> > >> not a global free list, sync.Pool has very little to contribute here.
> > >>
> > >
> > >
> > > Are we talking about current non-final non-scalable Pool implementation?
> > > Or the final, scalable one? Or both?
> > >
> > > The plan here, as I see it:
> > > 1. Make the Pool scalable
> > > 2. Replace the local pools in gob with a global one
> > >
> > 
> > I don't see any need to replace the local free lists with a global one.
> > That's exactly the overuse I want to avoid, exactly the overuse that will
> > cause us to take sync.Pool back out of the library.
> > 
> > As I explained above, the most significant feature of sync.Pool is that it
> > lets the runtime influence when memory is released. That is not necessary
> > in this case, as I explained above. The memory will be released when the
> > Decoder is released.
> 
> 
> I do not understand why you call it overuse.
> If we assume that this code needs pooling, then a global sync.Pool looks like
> what doctor said.
> The current ad-hoc pooling solution simply does not work in some cases, and in
> some cases it works poorly. If you have a small object with some nesting, then
> each operation will allocate several objects with no pooling at all. In this
> case the ad-hoc pool adds code and slows down execution giving nothing in
> return. In some intermediate cases you can have some limited benefit from the
> pool, but still significantly less than from a global sync.Pool.
> At the same time I do not see any reason to not use sync.Pool.
> So why not delete the ad-hoc code and replace it with a better standard
> component?

Note that there can be (1) short-lived connections and (2) zillions of
connections with very low activity (instant [web] messaging). For (1) we
under-cache, for (2) we over-cache.

[rsc, 2014-01-06 20:27:07]

I do not want the design of everything to start getting bogged down in
memory management details like deciding which pool it goes in. That's the
kind of overuse I mean. If this starts to dominate discussions about the
implementation of the Go standard library, it will not last for long.

Russ

[cespare, 2014-01-06 21:08:42]

Sergey, your use case sounds to me like the second use case on the original
issue rather than the first one (which sync.Pool attempts to solve):

https://code.google.com/p/go/issues/detail?id=4720




On Mon, Jan 6, 2014 at 1:03 PM, <shkarupin@gmail.com> wrote:

> Dmitry,
> Does this mean the cache is designed for a certain number of elements e.g.
> not too high and not too low? I had a use case where we developed a search
> engine and had to reuse pages (4kb blocks) all the time. The number of
> pages was high - easy 100,000+. Would this pool be suitable for this type
> of task?
>
>
> On Sunday, December 29, 2013 12:16:21 PM UTC-6, Dmitry Vyukov wrote:
>
>> On 2013/12/23 06:32:37, dvyukov wrote:
>> > On 2013/12/20 17:07:32, rsc wrote:
>> > > On Fri, Dec 20, 2013 at 1:24 AM, <mailto:dvy...@google.com> wrote:
>> > >
>> > > > On 2013/12/19 16:20:58, rsc wrote:
>> > > >
>> > > >> not lgtm
>> > > >>
>> > > >
>> > > >  sync.Pool is not for short-lived free lists, especially ones that
>> are
>> > > >> already managed inside of another data structure. The free list
>> inside
>> > > >>
>> > > > the
>> > > >
>> > > >> Decoder only lives as long as the Decoder does. There is no need
>> for
>> > > >>
>> > > > what
>> > > >
>> > > >> sync.Pool contributes.
>> > > >>
>> > > >
>> > > >  sync.Pool solves a real problem for global free lists. Since this
>> case
>> > > >>
>> > > > is
>> > > >
>> > > >> not a global free list, sync.Pool has very little to contribute
>> here.
>> > > >>
>> > > >
>> > > >
>> > > > Are we talking about current non-final non-scalable Pool
>> implementation?
>> > > > Or the final, scalable one? Or both?
>> > > >
>> > > > The plan here, as I see it:
>> > > > 1. Make the Pool scalable
>> > > > 2. Replace the local pools in gob with a global one
>> > > >
>> > >
>> > > I don't see any need to replace the local free lists with a global
>> one.
>> > > That's exactly the overuse I want to avoid, exactly the overuse that
>> will
>> > > cause us to take sync.Pool back out of the library.
>> > >
>> > > As I explained above, the most significant feature of sync.Pool is
>> that it
>> > > lets the runtime influence when memory is released. That is not
>> necessary
>> > > in this case, as I explained above. The memory will be released when
>> the
>> > > Decoder is released.
>>
>>
>> > I do not understand why you call it overuse.
>> > If we assume that this code needs pooling, then a global sync.Pool
>> looks like
>> > what doctor said.
>> > The current ad-hoc pooling solution simply does not work in some
>> cases, and in
>> > some cases it works poorly. If you have a small object with some
>> nesting, then
>> > each operation will allocate several objects with no pooling at all.
>> In this
>> > case the ad-hoc pool adds code and slows down execution giving nothing
>> in
>> > return. In some intermediate cases you can have some limited benefit
>> from the
>> > pool, but still significantly less than from a global sync.Pool.
>> > At the same time I do not see any reason to not use sync.Pool.
>> > So why not delete the ad-hoc code and replace it with a better
>> standard
>> > component?
>>
>> Note that there can be (1) short-lived connections and (2) zillions of
>> connections with very low activity (instant [web] messaging). For (1) we
>> under-cache, for (2) we over-cache.
>>
>>
>> https://codereview.appspot.com/44050044/
>>
>  --
>
> ---
> 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.
>

[shkarupin_gmail.com, 2014-01-06 21:18:29]

I think our pool was closer to the one described here, unless I'm missing 
smth. It was simple "stack like" mem pool to avoid excessive gc. We didn't 
care about data caching just mem blocks

On Monday, January 6, 2014 3:08:21 PM UTC-6, Caleb Spare wrote:
>
> Sergey, your use case sounds to me like the second use case on the 
> original issue rather than the first one (which sync.Pool attempts to 
> solve):
>
> https://code.google.com/p/go/issues/detail?id=4720
>
>
>
>
> On Mon, Jan 6, 2014 at 1:03 PM, <shka...@gmail.com <javascript:>> wrote:
>
>> Dmitry,
>> Does this mean the cache is designed for a certain number of elements 
>> e.g. not too high and not too low? I had a use case where we developed a 
>> search engine and had to reuse pages (4kb blocks) all the time. The number 
>> of pages was high - easy 100,000+. Would this pool be suitable for this 
>> type of task?
>>
>>
>> On Sunday, December 29, 2013 12:16:21 PM UTC-6, Dmitry Vyukov wrote:
>>
>>> On 2013/12/23 06:32:37, dvyukov wrote: 
>>> > On 2013/12/20 17:07:32, rsc wrote: 
>>> > > On Fri, Dec 20, 2013 at 1:24 AM, <mailto:dvy...@google.com> wrote: 
>>> > > 
>>> > > > On 2013/12/19 16:20:58, rsc wrote: 
>>> > > > 
>>> > > >> not lgtm 
>>> > > >> 
>>> > > > 
>>> > > >  sync.Pool is not for short-lived free lists, especially ones that 
>>> are 
>>> > > >> already managed inside of another data structure. The free list 
>>> inside 
>>> > > >> 
>>> > > > the 
>>> > > > 
>>> > > >> Decoder only lives as long as the Decoder does. There is no need 
>>> for 
>>> > > >> 
>>> > > > what 
>>> > > > 
>>> > > >> sync.Pool contributes. 
>>> > > >> 
>>> > > > 
>>> > > >  sync.Pool solves a real problem for global free lists. Since this 
>>> case 
>>> > > >> 
>>> > > > is 
>>> > > > 
>>> > > >> not a global free list, sync.Pool has very little to contribute 
>>> here. 
>>> > > >> 
>>> > > > 
>>> > > > 
>>> > > > Are we talking about current non-final non-scalable Pool 
>>> implementation? 
>>> > > > Or the final, scalable one? Or both? 
>>> > > > 
>>> > > > The plan here, as I see it: 
>>> > > > 1. Make the Pool scalable 
>>> > > > 2. Replace the local pools in gob with a global one 
>>> > > > 
>>> > > 
>>> > > I don't see any need to replace the local free lists with a global 
>>> one. 
>>> > > That's exactly the overuse I want to avoid, exactly the overuse that 
>>> will 
>>> > > cause us to take sync.Pool back out of the library. 
>>> > > 
>>> > > As I explained above, the most significant feature of sync.Pool is 
>>> that it 
>>> > > lets the runtime influence when memory is released. That is not 
>>> necessary 
>>> > > in this case, as I explained above. The memory will be released when 
>>> the 
>>> > > Decoder is released. 
>>>
>>>
>>> > I do not understand why you call it overuse. 
>>> > If we assume that this code needs pooling, then a global sync.Pool 
>>> looks like 
>>> > what doctor said. 
>>> > The current ad-hoc pooling solution simply does not work in some 
>>> cases, and in 
>>> > some cases it works poorly. If you have a small object with some 
>>> nesting, then 
>>> > each operation will allocate several objects with no pooling at all. 
>>> In this 
>>> > case the ad-hoc pool adds code and slows down execution giving nothing 
>>> in 
>>> > return. In some intermediate cases you can have some limited benefit 
>>> from the 
>>> > pool, but still significantly less than from a global sync.Pool. 
>>> > At the same time I do not see any reason to not use sync.Pool. 
>>> > So why not delete the ad-hoc code and replace it with a better 
>>> standard 
>>> > component? 
>>>
>>> Note that there can be (1) short-lived connections and (2) zillions of 
>>> connections with very low activity (instant [web] messaging). For (1) we 
>>> under-cache, for (2) we over-cache. 
>>>
>>>
>>> https://codereview.appspot.com/44050044/ 
>>>
>>  -- 
>>  
>> --- 
>> 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+...@googlegroups.com <javascript:>.
>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>
>

[dvyukov, 2014-01-09 11:02:31]

On Tue, Jan 7, 2014 at 1:03 AM,  <shkarupin@gmail.com> wrote:
> Dmitry,
> Does this mean the cache is designed for a certain number of elements e.g.
> not too high and not too low? I had a use case where we developed a search
> engine and had to reuse pages (4kb blocks) all the time. The number of pages
> was high - easy 100,000+. Would this pool be suitable for this type of task?

sync.Pool must be suitable for this
my comment wrt under/over-caching relates to what we have now w/o sync.Pool



> On Sunday, December 29, 2013 12:16:21 PM UTC-6, Dmitry Vyukov wrote:
>>
>> On 2013/12/23 06:32:37, dvyukov wrote:
>> > On 2013/12/20 17:07:32, rsc wrote:
>> > > On Fri, Dec 20, 2013 at 1:24 AM, <mailto:dvy...@google.com> wrote:
>> > >
>> > > > On 2013/12/19 16:20:58, rsc wrote:
>> > > >
>> > > >> not lgtm
>> > > >>
>> > > >
>> > > >  sync.Pool is not for short-lived free lists, especially ones that
>> are
>> > > >> already managed inside of another data structure. The free list
>> inside
>> > > >>
>> > > > the
>> > > >
>> > > >> Decoder only lives as long as the Decoder does. There is no need
>> for
>> > > >>
>> > > > what
>> > > >
>> > > >> sync.Pool contributes.
>> > > >>
>> > > >
>> > > >  sync.Pool solves a real problem for global free lists. Since this
>> case
>> > > >>
>> > > > is
>> > > >
>> > > >> not a global free list, sync.Pool has very little to contribute
>> here.
>> > > >>
>> > > >
>> > > >
>> > > > Are we talking about current non-final non-scalable Pool
>> implementation?
>> > > > Or the final, scalable one? Or both?
>> > > >
>> > > > The plan here, as I see it:
>> > > > 1. Make the Pool scalable
>> > > > 2. Replace the local pools in gob with a global one
>> > > >
>> > >
>> > > I don't see any need to replace the local free lists with a global
>> one.
>> > > That's exactly the overuse I want to avoid, exactly the overuse that
>> will
>> > > cause us to take sync.Pool back out of the library.
>> > >
>> > > As I explained above, the most significant feature of sync.Pool is
>> that it
>> > > lets the runtime influence when memory is released. That is not
>> necessary
>> > > in this case, as I explained above. The memory will be released when
>> the
>> > > Decoder is released.
>>
>>
>> > I do not understand why you call it overuse.
>> > If we assume that this code needs pooling, then a global sync.Pool
>> looks like
>> > what doctor said.
>> > The current ad-hoc pooling solution simply does not work in some
>> cases, and in
>> > some cases it works poorly. If you have a small object with some
>> nesting, then
>> > each operation will allocate several objects with no pooling at all.
>> In this
>> > case the ad-hoc pool adds code and slows down execution giving nothing
>> in
>> > return. In some intermediate cases you can have some limited benefit
>> from the
>> > pool, but still significantly less than from a global sync.Pool.
>> > At the same time I do not see any reason to not use sync.Pool.
>> > So why not delete the ad-hoc code and replace it with a better
>> standard
>> > component?
>>
>> Note that there can be (1) short-lived connections and (2) zillions of
>> connections with very low activity (instant [web] messaging). For (1) we
>> under-cache, for (2) we over-cache.
>>
>>
>> https://codereview.appspot.com/44050044/