code review 37720047: encoding/json: use sync.Pool

encoding/json: use sync.Pool

Benchmark is within the noise. I had to run this a dozen times
each before & after (on wall power, without a browser running)
before I could get halfway consistent numbers, and even then
they jumped all over the place, with the new one sometimes
being better. But these are the best of a dozen each.

Slowdown is expected anyway, since I imagine channels are
optimized more.

benchmark                 old ns/op    new ns/op    delta
BenchmarkCodeEncoder       26556987     27291072   +2.76%
BenchmarkEncoderEncode         1069         1071   +0.19%

benchmark                  old MB/s     new MB/s  speedup
BenchmarkCodeEncoder          73.07        71.10    0.97x

benchmark                old allocs   new allocs    delta
BenchmarkEncoderEncode            2            2    0.00%

benchmark                 old bytes    new bytes    delta
BenchmarkEncoderEncode          221          221    0.00%

Update Issue 4720

[bradfitz, 2013-12-18 21:40:14]

Hello golang-dev@googlegroups.com,

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

[iant, 2013-12-18 22:22:28]

LGTM

[bradfitz, 2013-12-18 23:52:08]

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

encoding/json: use sync.Pool

Benchmark is within the noise. I had to run this a dozen times
each before & after (on wall power, without a browser running)
before I could get halfway consistent numbers, and even then
they jumped all over the place, with the new one sometimes
being better. But these are the best of a dozen each.

Slowdown is expected anyway, since I imagine channels are
optimized more.

benchmark                 old ns/op    new ns/op    delta
BenchmarkCodeEncoder       26556987     27291072   +2.76%
BenchmarkEncoderEncode         1069         1071   +0.19%

benchmark                  old MB/s     new MB/s  speedup
BenchmarkCodeEncoder          73.07        71.10    0.97x

benchmark                old allocs   new allocs    delta
BenchmarkEncoderEncode            2            2    0.00%

benchmark                 old bytes    new bytes    delta
BenchmarkEncoderEncode          221          221    0.00%

Update Issue 4720

R=golang-dev, iant
CC=golang-dev
https://codereview.appspot.com/37720047

[dvyukov, 2013-12-19 07:35:31]

https://codereview.appspot.com/37720047/diff/60001/src/pkg/encoding/json/enco...
File src/pkg/encoding/json/encode.go (right):

https://codereview.appspot.com/37720047/diff/60001/src/pkg/encoding/json/enco...
src/pkg/encoding/json/encode.go:247: if v := encodeStatePool.Get(); v != nil {
If cost Reset'ing the Buffer is small, a cleaner approach can be to reset it
before Put, then you can use Pool.New

[iant, 2013-12-19 16:22:04]

On Thu, Dec 19, 2013 at 12:49 AM,  <michal@zeto.wroc.pl> wrote:
> LGTM ? Really ? sync.Pool is a crap. Dont use this crap in stdlib. I
> strongly encourage You to study net/rpc package (spacially field "next" in
> type Request and functions: freeRequest(...) and getRequest(...)). You will
> learn that solution used there is so simple that sync.Pool is unnecessary.

I think that is a discussion to have on golang-dev.

Ian

[rsc, 2013-12-19 16:27:44]

On Thu, Dec 19, 2013 at 3:49 AM, <michal@zeto.wroc.pl> wrote:

> LGTM ? Really ? sync.Pool is a crap. Dont use this crap in stdlib. I
> strongly encourage You to study net/rpc package (spacially field "next" in
> type Request and functions: freeRequest(...) and getRequest(...)). You will
> learn that solution used there is so simple that sync.Pool is unnecessary.
>

This comment is far from constructive.

I agree that sync.Pool can be overused, and that we need to be careful to
avoid that. However, sync.Pool is the right implementation for a global
free list, because it provides a way for the runtime to reclaim the memory
if it decides that it would be better used by some other package.

This CL changed a global free list backed by a channel into a global free
list backed by a sync.Pool. That is exactly what sync.Pool is for. It would
be simpler to keep the channel or use a linked list, but both of those
solutions have the memory retention problem that sync.Pool solves.

The comparison with net/rpc is not quite the same. In that package the free
list is not global. It is attached to another type (Server) that a client
can let go of and cause to be garbage collected.

Russ

[michal_pawelek_zeto.wroc.pl, 2013-12-20 08:13:38]

1. What is "global free list" ? (I know what is free list). At what level free
list is global - at GO runtime level or package level or function level ?
2. You wrote:
    "because it provides a way for the runtime to reclaim the memory if it
decides that it would be better used by some other package"
    IMHO this should be the task of garbage collector. Why create another
garbage collector ? Even in Java there is no such thing like general-purpose
Pool (there are specialized pools e.g ThreadPool - but it is very good only for
threads).
3. How should I know if should use sync.Pool or if I should relay on garbage
collector ?
  ----- Original Message ----- 
  From: Russ Cox 
  To: michal@zeto.wroc.pl 
  Cc: golang-dev ; reply@codereview-hr.appspotmail.com ; Brad Fitzpatrick 
  Sent: Thursday, December 19, 2013 5:27 PM
  Subject: Re: [golang-dev] Re: code review 37720047: encoding/json: use
sync.Pool (issue 37720047)


  On Thu, Dec 19, 2013 at 3:49 AM, <michal@zeto.wroc.pl> wrote:

    LGTM ? Really ? sync.Pool is a crap. Dont use this crap in stdlib. I
strongly encourage You to study net/rpc package (spacially field "next" in type
Request and functions: freeRequest(...) and getRequest(...)). You will learn
that solution used there is so simple that sync.Pool is unnecessary.



  This comment is far from constructive.


  I agree that sync.Pool can be overused, and that we need to be careful to
avoid that. However, sync.Pool is the right implementation for a global free
list, because it provides a way for the runtime to reclaim the memory if it
decides that it would be better used by some other package.


  This CL changed a global free list backed by a channel into a global free list
backed by a sync.Pool. That is exactly what sync.Pool is for. It would be
simpler to keep the channel or use a linked list, but both of those solutions
have the memory retention problem that sync.Pool solves.


  The comparison with net/rpc is not quite the same. In that package the free
list is not global. It is attached to another type (Server) that a client can
let go of and cause to be garbage collected.


  Russ


__________ Informacja programu ESET Endpoint Antivirus, wersja bazy sygnatur
wirusów 9195 (20131219) __________

Wiadomoœæ zosta³a sprawdzona przez program ESET Endpoint Antivirus.

http://www.eset.pl lub http://www.eset.com 

[dvyukov, 2013-12-20 08:17:43]

On Fri, Dec 20, 2013 at 12:13 PM, Michal Pawelek
<michal_pawelek@zeto.wroc.pl> wrote:
> 1. What is "global free list" ? (I know what is free list). At what level
> free list is global - at GO runtime level or package level or function level
> ?

It roughly means "all objects of the same type in the same package".


> 2. You wrote:
>     "because it provides a way for the runtime to reclaim the memory if it
> decides that it would be better used by some other package"
>     IMHO this should be the task of garbage collector. Why create another
> garbage collector ? Even in Java there is no such thing like general-purpose
> Pool (there are specialized pools e.g ThreadPool - but it is very good only
> for threads).

And ThreadPool should be a task of OS scheduler. Why create another scheduler?
This pool as general as ThreadPool -- it's for memory blocks.

> 3. How should I know if should use sync.Pool or if I should relay on garbage
> collector ?

Short version: 1. Profile, 2. Try


>
> ----- Original Message -----
> From: Russ Cox
> To: michal@zeto.wroc.pl
> Cc: golang-dev ; reply@codereview-hr.appspotmail.com ; Brad Fitzpatrick
> Sent: Thursday, December 19, 2013 5:27 PM
> Subject: Re: [golang-dev] Re: code review 37720047: encoding/json: use
> sync.Pool (issue 37720047)
>
> On Thu, Dec 19, 2013 at 3:49 AM, <michal@zeto.wroc.pl> wrote:
>>
>> LGTM ? Really ? sync.Pool is a crap. Dont use this crap in stdlib. I
>> strongly encourage You to study net/rpc package (spacially field "next" in
>> type Request and functions: freeRequest(...) and getRequest(...)). You will
>> learn that solution used there is so simple that sync.Pool is unnecessary.
>
>
> This comment is far from constructive.
>
> I agree that sync.Pool can be overused, and that we need to be careful to
> avoid that. However, sync.Pool is the right implementation for a global free
> list, because it provides a way for the runtime to reclaim the memory if it
> decides that it would be better used by some other package.
>
> This CL changed a global free list backed by a channel into a global free
> list backed by a sync.Pool. That is exactly what sync.Pool is for. It would
> be simpler to keep the channel or use a linked list, but both of those
> solutions have the memory retention problem that sync.Pool solves.
>
> The comparison with net/rpc is not quite the same. In that package the free
> list is not global. It is attached to another type (Server) that a client
> can let go of and cause to be garbage collected.
>
> Russ
>
>
>
> __________ Informacja programu ESET Endpoint Antivirus, wersja bazy sygnatur
> wirusów 9195 (20131219) __________
>
> Wiadomoœæ zosta³a sprawdzona przez program ESET Endpoint Antivirus.
>
> http://www.eset.pl lub http://www.eset.com
>
> --
>
> ---
> 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.

[iant, 2013-12-20 14:16:37]

On Fri, Dec 20, 2013 at 12:13 AM, Michal Pawelek
<michal_pawelek@zeto.wroc.pl> wrote:
>
> 2. You wrote:
>     "because it provides a way for the runtime to reclaim the memory if it
> decides that it would be better used by some other package"
>     IMHO this should be the task of garbage collector. Why create another
> garbage collector ? Even in Java there is no such thing like general-purpose
> Pool (there are specialized pools e.g ThreadPool - but it is very good only
> for threads).

Go and Java are very different languages.

Profiling has showed us that in the current implementation it's
generally a win to avoid allocation, something that is easier to do in
Go than in Java.  That fact led people to create several different
local caches.  The sync.Pool type permits us to aggregate those caches
in a single implementation.  We can now make that implementation
faster, speeding up all uses.

Of course it's possible that in the future it will be better to simply
rely on the garbage collector.  In that case sync.Pool can become a
trivial wrapper, once again speeding up code in a single place rather
than requiring us to find the various different local caches.


> 3. How should I know if should use sync.Pool or if I should relay on garbage
> collector ?

Profile.

Ian