qbit/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-11-17 14:24:50 -07:00
Code Releases Activity

encoding/json: fix performance regression in the decoder

Browse Source 
In golang.org/cl/145218, a feature was added where the JSON decoder
would keep track of the entire path to a field when reporting an
UnmarshalTypeError.

However, we all failed to check if this affected the benchmarks - myself
included, as a reviewer. Below are the numbers comparing the CL's parent
with itself, once it was merged:

name           old time/op    new time/op    delta
CodeDecoder-8    12.9ms ± 1%    28.2ms ± 2%   +119.33%  (p=0.002 n=6+6)

name           old speed      new speed      delta
CodeDecoder-8   151MB/s ± 1%    69MB/s ± 3%    -54.40%  (p=0.002 n=6+6)

name           old alloc/op   new alloc/op   delta
CodeDecoder-8    2.74MB ± 0%  109.39MB ± 0%  +3891.83%  (p=0.002 n=6+6)

name           old allocs/op  new allocs/op  delta
CodeDecoder-8     77.5k ± 0%    168.5k ± 0%   +117.30%  (p=0.004 n=6+5)

The reason why the decoder got twice as slow is because it now allocated
~40x as many objects, which puts a lot of pressure on the garbage
collector.

The reason is that the CL concatenated strings every time a nested field
was decoded. In other words, practically every field generated garbage
when decoded. This is hugely wasteful, especially considering that the
vast majority of JSON decoding inputs won't return UnmarshalTypeError.

Instead, use a stack of fields, and make sure to always use the same
backing array, to ensure we only need to grow the slice to the maximum
depth once.

The original CL also introduced a bug. The field stack string wasn't
reset to its original state when reaching "d.opcode == scanEndObject",
so the last field in a decoded struct could leak. For example, an added
test decodes a list of structs, and encoding/json before this CL would
fail:

	got:  cannot unmarshal string into Go struct field T.Ts.Y.Y.Y of type int
	want: cannot unmarshal string into Go struct field T.Ts.Y of type int

To fix that, simply reset the stack after decoding every field, even if
it's the last.

Below is the original performance versus this CL. There's a tiny
performance hit, probably due to the append for every decoded field, but
at least we're back to the usual ~150MB/s.

name           old time/op    new time/op    delta
CodeDecoder-8    12.9ms ± 1%    13.0ms ± 1%  +1.25%  (p=0.009 n=6+6)

name           old speed      new speed      delta
CodeDecoder-8   151MB/s ± 1%   149MB/s ± 1%  -1.24%  (p=0.009 n=6+6)

name           old alloc/op   new alloc/op   delta
CodeDecoder-8    2.74MB ± 0%    2.74MB ± 0%  +0.00%  (p=0.002 n=6+6)

name           old allocs/op  new allocs/op  delta
CodeDecoder-8     77.5k ± 0%     77.5k ± 0%  +0.00%  (p=0.002 n=6+6)

Finally, make all of these benchmarks report allocs by default. The
decoder ones are pretty sensitive to generated garbage, so ReportAllocs
would have made the performance regression more obvious.

Change-Id: I67b50f86b2e72f55539429450c67bfb1a9464b67
Reviewed-on: https://go-review.googlesource.com/c/go/+/167978
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This commit is contained in:
Daniel Martí 2019-03-17 22:45:30 +00:00
parent 3496ff1d19
commit e5f6e2d1c8
3 changed files with 40 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
src/encoding/json/bench_test.go
View File

@ -82,6 +82,7 @@ func codeInit() {
}
func BenchmarkCodeEncoder(b *testing.B) {
b.ReportAllocs()
if codeJSON == nil {
b.StopTimer()
codeInit()
@ -99,6 +100,7 @@ func BenchmarkCodeEncoder(b *testing.B) {
}
func BenchmarkCodeMarshal(b *testing.B) {
b.ReportAllocs()
if codeJSON == nil {
b.StopTimer()
codeInit()
@ -133,6 +135,7 @@ func benchMarshalBytes(n int) func(*testing.B) {
}
func BenchmarkMarshalBytes(b *testing.B) {
b.ReportAllocs()
// 32 fits within encodeState.scratch.
b.Run("32", benchMarshalBytes(32))
// 256 doesn't fit in encodeState.scratch, but is small enough to
@ -143,6 +146,7 @@ func BenchmarkMarshalBytes(b *testing.B) {
}
func BenchmarkCodeDecoder(b *testing.B) {
b.ReportAllocs()
if codeJSON == nil {
b.StopTimer()
codeInit()
@ -167,6 +171,7 @@ func BenchmarkCodeDecoder(b *testing.B) {
}
func BenchmarkUnicodeDecoder(b *testing.B) {
b.ReportAllocs()
j := []byte(`"\uD83D\uDE01"`)
b.SetBytes(int64(len(j)))
r := bytes.NewReader(j)
@ -182,6 +187,7 @@ func BenchmarkUnicodeDecoder(b *testing.B) {
}
func BenchmarkDecoderStream(b *testing.B) {
b.ReportAllocs()
b.StopTimer()
var buf bytes.Buffer
dec := NewDecoder(&buf)
@ -204,6 +210,7 @@ func BenchmarkDecoderStream(b *testing.B) {
}
func BenchmarkCodeUnmarshal(b *testing.B) {
b.ReportAllocs()
if codeJSON == nil {
b.StopTimer()
codeInit()
@ -221,6 +228,7 @@ func BenchmarkCodeUnmarshal(b *testing.B) {
}
func BenchmarkCodeUnmarshalReuse(b *testing.B) {
b.ReportAllocs()
if codeJSON == nil {
b.StopTimer()
codeInit()
@ -238,6 +246,7 @@ func BenchmarkCodeUnmarshalReuse(b *testing.B) {
}
func BenchmarkUnmarshalString(b *testing.B) {
b.ReportAllocs()
data := []byte(`"hello, world"`)
b.RunParallel(func(pb *testing.PB) {
var s string
@ -250,6 +259,7 @@ func BenchmarkUnmarshalString(b *testing.B) {
}
func BenchmarkUnmarshalFloat64(b *testing.B) {
b.ReportAllocs()
data := []byte(`3.14`)
b.RunParallel(func(pb *testing.PB) {
var f float64
@ -262,6 +272,7 @@ func BenchmarkUnmarshalFloat64(b *testing.B) {
}
func BenchmarkUnmarshalInt64(b *testing.B) {
b.ReportAllocs()
data := []byte(`3`)
b.RunParallel(func(pb *testing.PB) {
var x int64
@ -300,6 +311,7 @@ func BenchmarkUnmapped(b *testing.B) {
}
func BenchmarkTypeFieldsCache(b *testing.B) {
b.ReportAllocs()
var maxTypes int = 1e6
if testenv.Builder() != "" {
maxTypes = 1e3 // restrict cache sizes on builders

28
src/encoding/json/decode.go
View File

@ -14,6 +14,7 @@ import (
"fmt"
"reflect"
"strconv"
"strings"
"unicode"
"unicode/utf16"
"unicode/utf8"
@ -266,8 +267,8 @@ type decodeState struct {
opcode int // last read result
scan scanner
errorContext struct { // provides context for type errors
Struct reflect.Type
Field string
Struct reflect.Type
FieldStack []string
}
savedError error
useNumber bool
@ -289,7 +290,9 @@ func (d *decodeState) init(data []byte) *decodeState {
d.off = 0
d.savedError = nil
d.errorContext.Struct = nil
d.errorContext.Field = ""
// Reuse the allocated space for the FieldStack slice.
d.errorContext.FieldStack = d.errorContext.FieldStack[:0]
return d
}
@ -303,11 +306,11 @@ func (d *decodeState) saveError(err error) {
// addErrorContext returns a new error enhanced with information from d.errorContext
func (d *decodeState) addErrorContext(err error) error {
if d.errorContext.Struct != nil || d.errorContext.Field != "" {
if d.errorContext.Struct != nil || len(d.errorContext.FieldStack) > 0 {
switch err := err.(type) {
case *UnmarshalTypeError:
err.Struct = d.errorContext.Struct.Name()
err.Field = d.errorContext.Field
err.Field = strings.Join(d.errorContext.FieldStack, ".")
return err
}
}
@ -659,7 +662,7 @@ func (d *decodeState) object(v reflect.Value) error {
}
var mapElem reflect.Value
originalErrorContext := d.errorContext
origErrorContext := d.errorContext
for {
// Read opening " of string key or closing }.
@ -730,11 +733,7 @@ func (d *decodeState) object(v reflect.Value) error {
}
subv = subv.Field(i)
}
if originalErrorContext.Field == "" {
d.errorContext.Field = f.name
} else {
d.errorContext.Field = originalErrorContext.Field + "." + f.name
}
d.errorContext.FieldStack = append(d.errorContext.FieldStack, f.name)
d.errorContext.Struct = t
} else if d.disallowUnknownFields {
d.saveError(fmt.Errorf("json: unknown field %q", key))
@ -814,14 +813,17 @@ func (d *decodeState) object(v reflect.Value) error {
if d.opcode == scanSkipSpace {
d.scanWhile(scanSkipSpace)
}
// Reset errorContext to its original state.
// Keep the same underlying array for FieldStack, to reuse the
// space and avoid unnecessary allocs.
d.errorContext.FieldStack = d.errorContext.FieldStack[:len(origErrorContext.FieldStack)]
d.errorContext.Struct = origErrorContext.Struct
if d.opcode == scanEndObject {
break
}
if d.opcode != scanObjectValue {
panic(phasePanicMsg)
}
d.errorContext = originalErrorContext
}
return nil
}

14
src/encoding/json/decode_test.go
View File

@ -50,7 +50,8 @@ type P struct {
}
type PP struct {
T T
T T
Ts []T
}
type SS string
@ -943,6 +944,17 @@ var unmarshalTests = []unmarshalTest{
Offset: 29,
},
},
{
in: `{"Ts": [{"Y": 1}, {"Y": 2}, {"Y": "bad-type"}]}`,
ptr: new(PP),
err: &UnmarshalTypeError{
Value: "string",
Struct: "T",
Field: "Ts.Y",
Type: reflect.TypeOf(int(0)),
Offset: 29,
},
},
}
func TestMarshal(t *testing.T) {