From 4d04418112f57c51ca77530d8afd1e7385c4e88a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Sun, 17 Mar 2019 22:45:30 +0000 Subject: [PATCH] encoding/json: fix performance regression in the decoder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Run-TryBot: Daniel Martí TryBot-Result: Gobot Gobot --- bench_test.go | 12 ++++++++++++ decode.go | 28 +++++++++++++++------------- decode_test.go | 14 +++++++++++++- 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/bench_test.go b/bench_test.go index 72cb349..c81ab8e 100644 --- a/bench_test.go +++ b/bench_test.go @@ -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 diff --git a/decode.go b/decode.go index 3900bcc..3f9fe1f 100644 --- a/decode.go +++ b/decode.go @@ -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 } diff --git a/decode_test.go b/decode_test.go index d99d65d..8da74fa 100644 --- a/decode_test.go +++ b/decode_test.go @@ -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) {