code review 12243043: encoding/json: Speed up decoding by 50% (issue 12243043)

[alberto.ga...@gmail.com]

Reviewers: golang-dev1,

Message:
Hello golan...@googlegroups.com,

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


Description:
encoding/json: Speed up decoding by 50%

* Avoid recalculating len(data) while decoding.
* Avoid excessive byte to int castings.
* Avoid creating temporary slices in nextValue(), returning
an offset suffices.
* Cache key <=> field lookups. They use strings.EqualFold,
which is pretty slow.
* Avoid converting keys to strings when possible.

Please review this at https://codereview.appspot.com/12243043/

Affected files:
M src/pkg/encoding/json/bench_test.go
M src/pkg/encoding/json/decode.go
M src/pkg/encoding/json/decode_test.go
M src/pkg/encoding/json/indent.go
M src/pkg/encoding/json/scanner.go
M src/pkg/encoding/json/scanner_test.go
M src/pkg/encoding/json/stream.go

[alberto.ga...@gmail.com]

Here are some benchmarks before and after applying this patch set:

Before:
BenchmarkCodeUnmarshal 10 156293404 ns/op 12.42 MB/s
BenchmarkCodeUnmarshalReuse 10 150102989 ns/op


After:
BenchmarkCodeUnmarshal 20 97127556 ns/op 19.98 MB/s
BenchmarkCodeUnmarshalMethod 20 97804487 ns/op 19.84 MB/s
BenchmarkCodeUnmarshalReuse 20 89162147 ns/op 21.76 MB/s

The difference is quite substantial.

Regards,
Alberto

https://codereview.appspot.com/12243043/

[David Symonds]

Please use misc/benchcmp to compare benchmark output, and include it in the CL description.

[Rémy Oudompheng]

Is it possible for you to post CPU profiles?
I would like to know if the improvement is consistent across various
stack splitting positions.

Rémy.

2013/8/1, David Symonds <dsym...@golang.org>:

> Please use misc/benchcmp to compare benchmark output, and include it in the
> CL description.
>

> --
>
> ---
> 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+...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>

[alberto.ga...@gmail.com]

On 2013/08/01 10:36:18, dsymonds wrote:
> Please use misc/benchcmp to compare benchmark output, and include it
in the
> CL description.

Thanks, done!

https://codereview.appspot.com/12243043/

[alberto.ga...@gmail.com]

On 2013/08/01 10:50:52, remyoudompheng wrote:
> Is it possible for you to post CPU profiles?
> I would like to know if the improvement is consistent across various
> stack splitting positions.

> Rémy.

Sure, no problem. Here are the CPU profiles for BenchmarkCodeUnmarshal
before and after:

fiam@ubuntu:~/go/src/pkg/encoding/json$ go test -v -run=none
-bench=CodeUnmarshal$ -cpuprofile=cpu.old.prof
PASS
BenchmarkCodeUnmarshal 10 149568191 ns/op 12.97 MB/s

https://dl.dropboxusercontent.com/u/3193787/cpu.old.prof


fiam@ubuntu:~/go/src/pkg/encoding/json$ go test -v -run=none
-bench=CodeUnmarshal$ -cpuprofile=cpu.new.prof
PASS
BenchmarkCodeUnmarshal 20 96215951 ns/op 20.17 MB/s
ok encoding/json 2.257s

https://dl.dropboxusercontent.com/u/3193787/cpu.new.prof

Regards,
Alberto

https://codereview.appspot.com/12243043/

[rogp...@gmail.com]

I'd like to see the incremental effect of each
change included in this CL, rather than bundling
them all together. Particularly the unsafe change,
if we even want to contemplate that one.


https://codereview.appspot.com/12243043/diff/5001/src/pkg/encoding/json/decode.go
File src/pkg/encoding/json/decode.go (right):

https://codereview.appspot.com/12243043/diff/5001/src/pkg/encoding/json/decode.go#newcode21
src/pkg/encoding/json/decode.go:21: "unsafe"
I think this is a no-no. We don't want to
add new unsafe packages.

https://codereview.appspot.com/12243043/

[alberto.ga...@gmail.com]

On 2013/08/01 13:45:19, rog wrote:
> I'd like to see the incremental effect of each
> change included in this CL, rather than bundling
> them all together. Particularly the unsafe change,
> if we even want to contemplate that one.

The difference is significant for that change, specially when it comes
to allocations:

fiam@ubuntu:~/go/src/pkg/encoding/json$ ~/go/misc/benchcmp unsafe.txt
safe.txt
benchmark old ns/op new ns/op delta
BenchmarkCodeUnmarshal 82400716 90397421 +9.70%
BenchmarkCodeUnmarshalMethod 85276712 92357337 +8.30%
BenchmarkCodeUnmarshalReuse 76710096 84389386 +10.01%

benchmark old MB/s new MB/s speedup
BenchmarkCodeUnmarshal 23.55 21.47 0.91x
BenchmarkCodeUnmarshalMethod 22.76 21.01 0.92x
BenchmarkCodeUnmarshalReuse 25.30 22.99 0.91x

benchmark old allocs new allocs delta
BenchmarkCodeUnmarshal 105749 195386 84.76%
BenchmarkCodeUnmarshalMethod 105748 195384 84.76%
BenchmarkCodeUnmarshalReuse 89892 179521 99.71%

benchmark old bytes new bytes delta
BenchmarkCodeUnmarshal 3457194 4275326 23.66%
BenchmarkCodeUnmarshalMethod 3456840 4274932 23.67%
BenchmarkCodeUnmarshalReuse 2047975 2864786 39.88%

https://codereview.appspot.com/12243043/diff/5001/src/pkg/encoding/json/decode.go
> File src/pkg/encoding/json/decode.go (right):


https://codereview.appspot.com/12243043/diff/5001/src/pkg/encoding/json/decode.go#newcode21
> src/pkg/encoding/json/decode.go:21: "unsafe"
> I think this is a no-no. We don't want to
> add new unsafe packages.

If there's a big concern, I could a "safe" version of the string to
[]byte conversion which could be conditionally
compiled using build tags. I really can't find any valid reason to not
take advantage of unsafe package in this
situation, because the string is thrown away as soon as the lookup in
the map is completed. Of course, if the
underlying []byte is modified by another goroutine while the lookup is
taking place, the result of the lookup is going to be wrong,
but if you're modifying the []byte while parsing it as JSON (you
definitely shouldn't!) the deserialization is also going to be wrong.

Regards,
Alberto

https://codereview.appspot.com/12243043/

[Rob Pike]

Do not use the unsafe package just to gain performance in the standard libraries.

-rob

[Russ Cox]

Brad also has a pending CL to make json faster.

I think we all agree it can be faster. 

I'd like some more time to think about what the best approach is.

Thanks.

Russ

[alberto.ga...@gmail.com]

Brad's CL is about encoding, this one speeds up decoding.

Regards,
Alberto

https://codereview.appspot.com/12243043/

[Russ Cox]

Everything I wrote still applies. I didn't mention encoding vs decoding.

[alberto.ga...@gmail.com]

unsafe it's used by math and encoding/binary just to gain performance.
Why can't encoding/json use it to gain performance too?

https://codereview.appspot.com/12243043/

[Brad Fitzpatrick]

Any ETA on that thinking?  Or guidelines for what class of performance improvements (if any) are acceptable?

In the meantime I've just stopped doing things related to performance or garbage.

My JSON encoding CL just does what encoding/gob does, but without unsafe.  (caching per-Type encoders)

--

[Rob Pike]

Because I don't want any more of this in the library. Because I'd rather see the compiler improve. Because packages that use unsafe can't be deployed in some environments, which means we need two versions of the code, which makes the code harder to maintain and test. Because it's ugly and unreadable.

Most important, because it's unsafe.

-rob

[alberto.ga...@gmail.com]

On 2013/08/01 21:43:30, r wrote:
> Because I don't want any more of this in the library. Because I'd
rather
> see the compiler improve. Because packages that use unsafe can't be
> deployed in some environments, which means we need two versions of the
> code, which makes the code harder to maintain and test. Because it's
ugly
> and unreadable.

> Most important, because it's unsafe.

The only environment that I know of which doesn't allow package unsafe
to be used
is App Engine, and that's for user code, not standard library code
(otherwise, the math
package would have a safe version of Float32bits and the other functions
in math/unsafe.go).
Furthermore, I'm sure the majority of Go users don't care about App
Engine (but obviously Google does).

I'd also love the compiler to improve when it comes to []byte to string
conversion, but we have to be pragmatic
in the meantime. The usage of the unsafe package we're discussing
involves *one line*, which
speeds up the code significantly and reduces the number of allocations
by 50%, which in turn
benefits real world code immensely because the pressure on the GC is
reduced. Heck, most people
would call that an epic win.

In an ideal world, the compiler would produce a []byte to string
conversion without copying and/or the
runtime would have no problem dealing with the additional garbage
generated, but it's not the case right
now. By adding one ugly line we can ship a way better JSON decoder which
is gonna help a lot of
Go users. When the compiler and/or runtime improve, that line can be
changed to a simple cast and the
code will be beautiful again, and everyone will be happy.

In the meantime, I take full responsibility for that ugly line of code.
You can punch me in the face once for every
problem it causes down the line. And, yes, you can get that on writing,
signed by me.

Regards,
Alberto

https://codereview.appspot.com/12243043/

[alberto.ga...@gmail.com]

On 2013/08/01 15:24:06, rsc wrote:
> Everything I wrote still applies. I didn't mention encoding vs
decoding.

No worries, I got that. Just wanted to make clear that, while the two
CLs are about speeding up JSON, they touch different parts of the
package.

Regards,
Alberto

https://codereview.appspot.com/12243043/

[Rob Pike]

It was a mistake to export reflect.StructHeader. It's another mistake to build on that mistake. I stand by what I wrote before.

-rob