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>

1 files changed, 15 insertions, 13 deletions

diff --git a/src/encoding/json/decode.go b/src/encoding/json/decode.go
index 3900bcc165..3f9fe1f573 100644
--- 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
 }