code review 9129044: encoding/json: faster encoding

encoding/json: faster encoding

The old code was caching per-type struct field info. Instead,
cache type-specific encoding funcs, tailored for that
particular type to avoid unnecessary reflection at runtime.
Once the machine is built once, future encodings of that type
just run the func.

benchmark               old ns/op    new ns/op    delta
BenchmarkCodeEncoder     48424939     36975320  -23.64%

benchmark                old MB/s     new MB/s  speedup
BenchmarkCodeEncoder        40.07        52.48    1.31x

Additionally, the numbers seem stable now at ~52 MB/s, whereas
the numbers for the old code were all over the place: 11 MB/s,
40 MB/s, 13 MB/s, 39 MB/s, etc.  In the benchmark above I compared
against the best I saw the old code do.

[bradfitz, 2013-05-04 17:04:53]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-05-04 17:05:57]

(obviously for after Go 1.1.  just getting off my laptop...)



On Sat, May 4, 2013 at 10:04 AM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> encoding/json: faster encoding
>
> The old code was caching per-type struct field info. Instead,
> cache type-specific encoding funcs, tailored for that
> particular type to avoid unnecessary reflection at runtime.
> Once the machine is built once, future encodings of that type
> just run the func.
>
> benchmark               old ns/op    new ns/op    delta
> BenchmarkCodeEncoder     48424939     36975320  -23.64%
>
> benchmark                old MB/s     new MB/s  speedup
> BenchmarkCodeEncoder        40.07        52.48    1.31x
>
> Additionally, the numbers seem stable now at ~52 MB/s, whereas
> the numbers for the old code were all over the place: 11 MB/s,
> 40 MB/s, 13 MB/s, 39 MB/s, etc.  In the benchmark above I compared
> against the best I saw the old code do.
>
> Please review this at
https://codereview.appspot.**com/9129044/<https://codereview.appspot.com/9129...
>
> Affected files:
>   M src/pkg/encoding/json/decode_**test.go
>   M src/pkg/encoding/json/encode.**go
>
>
>

[gobot, 2013-05-14 22:15:03]

R=r (assigned by bradfitz)

[gobot, 2013-05-17 21:13:57]

R=rsc (assigned by r)

[bradfitz, 2013-06-03 13:09:54]

Ping.

This CL is now 4 weeks old, without any feedback.

I'll take a review from anybody, if encoding/json doesn't have an owner.



On Fri, May 17, 2013 at 2:13 PM, <gobot@golang.org> wrote:

> R=rsc (assigned by r)
>
>
https://codereview.appspot.**com/9129044/<https://codereview.appspot.com/9129...
>

[rsc, 2013-06-03 13:11:33]

Still having trouble getting through my inbox. How about this: I will
review this CL when the "no more noise on golang-dev" goes into effect.

Russ

[adg, 2013-07-03 00:09:01]

Looks good.

https://codereview.appspot.com/9129044/diff/9001/src/pkg/encoding/json/encode.go
File src/pkg/encoding/json/encode.go (right):

https://codereview.appspot.com/9129044/diff/9001/src/pkg/encoding/json/encode...
src/pkg/encoding/json/encode.go:233: if _, ok := r.(runtime.Error); ok {
how about

switch e := r.(type) {
case runtime.Error:
case error:
  err = e
  return
}
panic(r)

https://codereview.appspot.com/9129044/diff/9001/src/pkg/encoding/json/encode...
src/pkg/encoding/json/encode.go:568: type encoderFunc func(e *encodeState, v
reflect.Value, quoted bool)
a doc comment explaining what an encoderfunc should do would be nice

https://codereview.appspot.com/9129044/diff/9001/src/pkg/encoding/json/encode...
src/pkg/encoding/json/encode.go:599: var wg sync.WaitGroup
you could also use 

done := make(chan struct{})
...
  <-done
...
close(done)

which might be faster? I don't know.

https://codereview.appspot.com/9129044/diff/9001/src/pkg/encoding/json/encode...
src/pkg/encoding/json/encode.go:608: // Might duplicate effort but won't hold
other computations back.
I don't think it will duplicate effort; you've already populated the cache with
the wait func; any other encodings of this type will wait for you to build this
f.

https://codereview.appspot.com/9129044/diff/9001/src/pkg/encoding/json/encode...
src/pkg/encoding/json/encode.go:671: if v.Kind() == reflect.Ptr && v.IsNil() {
shouldn't you let the marshaller implementation handle this case?

https://codereview.appspot.com/9129044/diff/9001/src/pkg/encoding/json/encode...
src/pkg/encoding/json/encode.go:687: va := v.Addr()
shouldn't the this func just be

valueIsMarshallerEncoder(e, v.Addr(), quoted)

??

https://codereview.appspot.com/9129044/diff/9001/src/pkg/encoding/json/encode...
src/pkg/encoding/json/encode.go:757: float32Encoder = (floatEncoder(32)).encode
very nice

[rsc, 2013-08-06 16:33:26]

I took a look at this CL. I understand that it is a bit like gob, in that
it is using lots of closures, but I am not convinced that's a good thing.
The problem with closures is that they are opaque: you can't look at them,
only execute them. So they turn out to be difficult to debug. I've been
through a few of those debugging sessions in gob and I don't want json to
become like that. It is also telling that the code is using closures,
method values, and ordinary functions. Too many mechanisms.

I think the idea of precomputing the converter is fine, but can you do it
with an ordinary interface and type-specific implementations? It should
boil down to the same thing, should be just as fast, and should be easy to
convert the current code to that form, but I think it will be easier to
maintain in the long run.

Also, can you move the code up where reflectValueQuoted was, in the same
order that the switch did things, so that the common code snippets have a
chance to be identified as common in the diffs?

Thanks.
Russ

[r, 2013-08-06 20:48:57]

also, gob needs a rewrite, now that we know how to write code like this.

-rob

[bradfitz, 2013-08-07 16:35:44]

Hello rsc@golang.org, adg@golang.org (cc: gobot@golang.org,
golang-dev@googlegroups.com, r@golang.org),

Please take another look.

[bradfitz, 2013-08-07 16:37:30]

PTAL

Rearranged the code a bit to try to minimize patch size.  Not sure it
helped too much.

All closures gone now.  Looking at go tool pprof --svg, all the boxes have
readable names now.  I was concerned about this before too and was in the
process of moving things to types (hence "too many mechanisms") but was
trying to keep the code smaller, hence some of the remaining closures.




On Wed, Aug 7, 2013 at 9:35 AM, <bradfitz@golang.org> wrote:

> Hello rsc@golang.org, adg@golang.org (cc: gobot@golang.org,
> golang-dev@googlegroups.com, r@golang.org),
>
> Please take another look.
>
>
>
https://codereview.appspot.**com/9129044/<https://codereview.appspot.com/9129...
>

[rsc, 2013-08-09 02:04:12]

LGTM

https://codereview.appspot.com/9129044/diff/26001/src/pkg/encoding/json/encod...
File src/pkg/encoding/json/encode.go (right):

https://codereview.appspot.com/9129044/diff/26001/src/pkg/encoding/json/encod...
src/pkg/encoding/json/encode.go:356: // buildTypeEncoder constructs an
encoderFunc for a type.
s/build/new/ throughout

https://codereview.appspot.com/9129044/diff/26001/src/pkg/encoding/json/encod...
src/pkg/encoding/json/encode.go:534: fieldFuncs []encoderFunc
fieldEncs (match all the elemEnc)

https://codereview.appspot.com/9129044/diff/26001/src/pkg/encoding/json/encod...
src/pkg/encoding/json/encode.go:632: arrayEncoder encoderFunc
arrayEnc

[bradfitz, 2013-08-09 16:46:52]

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

encoding/json: faster encoding

The old code was caching per-type struct field info. Instead,
cache type-specific encoding funcs, tailored for that
particular type to avoid unnecessary reflection at runtime.
Once the machine is built once, future encodings of that type
just run the func.

benchmark               old ns/op    new ns/op    delta
BenchmarkCodeEncoder     48424939     36975320  -23.64%

benchmark                old MB/s     new MB/s  speedup
BenchmarkCodeEncoder        40.07        52.48    1.31x

Additionally, the numbers seem stable now at ~52 MB/s, whereas
the numbers for the old code were all over the place: 11 MB/s,
40 MB/s, 13 MB/s, 39 MB/s, etc.  In the benchmark above I compared
against the best I saw the old code do.

R=rsc, adg
CC=gobot, golang-dev, r
https://codereview.appspot.com/9129044