code review 100070043: encoding/json: make MarshalIndent faster

encoding/json: make MarshalIndent faster

MarshalIndent was more than four times slower than Marshal.
"newline" function was notably slow because of repeating
calls to WriteString. This patch makes it more efficient.

benchmark                     old ns/op    new ns/op    delta
BenchmarkCodeMarshalIndent     99141396     68867402  -30.54%
BenchmarkIndent                71616746     40213030  -43.85%

benchmark                      old MB/s     new MB/s  speedup
BenchmarkCodeMarshalIndent        19.57        28.18    1.44x
BenchmarkIndent                   27.10        48.25    1.78x

benchmark                    old allocs   new allocs    delta
BenchmarkIndent                       8            8    0.00%

[ruiu, 2014-06-11 03:00:32]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[dave_cheney.net, 2014-06-11 03:05:20]

https://codereview.appspot.com/100070043/diff/70001/src/pkg/encoding/json/ind...
File src/pkg/encoding/json/indent.go (right):

https://codereview.appspot.com/100070043/diff/70001/src/pkg/encoding/json/ind...
src/pkg/encoding/json/indent.go:81: w.dst.WriteString(w.prefix)
is there a reason that the '\n' cannot be added to w.prefix in newNewlineWriter
then you can roll up these  two calls into 

nw := newlineWriter {
     ... 
     prefix: "\n"+prefix
     ...
}
...

w.dst.WriteString(w.prefix) // done!

[ruiu, 2014-06-11 03:10:35]

https://codereview.appspot.com/100070043/diff/70001/src/pkg/encoding/json/ind...
File src/pkg/encoding/json/indent.go (right):

https://codereview.appspot.com/100070043/diff/70001/src/pkg/encoding/json/ind...
src/pkg/encoding/json/indent.go:81: w.dst.WriteString(w.prefix)
We could, and it sounds like a good idea.

[dave_cheney.net, 2014-06-11 08:59:37]

On 2014/06/11 03:10:35, ruiu wrote:
>
https://codereview.appspot.com/100070043/diff/70001/src/pkg/encoding/json/ind...
> File src/pkg/encoding/json/indent.go (right):
> 
>
https://codereview.appspot.com/100070043/diff/70001/src/pkg/encoding/json/ind...
> src/pkg/encoding/json/indent.go:81: w.dst.WriteString(w.prefix)
> We could, and it sounds like a good idea.

Some random benchmark numbers

odessa(~/go/src/pkg/encoding/json) % benchcmp {old,new}.txt | grep Indent
BenchmarkCodeMarshalIndent      123842675     82004131      -33.78%     
BenchmarkCodeMarshalIndent     15.67        23.66        1.51x       
BenchmarkCodeMarshalIndent     46             49             +6.52%     
BenchmarkCodeMarshalIndent     21694157      23132749      +6.63%    

Runtime has gone down, but allocs have gone up. I'm not sure if the escape
analyser is strong enough.

[dave_cheney.net, 2014-06-11 09:14:53]

On 2014/06/11 08:59:37, dfc wrote:
> On 2014/06/11 03:10:35, ruiu wrote:
> >
>
https://codereview.appspot.com/100070043/diff/70001/src/pkg/encoding/json/ind...
> > File src/pkg/encoding/json/indent.go (right):
> > 
> >
>
https://codereview.appspot.com/100070043/diff/70001/src/pkg/encoding/json/ind...
> > src/pkg/encoding/json/indent.go:81: w.dst.WriteString(w.prefix)
> > We could, and it sounds like a good idea.
> 
> Some random benchmark numbers
> 
> odessa(~/go/src/pkg/encoding/json) % benchcmp {old,new}.txt | grep Indent
> BenchmarkCodeMarshalIndent      123842675     82004131      -33.78%     
> BenchmarkCodeMarshalIndent     15.67        23.66        1.51x       
> BenchmarkCodeMarshalIndent     46             49             +6.52%     
> BenchmarkCodeMarshalIndent     21694157      23132749      +6.63%    
> 
> Runtime has gone down, but allocs have gone up. I'm not sure if the escape
> analyser is strong enough.

Here is an alternative version which keeps allocs the same (pretty much
dominated by the initial size of buf)

https://codereview.appspot.com/105030045

odessa(~/go/src/pkg/encoding/json) % benchcmp {old,new}.txt 
benchmark                      old ns/op     new ns/op     delta       
BenchmarkCodeMarshalIndent     124383638     80612457      -35.19%     

benchmark                      old MB/s     new MB/s     speedup     
BenchmarkCodeMarshalIndent     15.60        24.07        1.54x       

benchmark                      old allocs     new allocs     delta      
BenchmarkCodeMarshalIndent     46             46             +0.00%     

benchmark                      old bytes     new bytes     delta      
BenchmarkCodeMarshalIndent     21696617      23133440      +6.62%

[ruiu, 2014-06-11 20:19:00]

How about managing newlineWriter using sync.Pool to avoid allocation? It
feels cleaner to me than doing everything in Indent as you did in your
patch.

On Wed, Jun 11, 2014 at 2:14 AM, <dave@cheney.net> wrote:

> On 2014/06/11 08:59:37, dfc wrote:
>
>> On 2014/06/11 03:10:35, ruiu wrote:
>> >
>>
>
> https://codereview.appspot.com/100070043/diff/70001/src/
> pkg/encoding/json/indent.go
>
>> > File src/pkg/encoding/json/indent.go (right):
>> >
>> >
>>
>
> https://codereview.appspot.com/100070043/diff/70001/src/
> pkg/encoding/json/indent.go#newcode81
>
>> > src/pkg/encoding/json/indent.go:81: w.dst.WriteString(w.prefix)
>> > We could, and it sounds like a good idea.
>>
>
>  Some random benchmark numbers
>>
>
>  odessa(~/go/src/pkg/encoding/json) % benchcmp {old,new}.txt | grep
>>
> Indent
>
>> BenchmarkCodeMarshalIndent      123842675     82004131      -33.78%
>>
>
>  BenchmarkCodeMarshalIndent     15.67        23.66        1.51x
>> BenchmarkCodeMarshalIndent     46             49             +6.52%
>>
>
>  BenchmarkCodeMarshalIndent     21694157      23132749      +6.63%
>>
>
>  Runtime has gone down, but allocs have gone up. I'm not sure if the
>>
> escape
>
>> analyser is strong enough.
>>
>
> Here is an alternative version which keeps allocs the same (pretty much
> dominated by the initial size of buf)
>
> https://codereview.appspot.com/105030045
>
>
> odessa(~/go/src/pkg/encoding/json) % benchcmp {old,new}.txt
> benchmark                      old ns/op     new ns/op     delta
> BenchmarkCodeMarshalIndent     124383638     80612457      -35.19%
>
> benchmark                      old MB/s     new MB/s     speedup
> BenchmarkCodeMarshalIndent     15.60        24.07        1.54x
>
> benchmark                      old allocs     new allocs     delta
> BenchmarkCodeMarshalIndent     46             46             +0.00%
>
> benchmark                      old bytes     new bytes     delta
> BenchmarkCodeMarshalIndent     21696617      23133440      +6.62%
>
> https://codereview.appspot.com/100070043/
>

[dave_cheney.net, 2014-06-11 20:21:29]

My CL was just a suggestion, I didn't want to steal your thunder. Give
sync.Pool a go and see what the benchmarks look like.


On Thu, Jun 12, 2014 at 6:18 AM, Rui Ueyama <ruiu@google.com> wrote:

> How about managing newlineWriter using sync.Pool to avoid allocation? It
> feels cleaner to me than doing everything in Indent as you did in your
> patch.
>
> On Wed, Jun 11, 2014 at 2:14 AM, <dave@cheney.net> wrote:
>
>> On 2014/06/11 08:59:37, dfc wrote:
>>
>>> On 2014/06/11 03:10:35, ruiu wrote:
>>> >
>>>
>>
>> https://codereview.appspot.com/100070043/diff/70001/src/
>> pkg/encoding/json/indent.go
>>
>>> > File src/pkg/encoding/json/indent.go (right):
>>> >
>>> >
>>>
>>
>> https://codereview.appspot.com/100070043/diff/70001/src/
>> pkg/encoding/json/indent.go#newcode81
>>
>>> > src/pkg/encoding/json/indent.go:81: w.dst.WriteString(w.prefix)
>>> > We could, and it sounds like a good idea.
>>>
>>
>>  Some random benchmark numbers
>>>
>>
>>  odessa(~/go/src/pkg/encoding/json) % benchcmp {old,new}.txt | grep
>>>
>> Indent
>>
>>> BenchmarkCodeMarshalIndent      123842675     82004131      -33.78%
>>>
>>
>>  BenchmarkCodeMarshalIndent     15.67        23.66        1.51x
>>> BenchmarkCodeMarshalIndent     46             49             +6.52%
>>>
>>
>>  BenchmarkCodeMarshalIndent     21694157      23132749      +6.63%
>>>
>>
>>  Runtime has gone down, but allocs have gone up. I'm not sure if the
>>>
>> escape
>>
>>> analyser is strong enough.
>>>
>>
>> Here is an alternative version which keeps allocs the same (pretty much
>> dominated by the initial size of buf)
>>
>> https://codereview.appspot.com/105030045
>>
>>
>> odessa(~/go/src/pkg/encoding/json) % benchcmp {old,new}.txt
>> benchmark                      old ns/op     new ns/op     delta
>> BenchmarkCodeMarshalIndent     124383638     80612457      -35.19%
>>
>> benchmark                      old MB/s     new MB/s     speedup
>> BenchmarkCodeMarshalIndent     15.60        24.07        1.54x
>>
>> benchmark                      old allocs     new allocs     delta
>> BenchmarkCodeMarshalIndent     46             46             +0.00%
>>
>> benchmark                      old bytes     new bytes     delta
>> BenchmarkCodeMarshalIndent     21696617      23133440      +6.62%
>>
>> https://codereview.appspot.com/100070043/
>>
>
>

[ruiu, 2014-06-11 20:26:18]

Yeah I didn't mean it, but just wanted to get your input about sync.Pool.
:) I'll give it a shot later.


On Wed, Jun 11, 2014 at 1:21 PM, Dave Cheney <dave@cheney.net> wrote:

> My CL was just a suggestion, I didn't want to steal your thunder. Give
> sync.Pool a go and see what the benchmarks look like.
>
>
> On Thu, Jun 12, 2014 at 6:18 AM, Rui Ueyama <ruiu@google.com> wrote:
>
>> How about managing newlineWriter using sync.Pool to avoid allocation? It
>> feels cleaner to me than doing everything in Indent as you did in your
>> patch.
>>
>> On Wed, Jun 11, 2014 at 2:14 AM, <dave@cheney.net> wrote:
>>
>>> On 2014/06/11 08:59:37, dfc wrote:
>>>
>>>> On 2014/06/11 03:10:35, ruiu wrote:
>>>> >
>>>>
>>>
>>> https://codereview.appspot.com/100070043/diff/70001/src/
>>> pkg/encoding/json/indent.go
>>>
>>>> > File src/pkg/encoding/json/indent.go (right):
>>>> >
>>>> >
>>>>
>>>
>>> https://codereview.appspot.com/100070043/diff/70001/src/
>>> pkg/encoding/json/indent.go#newcode81
>>>
>>>> > src/pkg/encoding/json/indent.go:81: w.dst.WriteString(w.prefix)
>>>> > We could, and it sounds like a good idea.
>>>>
>>>
>>>  Some random benchmark numbers
>>>>
>>>
>>>  odessa(~/go/src/pkg/encoding/json) % benchcmp {old,new}.txt | grep
>>>>
>>> Indent
>>>
>>>> BenchmarkCodeMarshalIndent      123842675     82004131      -33.78%
>>>>
>>>
>>>  BenchmarkCodeMarshalIndent     15.67        23.66        1.51x
>>>> BenchmarkCodeMarshalIndent     46             49             +6.52%
>>>>
>>>
>>>  BenchmarkCodeMarshalIndent     21694157      23132749      +6.63%
>>>>
>>>
>>>  Runtime has gone down, but allocs have gone up. I'm not sure if the
>>>>
>>> escape
>>>
>>>> analyser is strong enough.
>>>>
>>>
>>> Here is an alternative version which keeps allocs the same (pretty much
>>> dominated by the initial size of buf)
>>>
>>> https://codereview.appspot.com/105030045
>>>
>>>
>>> odessa(~/go/src/pkg/encoding/json) % benchcmp {old,new}.txt
>>> benchmark                      old ns/op     new ns/op     delta
>>> BenchmarkCodeMarshalIndent     124383638     80612457      -35.19%
>>>
>>> benchmark                      old MB/s     new MB/s     speedup
>>> BenchmarkCodeMarshalIndent     15.60        24.07        1.54x
>>>
>>> benchmark                      old allocs     new allocs     delta
>>> BenchmarkCodeMarshalIndent     46             46             +0.00%
>>>
>>> benchmark                      old bytes     new bytes     delta
>>> BenchmarkCodeMarshalIndent     21696617      23133440      +6.62%
>>>
>>> https://codereview.appspot.com/100070043/
>>>
>>
>>
>

[ruiu, 2014-06-12 01:50:06]

Hello golang-codereviews@googlegroups.com, dave@cheney.net (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[ruiu, 2014-06-16 23:01:10]

Ping.

[bradfitz, 2014-06-16 23:37:41]

https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
File src/pkg/encoding/json/indent.go (right):

https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
src/pkg/encoding/json/indent.go:72: var nwPool sync.Pool
I'd add the New func here, since it works in lieu of documentation to explain to
the reader how it's used:

var nwPool = sync.Pool{New: func() interface{} { return new(newlineWriter) }}

https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
src/pkg/encoding/json/indent.go:83: nw.prefix = "\n" + prefix
do you need this concatenation? can't you just WriteByte the '\n' before line
90?

[ruiu, 2014-06-16 23:47:47]

https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
File src/pkg/encoding/json/indent.go (right):

https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
src/pkg/encoding/json/indent.go:72: var nwPool sync.Pool
Done with newlines after { and before }, as it looked too packed.

https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
src/pkg/encoding/json/indent.go:83: nw.prefix = "\n" + prefix
Originally it's done in that way, and I agree that it's better than
concatenation. Done.

[bradfitz, 2014-06-16 23:57:49]

LGTM code-wise but give rsc a day or two to look first

[bradfitz, 2014-06-16 23:59:04]

https://codereview.appspot.com/100070043/diff/150001/src/pkg/encoding/json/be...
File src/pkg/encoding/json/bench_test.go (right):

https://codereview.appspot.com/100070043/diff/150001/src/pkg/encoding/json/be...
src/pkg/encoding/json/bench_test.go:114: if _, err := MarshalIndent(&codeStruct,
"", " "); err != nil {
I would only benchmark Indent, not MarshalIdent. Otherwise you're largely
measuring allocation/GC time.

I'd instead just Indent to the same bytes.Buffer repeatedly, and Reset it per
iteration.

[ruiu, 2014-06-17 00:57:17]

Makes sense. Added BenchmarkIndent. The result of the benchmark is this (also
pasted to the patch description):

benchmark                      old MB/s     new MB/s  speedup
BenchmarkIndent                   27.10        48.25    1.78x

benchmark                    old allocs   new allocs    delta
BenchmarkIndent                       8            8    0.00%

[dave_cheney.net, 2014-06-17 03:21:38]

On 17/06/2014, at 9:37, bradfitz@golang.org wrote:

> 
>
https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
> File src/pkg/encoding/json/indent.go (right):
> 
>
https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
> src/pkg/encoding/json/indent.go:72: var nwPool sync.Pool
> I'd add the New func here, since it works in lieu of documentation to
> explain to the reader how it's used:
> 
> var nwPool = sync.Pool{New: func() interface{} { return
> new(newlineWriter) }}
> 
>
https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
> src/pkg/encoding/json/indent.go:83: nw.prefix = "\n" + prefix
> do you need this concatenation? can't you just WriteByte the '\n' before
> line 90?

This was my suggestion to avoid two calls to WriteByte per newline()

> 
> https://codereview.appspot.com/100070043/

[ruiu, 2014-06-17 15:42:15]

On 2014/06/17 03:21:38, dfc wrote:
> 
> On 17/06/2014, at 9:37, mailto:bradfitz@golang.org wrote:
> 
> > 
> >
>
https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
> > File src/pkg/encoding/json/indent.go (right):
> > 
> >
>
https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
> > src/pkg/encoding/json/indent.go:72: var nwPool sync.Pool
> > I'd add the New func here, since it works in lieu of documentation to
> > explain to the reader how it's used:
> > 
> > var nwPool = sync.Pool{New: func() interface{} { return
> > new(newlineWriter) }}
> > 
> >
>
https://codereview.appspot.com/100070043/diff/130001/src/pkg/encoding/json/in...
> > src/pkg/encoding/json/indent.go:83: nw.prefix = "\n" + prefix
> > do you need this concatenation? can't you just WriteByte the '\n' before
> > line 90?
> 
> This was my suggestion to avoid two calls to WriteByte per newline()

I now prefer not to concatenate "\n" and the given string, as it'd always be a
new allocation. An extra call of WriteByte is not bad as a new allocation.

[dave_cheney.net, 2014-06-17 23:47:19]

Sure thing, go for it.


On Wed, Jun 18, 2014 at 1:42 AM, <ruiu@google.com> wrote:

> On 2014/06/17 03:21:38, dfc wrote:
>
>
>  On 17/06/2014, at 9:37, mailto:bradfitz@golang.org wrote:
>>
>
>  >
>> >
>>
>
> https://codereview.appspot.com/100070043/diff/130001/src/
> pkg/encoding/json/indent.go
>
>> > File src/pkg/encoding/json/indent.go (right):
>> >
>> >
>>
>
> https://codereview.appspot.com/100070043/diff/130001/src/
> pkg/encoding/json/indent.go#newcode72
>
>> > src/pkg/encoding/json/indent.go:72: var nwPool sync.Pool
>> > I'd add the New func here, since it works in lieu of documentation
>>
> to
>
>> > explain to the reader how it's used:
>> >
>> > var nwPool = sync.Pool{New: func() interface{} { return
>> > new(newlineWriter) }}
>> >
>> >
>>
>
> https://codereview.appspot.com/100070043/diff/130001/src/
> pkg/encoding/json/indent.go#newcode83
>
>> > src/pkg/encoding/json/indent.go:83: nw.prefix = "\n" + prefix
>> > do you need this concatenation? can't you just WriteByte the '\n'
>>
> before
>
>> > line 90?
>>
>
>  This was my suggestion to avoid two calls to WriteByte per newline()
>>
>
> I now prefer not to concatenate "\n" and the given string, as it'd
> always be a new allocation. An extra call of WriteByte is not bad as a
> new allocation.
>
> https://codereview.appspot.com/100070043/
>

[r, 2014-06-22 05:52:42]

NOT LGTM

Sync.Pool is not meant to live in every package.

[gobot, 2014-07-16 10:56:51]

R=close (assigned by dave@cheney.net)