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

Issue 37720047: code review 37720047: encoding/json: use sync.Pool (Closed)

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

Description

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

Patch Set 1 #

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

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

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

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -14 lines) Patch
M src/pkg/encoding/json/encode.go View 1 1 chunk +4 lines, -13 lines 1 comment Download
M src/pkg/encoding/json/stream.go View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 9
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
10 years, 11 months ago (2013-12-18 21:40:14 UTC) #1
iant
LGTM
10 years, 11 months ago (2013-12-18 22:22:28 UTC) #2
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=ff4cb9143edb *** encoding/json: use sync.Pool Benchmark is within the noise. I had ...
10 years, 11 months ago (2013-12-18 23:52:08 UTC) #3
dvyukov
https://codereview.appspot.com/37720047/diff/60001/src/pkg/encoding/json/encode.go File src/pkg/encoding/json/encode.go (right): https://codereview.appspot.com/37720047/diff/60001/src/pkg/encoding/json/encode.go#newcode247 src/pkg/encoding/json/encode.go:247: if v := encodeStatePool.Get(); v != nil { If ...
10 years, 11 months ago (2013-12-19 07:35:31 UTC) #4
iant
On Thu, Dec 19, 2013 at 12:49 AM, <michal@zeto.wroc.pl> wrote: > LGTM ? Really ? ...
10 years, 11 months ago (2013-12-19 16:22:04 UTC) #5
rsc
On Thu, Dec 19, 2013 at 3:49 AM, <michal@zeto.wroc.pl> wrote: > LGTM ? Really ? ...
10 years, 11 months ago (2013-12-19 16:27:44 UTC) #6
michal_pawelek_zeto.wroc.pl
1. What is "global free list" ? (I know what is free list). At what ...
10 years, 11 months ago (2013-12-20 08:13:38 UTC) #7
dvyukov
On Fri, Dec 20, 2013 at 12:13 PM, Michal Pawelek <michal_pawelek@zeto.wroc.pl> wrote: > 1. What ...
10 years, 11 months ago (2013-12-20 08:17:43 UTC) #8
iant
10 years, 11 months ago (2013-12-20 14:16:37 UTC) #9
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
Sign in to reply to this message.

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