encoding/json: fix performance regression in the decoder

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>

diff --git a/src/encoding/json/bench_test.go b/src/encoding/json/bench_test.go
index 72cb349062c4a85c49bcc586c85c2f88813f04b7..c81ab8e9935f483bc424f6d2481f84f7e907ae57 100644 (file)
--- a/src/encoding/json/bench_test.go
+++ b/src/encoding/json/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/src/encoding/json/decode.go b/src/encoding/json/decode.go
index 3900bcc165c5644e0dff1b96b2ea9423cd3bb2fd..3f9fe1f573342aa83a184a5d681ffca303a555aa 100644 (file)
--- a/src/encoding/json/decode.go
+++ b/src/encoding/json/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/src/encoding/json/decode_test.go b/src/encoding/json/decode_test.go
index d99d65d7634114218dcce8ee32498a8ae157bbb3..8da74fa3d35f30d928f4e55db0e4dbfae5adb071 100644 (file)
--- a/src/encoding/json/decode_test.go
+++ b/src/encoding/json/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) {