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

Issue 44050044: code review 44050044: encoding/gob: use sync.Pool (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 10 months ago by bradfitz
Modified:
10 years, 9 months ago
Reviewers:
cespare, shkarupin, rsc, r, dvyukov, golang-dev
Visibility:
Public.

Description

encoding/gob: use sync.Pool Update Issue 4720

Patch Set 1 #

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -18 lines) Patch
M src/pkg/encoding/gob/decode.go View 1 1 chunk +8 lines, -9 lines 0 comments Download
M src/pkg/encoding/gob/decoder.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/encoding/gob/encode.go View 1 2 chunks +5 lines, -7 lines 0 comments Download
M src/pkg/encoding/gob/encoder.go View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 20
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
10 years, 10 months ago (2013-12-19 00:17:02 UTC) #1
r
Not sure about this one. You're putting a mutex into something that is locked at ...
10 years, 10 months ago (2013-12-19 00:19:54 UTC) #2
r
Never mind, the mutexes will all happen on the same thread. Still, I don't see ...
10 years, 10 months ago (2013-12-19 00:21:11 UTC) #3
rsc
What is the improvement here? This is a local free list, not a global one. ...
10 years, 10 months ago (2013-12-19 00:25:34 UTC) #4
bradfitz
Oh, whoops, I thought this was a longer-lived thing. I don't use encoding/gob. I at ...
10 years, 10 months ago (2013-12-19 00:28:46 UTC) #5
dvyukov
Why it is a local cache? To avoid contention? Can it be made global?
10 years, 10 months ago (2013-12-19 07:28:15 UTC) #6
r
To avoid contention. To reduce memory usage. Because it makes sense to be local. Because ...
10 years, 10 months ago (2013-12-19 15:13:57 UTC) #7
dvyukov
On 2013/12/19 15:13:57, r wrote: > To avoid contention. To reduce memory usage. Because it ...
10 years, 10 months ago (2013-12-19 15:19:08 UTC) #8
r
But it's not the final version yet and it will add contention. The current code ...
10 years, 10 months ago (2013-12-19 15:23:34 UTC) #9
rsc
not lgtm sync.Pool is not for short-lived free lists, especially ones that are already managed ...
10 years, 10 months ago (2013-12-19 16:20:58 UTC) #10
bradfitz
On Thu, Dec 19, 2013 at 8:20 AM, Russ Cox <rsc@golang.org> wrote: > not lgtm ...
10 years, 10 months ago (2013-12-19 17:22:47 UTC) #11
rsc
On Thu, Dec 19, 2013 at 12:22 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > On Thu, Dec ...
10 years, 10 months ago (2013-12-19 17:58:51 UTC) #12
dvyukov
On 2013/12/19 16:20:58, rsc wrote: > not lgtm > > sync.Pool is not for short-lived ...
10 years, 10 months ago (2013-12-20 06:24:23 UTC) #13
rsc
On Fri, Dec 20, 2013 at 1:24 AM, <dvyukov@google.com> wrote: > On 2013/12/19 16:20:58, rsc ...
10 years, 10 months ago (2013-12-20 17:07:32 UTC) #14
dvyukov
On 2013/12/20 17:07:32, rsc wrote: > On Fri, Dec 20, 2013 at 1:24 AM, <mailto:dvyukov@google.com> ...
10 years, 9 months ago (2013-12-23 06:32:37 UTC) #15
dvyukov
On 2013/12/23 06:32:37, dvyukov wrote: > On 2013/12/20 17:07:32, rsc wrote: > > On Fri, ...
10 years, 9 months ago (2013-12-29 18:16:21 UTC) #16
rsc
I do not want the design of everything to start getting bogged down in memory ...
10 years, 9 months ago (2014-01-06 20:27:07 UTC) #17
cespare
Sergey, your use case sounds to me like the second use case on the original ...
10 years, 9 months ago (2014-01-06 21:08:42 UTC) #18
shkarupin_gmail.com
I think our pool was closer to the one described here, unless I'm missing smth. ...
10 years, 9 months ago (2014-01-06 21:18:29 UTC) #19
dvyukov
10 years, 9 months ago (2014-01-09 11:02:31 UTC) #20
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/
Sign in to reply to this message.

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