code review 7068043: encoding/json: improve performance of Unmarshal on prim...

encoding/json: improve performance of Unmarshal on primitive types

Skip most of the scanning and parsing logic for simple (non-object/array) JSON values.

benchmark                   old ns/op    new ns/op    delta
BenchmarkUnmarshalInt             948          436  -54.01%
BenchmarkUnmarshalUint            930          427  -54.09%
BenchmarkUnmarshalString         1407          715  -49.18%
BenchmarkUnmarshalFloat          1114          536  -51.89%
BenchmarkUnmarshalBool            759          266  -64.95%
BenchmarkUnmarshalStruct         8165         8181   +0.20%

No significant effects on the go1 benchmarks:

benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17    9647362752   9596196417   -0.53%
BenchmarkFannkuch11      5623613048   5518694872   -1.87%
BenchmarkGobDecode         32944041     33165434   +0.67%
BenchmarkGobEncode         21237482     21080554   -0.74%
BenchmarkGzip             750955920    749861980   -0.15%
BenchmarkGunzip           197369742    197886192   +0.26%
BenchmarkJSONEncode        79274091     78891137   -0.48%
BenchmarkJSONDecode       180257802    175280358   -2.76%
BenchmarkMandelbrot200      7396666      7388266   -0.11%
BenchmarkParse             11446460     11386550   -0.52%
BenchmarkRevcomp         1605152523   1599512029   -0.35%
BenchmarkTemplate         204538247    207765574   +1.58%

benchmark                  old MB/s     new MB/s  speedup
BenchmarkGobDecode            23.30        23.14    0.99x
BenchmarkGobEncode            36.14        36.41    1.01x
BenchmarkGzip                 25.84        25.88    1.00x
BenchmarkGunzip               98.32        98.06    1.00x
BenchmarkJSONEncode           24.48        24.60    1.00x
BenchmarkJSONDecode           10.76        11.07    1.03x
BenchmarkParse                 5.06         5.09    1.01x
BenchmarkRevcomp             158.34       158.90    1.00x
BenchmarkTemplate              9.49         9.34    0.98x

Fixes issue 3949.

[rick, 2013-01-05 06:21:40]

Hello golang-dev@googlegroups.com,

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

[dave_cheney.net, 2013-01-05 06:25:47]

Awesome, can you also run a before/after for $GOROOT/test/bench/go1 ?

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

https://codereview.appspot.com/7068043/diff/3001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:58: for i := 0; i < len(data); i++ {
what about

for _, c := range data {
    first = c
    if first > ' ' { 
       break
    }
}

to avoid the range check

[rick, 2013-01-05 15:40:55]

PTAL

I updated the benchmark results and added go1 benchmarks to the description.

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

https://codereview.appspot.com/7068043/diff/3001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:58: for i := 0; i < len(data); i++ {
On 2013/01/05 06:25:47, dfc wrote:
> what about
> 
> for _, c := range data {
>     first = c
>     if first > ' ' { 
>        break
>     }
> }
> 
> to avoid the range check

Done

[bradfitz, 2013-01-09 23:16:38]

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

https://codereview.appspot.com/7068043/diff/3001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:60: if first > ' ' {
this means you'll now accept this:

	var v int
	err := json.Unmarshal([]byte("\x01 42"), &v)

which is currently rejected: http://play.golang.org/p/VGUk3tGr3K

[rick, 2013-01-10 00:25:56]

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

https://codereview.appspot.com/7068043/diff/3001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:60: if first > ' ' {
An error will still be created on line 687 (in literalStore) because the value
is not a number. I ran a test with this case and it still failed with the same
error as v1.0.

On 2013/01/09 23:16:39, bradfitz wrote:
> this means you'll now accept this:
> 
> 	var v int
> 	err := json.Unmarshal([]byte("\x01 42"), &v)
> 
> which is currently rejected: http://play.golang.org/p/VGUk3tGr3K
>

[rick, 2013-01-10 01:38:26]

PTAL

I did notice that cases with legitimate whitespace would throw an error and
fixed that.

I also added tests for ignoring whitespace and having errors on control
characters.

Thanks.

[bradfitz, 2013-01-10 20:36:05]

thanks for the new tests.

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

https://codereview.appspot.com/7068043/diff/8/src/pkg/encoding/json/decode.go...
src/pkg/encoding/json/decode.go:59: var c byte
drop this variable?

[timo1, 2013-01-10 22:07:44]

> src/pkg/encoding/json/decode.go:59: var c byte
> drop this variable?

Or turn it around completely:

	// skip heavy processing for primitive values
	for i, c := range data {
		if isSpace(rune(c)) {
			continue
		}
		if first == '{' || first == '[' {
			break
		}
		rv := reflect.ValueOf(v)
		if rv.Kind() != reflect.Ptr || rv.IsNil() {
			return &InvalidUnmarshalError{reflect.TypeOf(v)}
		}
		var d decodeState
		d.literalStore(data[i:], rv.Elem(), false)
		return d.savedError
	}

[timo1, 2013-01-10 22:25:21]

Very sorry, left my computer unlocked with 14 yr old around ... will not happen
again.
Didn't mean to post to this thread at all, I am not involved with Go
development.

ps suggestion is flawed (first should be c).

[rick, 2013-01-10 22:50:50]

PTAL

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

https://codereview.appspot.com/7068043/diff/8/src/pkg/encoding/json/decode.go...
src/pkg/encoding/json/decode.go:59: var c byte
On 2013/01/10 20:36:05, bradfitz wrote:
> drop this variable?

Done.

[bradfitz, 2013-01-11 01:49:12]

LGTM

On Thu, Jan 10, 2013 at 2:50 PM, <rickarnoldjr@gmail.com> wrote:

> PTAL
>
>
>
>
> https://codereview.appspot.**com/7068043/diff/8/src/pkg/**
>
encoding/json/decode.go<https://codereview.appspot.com/7068043/diff/8/src/pkg/encoding/json/decode.go>
> File src/pkg/encoding/json/decode.**go (right):
>
> https://codereview.appspot.**com/7068043/diff/8/src/pkg/**
>
encoding/json/decode.go#**newcode59<https://codereview.appspot.com/7068043/diff/8/src/pkg/encoding/json/decode.go#newcode59>
> src/pkg/encoding/json/decode.**go:59: var c byte
> On 2013/01/10 20:36:05, bradfitz wrote:
>
>> drop this variable?
>>
>
> Done.
>
>
https://codereview.appspot.**com/7068043/<https://codereview.appspot.com/7068...
>

[bradfitz, 2013-01-11 02:03:14]

(Submitted, but no email was sent?)


On Thu, Jan 10, 2013 at 5:49 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> LGTM
>
>
> On Thu, Jan 10, 2013 at 2:50 PM, <rickarnoldjr@gmail.com> wrote:
>
>> PTAL
>>
>>
>>
>>
>> https://codereview.appspot.**com/7068043/diff/8/src/pkg/**
>>
encoding/json/decode.go<https://codereview.appspot.com/7068043/diff/8/src/pkg/encoding/json/decode.go>
>> File src/pkg/encoding/json/decode.**go (right):
>>
>> https://codereview.appspot.**com/7068043/diff/8/src/pkg/**
>>
encoding/json/decode.go#**newcode59<https://codereview.appspot.com/7068043/diff/8/src/pkg/encoding/json/decode.go#newcode59>
>>  src/pkg/encoding/json/decode.**go:59: var c byte
>> On 2013/01/10 20:36:05, bradfitz wrote:
>>
>>> drop this variable?
>>>
>>
>> Done.
>>
>>
https://codereview.appspot.**com/7068043/<https://codereview.appspot.com/7068...
>>
>
>

[rick, 2013-01-11 03:14:07]

On 2013/01/11 02:03:14, bradfitz wrote:
> (Submitted, but no email was sent?)

I did an "hg sync" and I'm getting this:

CL 7068043 has no files; delete (abandon) with hg change -d 7068043

But it looks like my changes were applied in the source tree.

[dave_cheney.net, 2013-01-11 03:18:17]

Use -D despite the helpful message. Code review thinks the CL is owner by
someone else. 

On 11/01/2013, at 14:14, rickarnoldjr@gmail.com wrote:

> On 2013/01/11 02:03:14, bradfitz wrote:
>> (Submitted, but no email was sent?)
> 
> I did an "hg sync" and I'm getting this:
> 
> CL 7068043 has no files; delete (abandon) with hg change -d 7068043
> 
> But it looks like my changes were applied in the source tree.
> 
> 
> 
> https://codereview.appspot.com/7068043/

[rsc, 2013-01-29 21:08:48]

This breaks unmarshaling of malformed data. I will roll it back and reopen the
issue.