From 74a92b8e8d0eae6bf9918ef16794b0363886713d Mon Sep 17 00:00:00 2001 From: "Pascal S. de Kloe" Date: Sat, 3 Mar 2018 15:20:26 +0100 Subject: [PATCH] encoding/json: apply conventional error handling in decoder MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit name old time/op new time/op delta CodeEncoder-12 1.89ms ± 1% 1.91ms ± 0% +1.16% (p=0.000 n=20+19) CodeMarshal-12 2.09ms ± 1% 2.12ms ± 0% +1.63% (p=0.000 n=17+18) CodeDecoder-12 8.43ms ± 1% 8.32ms ± 1% -1.35% (p=0.000 n=18+20) UnicodeDecoder-12 399ns ± 0% 339ns ± 0% -15.00% (p=0.000 n=20+19) DecoderStream-12 281ns ± 1% 231ns ± 0% -17.91% (p=0.000 n=20+16) CodeUnmarshal-12 9.35ms ± 2% 9.15ms ± 2% -2.11% (p=0.000 n=20+20) CodeUnmarshalReuse-12 8.41ms ± 2% 8.29ms ± 2% -1.34% (p=0.000 n=20+20) UnmarshalString-12 81.2ns ± 2% 74.0ns ± 4% -8.89% (p=0.000 n=20+20) UnmarshalFloat64-12 71.1ns ± 2% 64.3ns ± 1% -9.60% (p=0.000 n=20+19) UnmarshalInt64-12 60.6ns ± 2% 53.2ns ± 0% -12.28% (p=0.000 n=18+18) Issue10335-12 96.9ns ± 0% 87.7ns ± 1% -9.52% (p=0.000 n=17+20) Unmapped-12 247ns ± 4% 231ns ± 3% -6.34% (p=0.000 n=20+20) TypeFieldsCache/MissTypes1-12 11.1µs ± 0% 11.1µs ± 0% ~ (p=0.376 n=19+20) TypeFieldsCache/MissTypes10-12 33.9µs ± 0% 33.8µs ± 0% -0.32% (p=0.000 n=18+9) name old speed new speed delta CodeEncoder-12 1.03GB/s ± 1% 1.01GB/s ± 0% -1.15% (p=0.000 n=20+19) CodeMarshal-12 930MB/s ± 1% 915MB/s ± 0% -1.60% (p=0.000 n=17+18) CodeDecoder-12 230MB/s ± 1% 233MB/s ± 1% +1.37% (p=0.000 n=18+20) UnicodeDecoder-12 35.0MB/s ± 0% 41.2MB/s ± 0% +17.60% (p=0.000 n=20+19) CodeUnmarshal-12 208MB/s ± 2% 212MB/s ± 2% +2.16% (p=0.000 n=20+20) name old alloc/op new alloc/op delta Issue10335-12 184B ± 0% 184B ± 0% ~ (all equal) Unmapped-12 216B ± 0% 216B ± 0% ~ (all equal) name old allocs/op new allocs/op delta Issue10335-12 3.00 ± 0% 3.00 ± 0% ~ (all equal) Unmapped-12 4.00 ± 0% 4.00 ± 0% ~ (all equal) Change-Id: I4b1a87a205da2ef9a572f86f85bc833653c61570 Reviewed-on: https://go-review.googlesource.com/98440 Reviewed-by: Brad Fitzpatrick Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot --- src/encoding/json/decode.go | 234 +++++++++++++++++++----------------- src/encoding/json/encode.go | 5 + 2 files changed, 132 insertions(+), 107 deletions(-) diff --git a/src/encoding/json/decode.go b/src/encoding/json/decode.go index a096356b3d..d3ada54b69 100644 --- a/src/encoding/json/decode.go +++ b/src/encoding/json/decode.go @@ -167,22 +167,7 @@ func (e *InvalidUnmarshalError) Error() string { return "json: Unmarshal(nil " + e.Type.String() + ")" } -// jsonError is an error wrapper type for internal use only. -// Panics with errors are wrapped in jsonError so that the top-level recover -// can distinguish intentional panics from this package. -type jsonError struct{ error } - -func (d *decodeState) unmarshal(v interface{}) (err error) { - defer func() { - if r := recover(); r != nil { - if je, ok := r.(jsonError); ok { - err = je.error - } else { - panic(r) - } - } - }() - +func (d *decodeState) unmarshal(v interface{}) error { rv := reflect.ValueOf(v) if rv.Kind() != reflect.Ptr || rv.IsNil() { return &InvalidUnmarshalError{reflect.TypeOf(v)} @@ -192,7 +177,10 @@ func (d *decodeState) unmarshal(v interface{}) (err error) { d.scanWhile(scanSkipSpace) // We decode rv not rv.Elem because the Unmarshaler interface // test must be applied at the top level of the value. - d.value(rv) + err := d.value(rv) + if err != nil { + return err + } return d.savedError } @@ -306,11 +294,6 @@ func (d *decodeState) init(data []byte) *decodeState { return d } -// error aborts the decoding by panicking with err wrapped in jsonError. -func (d *decodeState) error(err error) { - panic(jsonError{d.addErrorContext(err)}) -} - // saveError saves the first err it is called with, // for reporting at the end of the unmarshal. func (d *decodeState) saveError(err error) { @@ -380,14 +363,16 @@ func (d *decodeState) scanWhile(op int) { // value consumes a JSON value from d.data[d.off-1:], decoding into v, and // reads the following byte ahead. If v is invalid, the value is discarded. // The first byte of the value has been read already. -func (d *decodeState) value(v reflect.Value) { +func (d *decodeState) value(v reflect.Value) error { switch d.opcode { default: - d.error(errPhase) + return errPhase case scanBeginArray: if v.IsValid() { - d.array(v) + if err := d.array(v); err != nil { + return err + } } else { d.skip() } @@ -395,7 +380,9 @@ func (d *decodeState) value(v reflect.Value) { case scanBeginObject: if v.IsValid() { - d.object(v) + if err := d.object(v); err != nil { + return err + } } else { d.skip() } @@ -407,9 +394,12 @@ func (d *decodeState) value(v reflect.Value) { d.scanWhile(scanContinue) if v.IsValid() { - d.literalStore(d.data[start:d.readIndex()], v, false) + if err := d.literalStore(d.data[start:d.readIndex()], v, false); err != nil { + return err + } } } + return nil } type unquotedValue struct{} @@ -418,10 +408,10 @@ type unquotedValue struct{} // quoted string literal or literal null into an interface value. // If it finds anything other than a quoted string literal or null, // valueQuoted returns unquotedValue{}. -func (d *decodeState) valueQuoted() interface{} { +func (d *decodeState) valueQuoted() (interface{}, error) { switch d.opcode { default: - d.error(errPhase) + return nil, errPhase case scanBeginArray: d.skip() @@ -432,12 +422,16 @@ func (d *decodeState) valueQuoted() interface{} { d.scanNext() case scanBeginLiteral: - switch v := d.literalInterface().(type) { + v, err := d.literalInterface() + if err != nil { + return nil, err + } + switch v.(type) { case nil, string: - return v + return v, nil } } - return unquotedValue{} + return unquotedValue{}, nil } // indirect walks down v allocating pointers as needed, @@ -511,22 +505,18 @@ func indirect(v reflect.Value, decodingNull bool) (Unmarshaler, encoding.TextUnm // array consumes an array from d.data[d.off-1:], decoding into v. // The first byte of the array ('[') has been read already. -func (d *decodeState) array(v reflect.Value) { +func (d *decodeState) array(v reflect.Value) error { // Check for unmarshaler. u, ut, pv := indirect(v, false) if u != nil { start := d.readIndex() d.skip() - err := u.UnmarshalJSON(d.data[start:d.off]) - if err != nil { - d.error(err) - } - return + return u.UnmarshalJSON(d.data[start:d.off]) } if ut != nil { d.saveError(&UnmarshalTypeError{Value: "array", Type: v.Type(), Offset: int64(d.off)}) d.skip() - return + return nil } v = pv @@ -535,15 +525,19 @@ func (d *decodeState) array(v reflect.Value) { case reflect.Interface: if v.NumMethod() == 0 { // Decoding into nil interface? Switch to non-reflect code. - v.Set(reflect.ValueOf(d.arrayInterface())) - return + ai, err := d.arrayInterface() + if err != nil { + return err + } + v.Set(reflect.ValueOf(ai)) + return nil } // Otherwise it's invalid. fallthrough default: d.saveError(&UnmarshalTypeError{Value: "array", Type: v.Type(), Offset: int64(d.off)}) d.skip() - return + return nil case reflect.Array: case reflect.Slice: break @@ -576,10 +570,14 @@ func (d *decodeState) array(v reflect.Value) { if i < v.Len() { // Decode into element. - d.value(v.Index(i)) + if err := d.value(v.Index(i)); err != nil { + return err + } } else { // Ran out of fixed array: skip. - d.value(reflect.Value{}) + if err := d.value(reflect.Value{}); err != nil { + return err + } } i++ @@ -591,7 +589,7 @@ func (d *decodeState) array(v reflect.Value) { break } if d.opcode != scanArrayValue { - d.error(errPhase) + return errPhase } } @@ -609,6 +607,7 @@ func (d *decodeState) array(v reflect.Value) { if i == 0 && v.Kind() == reflect.Slice { v.Set(reflect.MakeSlice(v.Type(), 0, 0)) } + return nil } var nullLiteral = []byte("null") @@ -616,29 +615,29 @@ var textUnmarshalerType = reflect.TypeOf(new(encoding.TextUnmarshaler)).Elem() // object consumes an object from d.data[d.off-1:], decoding into v. // The first byte ('{') of the object has been read already. -func (d *decodeState) object(v reflect.Value) { +func (d *decodeState) object(v reflect.Value) error { // Check for unmarshaler. u, ut, pv := indirect(v, false) if u != nil { start := d.readIndex() d.skip() - err := u.UnmarshalJSON(d.data[start:d.off]) - if err != nil { - d.error(err) - } - return + return u.UnmarshalJSON(d.data[start:d.off]) } if ut != nil { d.saveError(&UnmarshalTypeError{Value: "object", Type: v.Type(), Offset: int64(d.off)}) d.skip() - return + return nil } v = pv // Decoding into nil interface? Switch to non-reflect code. if v.Kind() == reflect.Interface && v.NumMethod() == 0 { - v.Set(reflect.ValueOf(d.objectInterface())) - return + oi, err := d.objectInterface() + if err != nil { + return err + } + v.Set(reflect.ValueOf(oi)) + return nil } // Check type of target: @@ -658,7 +657,7 @@ func (d *decodeState) object(v reflect.Value) { if !reflect.PtrTo(t.Key()).Implements(textUnmarshalerType) { d.saveError(&UnmarshalTypeError{Value: "object", Type: v.Type(), Offset: int64(d.off)}) d.skip() - return + return nil } } if v.IsNil() { @@ -669,7 +668,7 @@ func (d *decodeState) object(v reflect.Value) { default: d.saveError(&UnmarshalTypeError{Value: "object", Type: v.Type(), Offset: int64(d.off)}) d.skip() - return + return nil } var mapElem reflect.Value @@ -682,7 +681,7 @@ func (d *decodeState) object(v reflect.Value) { break } if d.opcode != scanBeginLiteral { - d.error(errPhase) + return errPhase } // Read key. @@ -691,7 +690,7 @@ func (d *decodeState) object(v reflect.Value) { item := d.data[start:d.readIndex()] key, ok := unquoteBytes(item) if !ok { - d.error(errPhase) + return errPhase } // Figure out field corresponding to key. @@ -756,21 +755,31 @@ func (d *decodeState) object(v reflect.Value) { d.scanWhile(scanSkipSpace) } if d.opcode != scanObjectKey { - d.error(errPhase) + return errPhase } d.scanWhile(scanSkipSpace) if destring { - switch qv := d.valueQuoted().(type) { + q, err := d.valueQuoted() + if err != nil { + return err + } + switch qv := q.(type) { case nil: - d.literalStore(nullLiteral, subv, false) + if err := d.literalStore(nullLiteral, subv, false); err != nil { + return err + } case string: - d.literalStore([]byte(qv), subv, true) + if err := d.literalStore([]byte(qv), subv, true); err != nil { + return err + } default: d.saveError(fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal unquoted value into %v", subv.Type())) } } else { - d.value(subv) + if err := d.value(subv); err != nil { + return err + } } // Write value back to map; @@ -783,7 +792,9 @@ func (d *decodeState) object(v reflect.Value) { kv = reflect.ValueOf(key).Convert(kt) case reflect.PtrTo(kt).Implements(textUnmarshalerType): kv = reflect.New(v.Type().Key()) - d.literalStore(item, kv, true) + if err := d.literalStore(item, kv, true); err != nil { + return err + } kv = kv.Elem() default: switch kt.Kind() { @@ -792,7 +803,7 @@ func (d *decodeState) object(v reflect.Value) { n, err := strconv.ParseInt(s, 10, 64) if err != nil || reflect.Zero(kt).OverflowInt(n) { d.saveError(&UnmarshalTypeError{Value: "number " + s, Type: kt, Offset: int64(start + 1)}) - return + return nil } kv = reflect.ValueOf(n).Convert(kt) case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: @@ -800,7 +811,7 @@ func (d *decodeState) object(v reflect.Value) { n, err := strconv.ParseUint(s, 10, 64) if err != nil || reflect.Zero(kt).OverflowUint(n) { d.saveError(&UnmarshalTypeError{Value: "number " + s, Type: kt, Offset: int64(start + 1)}) - return + return nil } kv = reflect.ValueOf(n).Convert(kt) default: @@ -818,12 +829,13 @@ func (d *decodeState) object(v reflect.Value) { break } if d.opcode != scanObjectValue { - d.error(errPhase) + return errPhase } d.errorContext.Struct = "" d.errorContext.Field = "" } + return nil } // convertNumber converts the number literal s to a float64 or a Number @@ -846,21 +858,21 @@ var numberType = reflect.TypeOf(Number("")) // fromQuoted indicates whether this literal came from unwrapping a // string from the ",string" struct tag option. this is used only to // produce more helpful error messages. -func (d *decodeState) literalStore(item []byte, v reflect.Value, fromQuoted bool) { +func (d *decodeState) literalStore(item []byte, v reflect.Value, fromQuoted bool) error { // Check for unmarshaler. if len(item) == 0 { //Empty string given d.saveError(fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal %q into %v", item, v.Type())) - return + return nil } isNull := item[0] == 'n' // null u, ut, pv := indirect(v, isNull) if u != nil { err := u.UnmarshalJSON(item) if err != nil { - d.error(err) + return err } - return + return nil } if ut != nil { if item[0] != '"' { @@ -878,21 +890,21 @@ func (d *decodeState) literalStore(item []byte, v reflect.Value, fromQuoted bool } d.saveError(&UnmarshalTypeError{Value: val, Type: v.Type(), Offset: int64(d.readIndex())}) } - return + return nil } s, ok := unquoteBytes(item) if !ok { if fromQuoted { - d.error(fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal %q into %v", item, v.Type())) + return fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal %q into %v", item, v.Type()) } else { - d.error(errPhase) + return errPhase } } err := ut.UnmarshalText(s) if err != nil { - d.error(err) + return err } - return + return nil } v = pv @@ -939,9 +951,9 @@ func (d *decodeState) literalStore(item []byte, v reflect.Value, fromQuoted bool s, ok := unquoteBytes(item) if !ok { if fromQuoted { - d.error(fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal %q into %v", item, v.Type())) + return fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal %q into %v", item, v.Type()) } else { - d.error(errPhase) + return errPhase } } switch v.Kind() { @@ -972,9 +984,9 @@ func (d *decodeState) literalStore(item []byte, v reflect.Value, fromQuoted bool default: // number if c != '-' && (c < '0' || c > '9') { if fromQuoted { - d.error(fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal %q into %v", item, v.Type())) + return fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal %q into %v", item, v.Type()) } else { - d.error(errPhase) + return errPhase } } s := string(item) @@ -983,14 +995,14 @@ func (d *decodeState) literalStore(item []byte, v reflect.Value, fromQuoted bool if v.Kind() == reflect.String && v.Type() == numberType { v.SetString(s) if !isValidNumber(s) { - d.error(fmt.Errorf("json: invalid number literal, trying to unmarshal %q into Number", item)) + return fmt.Errorf("json: invalid number literal, trying to unmarshal %q into Number", item) } break } if fromQuoted { - d.error(fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal %q into %v", item, v.Type())) + return fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal %q into %v", item, v.Type()) } else { - d.error(&UnmarshalTypeError{Value: "number", Type: v.Type(), Offset: int64(d.readIndex())}) + return &UnmarshalTypeError{Value: "number", Type: v.Type(), Offset: int64(d.readIndex())} } case reflect.Interface: n, err := d.convertNumber(s) @@ -1029,6 +1041,7 @@ func (d *decodeState) literalStore(item []byte, v reflect.Value, fromQuoted bool v.SetFloat(n) } } + return nil } // The xxxInterface routines build up a value to be stored @@ -1036,25 +1049,24 @@ func (d *decodeState) literalStore(item []byte, v reflect.Value, fromQuoted bool // but they avoid the weight of reflection in this common case. // valueInterface is like value but returns interface{} -func (d *decodeState) valueInterface() (val interface{}) { +func (d *decodeState) valueInterface() (val interface{}, err error) { switch d.opcode { default: - d.error(errPhase) - panic("unreachable") + err = errPhase case scanBeginArray: - val = d.arrayInterface() + val, err = d.arrayInterface() d.scanNext() case scanBeginObject: - val = d.objectInterface() + val, err = d.objectInterface() d.scanNext() case scanBeginLiteral: - val = d.literalInterface() + val, err = d.literalInterface() } return } // arrayInterface is like array but returns []interface{}. -func (d *decodeState) arrayInterface() []interface{} { +func (d *decodeState) arrayInterface() ([]interface{}, error) { var v = make([]interface{}, 0) for { // Look ahead for ] - can only happen on first iteration. @@ -1063,7 +1075,11 @@ func (d *decodeState) arrayInterface() []interface{} { break } - v = append(v, d.valueInterface()) + vi, err := d.valueInterface() + if err != nil { + return nil, err + } + v = append(v, vi) // Next token must be , or ]. if d.opcode == scanSkipSpace { @@ -1073,14 +1089,14 @@ func (d *decodeState) arrayInterface() []interface{} { break } if d.opcode != scanArrayValue { - d.error(errPhase) + return nil, errPhase } } - return v + return v, nil } // objectInterface is like object but returns map[string]interface{}. -func (d *decodeState) objectInterface() map[string]interface{} { +func (d *decodeState) objectInterface() (map[string]interface{}, error) { m := make(map[string]interface{}) for { // Read opening " of string key or closing }. @@ -1090,7 +1106,7 @@ func (d *decodeState) objectInterface() map[string]interface{} { break } if d.opcode != scanBeginLiteral { - d.error(errPhase) + return nil, errPhase } // Read string key. @@ -1099,7 +1115,7 @@ func (d *decodeState) objectInterface() map[string]interface{} { item := d.data[start:d.readIndex()] key, ok := unquote(item) if !ok { - d.error(errPhase) + return nil, errPhase } // Read : before value. @@ -1107,12 +1123,16 @@ func (d *decodeState) objectInterface() map[string]interface{} { d.scanWhile(scanSkipSpace) } if d.opcode != scanObjectKey { - d.error(errPhase) + return nil, errPhase } d.scanWhile(scanSkipSpace) // Read value. - m[key] = d.valueInterface() + vi, err := d.valueInterface() + if err != nil { + return nil, err + } + m[key] = vi // Next token must be , or }. if d.opcode == scanSkipSpace { @@ -1122,16 +1142,16 @@ func (d *decodeState) objectInterface() map[string]interface{} { break } if d.opcode != scanObjectValue { - d.error(errPhase) + return nil, errPhase } } - return m + return m, nil } // literalInterface consumes and returns a literal from d.data[d.off-1:] and // it reads the following byte ahead. The first byte of the literal has been // read already (that's how the caller knows it's a literal). -func (d *decodeState) literalInterface() interface{} { +func (d *decodeState) literalInterface() (interface{}, error) { // All bytes inside literal return scanContinue op code. start := d.readIndex() d.scanWhile(scanContinue) @@ -1140,27 +1160,27 @@ func (d *decodeState) literalInterface() interface{} { switch c := item[0]; c { case 'n': // null - return nil + return nil, nil case 't', 'f': // true, false - return c == 't' + return c == 't', nil case '"': // string s, ok := unquote(item) if !ok { - d.error(errPhase) + return nil, errPhase } - return s + return s, nil default: // number if c != '-' && (c < '0' || c > '9') { - d.error(errPhase) + return nil, errPhase } n, err := d.convertNumber(string(item)) if err != nil { d.saveError(err) } - return n + return n, nil } } diff --git a/src/encoding/json/encode.go b/src/encoding/json/encode.go index e7e7c4b7ef..46aa78a70b 100644 --- a/src/encoding/json/encode.go +++ b/src/encoding/json/encode.go @@ -281,6 +281,11 @@ func newEncodeState() *encodeState { return new(encodeState) } +// jsonError is an error wrapper type for internal use only. +// Panics with errors are wrapped in jsonError so that the top-level recover +// can distinguish intentional panics from this package. +type jsonError struct{ error } + func (e *encodeState) marshal(v interface{}, opts encOpts) (err error) { defer func() { if r := recover(); r != nil { -- 2.48.1