From d170d3edd77aa571e65ff51ff382b90e9b00f40b Mon Sep 17 00:00:00 2001 From: =?utf8?q?Martin=20Mo=CC=88hrmann?= Date: Sat, 19 Mar 2016 08:54:07 +0100 Subject: [PATCH] fmt: cleanup reflect value handling MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Merge printReflectValue into printValue. Determine if handleMethods was already called in printArg by checking if depth is 0. Do not call handleMethods on depth 0 again in printValue to not introduce a performance regression. handleMethods is called already in printArg to not introduce a performance penalty for top-level Stringer, GoStringer, Errors and Formatters by using reflect.ValueOf on them just to retrieve them again as interface{} values in printValue. Clear p.arg in printValue after handleMethods to print the type of the value inside the reflect.Value when a bad verb is encountered on the top level instead of printing "reflect.Value=" as the type of the argument. This also fixes a bug that incorrectly prints the whole map instead of just the value for a key if the returned value by the map for the key is an invalid reflect value. name old time/op new time/op delta SprintfPadding-2 229ns ± 2% 227ns ± 1% -0.50% (p=0.013 n=20+20) SprintfEmpty-2 36.4ns ± 6% 37.2ns ±14% ~ (p=0.091 n=18+20) SprintfString-2 102ns ± 1% 102ns ± 0% ~ (p=0.751 n=20+20) SprintfTruncateString-2 142ns ± 0% 141ns ± 1% -0.95% (p=0.000 n=16+20) SprintfQuoteString-2 389ns ± 0% 388ns ± 0% -0.12% (p=0.019 n=20+20) SprintfInt-2 100ns ± 2% 100ns ± 1% ~ (p=0.188 n=20+15) SprintfIntInt-2 155ns ± 3% 154ns ± 2% ~ (p=0.092 n=20+20) SprintfPrefixedInt-2 250ns ± 2% 251ns ± 3% ~ (p=0.559 n=20+20) SprintfFloat-2 177ns ± 2% 175ns ± 1% -1.30% (p=0.000 n=20+20) SprintfComplex-2 516ns ± 1% 510ns ± 1% -1.13% (p=0.000 n=19+16) SprintfBoolean-2 90.9ns ± 3% 90.6ns ± 1% ~ (p=0.193 n=19+19) SprintfHexString-2 171ns ± 1% 169ns ± 1% -1.44% (p=0.000 n=19+20) SprintfHexBytes-2 180ns ± 1% 180ns ± 1% ~ (p=0.060 n=19+18) SprintfBytes-2 330ns ± 1% 329ns ± 1% -0.42% (p=0.003 n=20+20) SprintfStringer-2 354ns ± 3% 352ns ± 3% ~ (p=0.525 n=20+19) SprintfStructure-2 804ns ± 3% 776ns ± 2% -3.56% (p=0.000 n=20+20) FprintInt-2 155ns ± 0% 151ns ± 1% -2.35% (p=0.000 n=19+20) FprintfBytes-2 169ns ± 0% 170ns ± 1% +0.81% (p=0.000 n=18+19) FprintIntNoAlloc-2 112ns ± 0% 109ns ± 1% -2.28% (p=0.000 n=20+20) Change-Id: Ib9a39082ed1be0f1f7499ee6fb6c9530f043e43a Reviewed-on: https://go-review.googlesource.com/20923 Run-TryBot: Rob Pike Reviewed-by: Rob Pike --- src/fmt/fmt_test.go | 26 ++++++++++++ src/fmt/print.go | 99 +++++++++++++++++---------------------------- 2 files changed, 63 insertions(+), 62 deletions(-) diff --git a/src/fmt/fmt_test.go b/src/fmt/fmt_test.go index be7299cdbc..ffa24995a5 100644 --- a/src/fmt/fmt_test.go +++ b/src/fmt/fmt_test.go @@ -977,6 +977,8 @@ var fmtTests = []struct { // invalid reflect.Value doesn't crash. {"%v", reflect.Value{}, ""}, + {"%v", &reflect.Value{}, ""}, + {"%v", SI{reflect.Value{}}, "{}"}, // Tests to check that not supported verbs generate an error string. {"%☠", nil, "%!☠()"}, @@ -995,6 +997,12 @@ var fmtTests = []struct { {"%☠", &intVar, "%!☠(*int=0xPTR)"}, {"%☠", make(chan int), "%!☠(chan int=0xPTR)"}, {"%☠", func() {}, "%!☠(func()=0xPTR)"}, + {"%☠", reflect.ValueOf(renamedInt(0)), "%!☠(fmt_test.renamedInt=0)"}, + {"%☠", SI{renamedInt(0)}, "{%!☠(fmt_test.renamedInt=0)}"}, + {"%☠", &[]interface{}{I(1), G(2)}, "&[%!☠(fmt_test.I=1) %!☠(fmt_test.G=2)]"}, + {"%☠", SI{&[]interface{}{I(1), G(2)}}, "{%!☠(*[]interface {}=&[1 2])}"}, + {"%☠", reflect.Value{}, ""}, + {"%☠", map[float64]int{NaN: 1}, "map[%!☠(float64=NaN):%!☠()]"}, } // zeroFill generates zero-filled strings of the specified width. The length @@ -1262,6 +1270,24 @@ func BenchmarkSprintfBytes(b *testing.B) { }) } +func BenchmarkSprintfStringer(b *testing.B) { + stringer := I(12345) + b.RunParallel(func(pb *testing.PB) { + for pb.Next() { + Sprintf("%v", stringer) + } + }) +} + +func BenchmarkSprintfStructure(b *testing.B) { + s := &[]interface{}{SI{12345}, map[int]string{0: "hello"}} + b.RunParallel(func(pb *testing.PB) { + for pb.Next() { + Sprintf("%#v", s) + } + }) +} + func BenchmarkManyArgs(b *testing.B) { b.RunParallel(func(pb *testing.PB) { var buf bytes.Buffer diff --git a/src/fmt/print.go b/src/fmt/print.go index c80223269b..6c64773a1b 100644 --- a/src/fmt/print.go +++ b/src/fmt/print.go @@ -657,57 +657,44 @@ func (p *pp) printArg(arg interface{}, verb rune) { case []byte: p.fmtBytes(f, verb, bytesString) case reflect.Value: - p.printReflectValue(f, verb, 0) - return + p.printValue(f, verb, 0) default: // If the type is not simple, it might have methods. - if p.handleMethods(verb) { - return + if !p.handleMethods(verb) { + // Need to use reflection, since the type had no + // interface methods that could be used for formatting. + p.printValue(reflect.ValueOf(f), verb, 0) } - // Need to use reflection - p.printReflectValue(reflect.ValueOf(arg), verb, 0) - return } - p.arg = nil } +var byteType = reflect.TypeOf(byte(0)) + // printValue is similar to printArg but starts with a reflect value, not an interface{} value. // It does not handle 'p' and 'T' verbs because these should have been already handled by printArg. func (p *pp) printValue(value reflect.Value, verb rune, depth int) { - if !value.IsValid() { - switch verb { - case 'v': - p.buf.WriteString(nilAngleString) - default: - p.badVerb(verb) - } - return - } - - // Handle values with special methods. - // Call always, even when arg == nil, because handleMethods clears p.fmt.plus for us. - p.arg = nil // Make sure it's cleared, for safety. - if value.CanInterface() { + // Handle values with special methods if not already handled by printArg (depth == 0). + if depth > 0 && value.IsValid() && value.CanInterface() { p.arg = value.Interface() + if p.handleMethods(verb) { + return + } } - if p.handleMethods(verb) { - return - } - - p.printReflectValue(value, verb, depth) -} - -var byteType = reflect.TypeOf(byte(0)) - -// printReflectValue is the fallback for both printArg and printValue. -// It uses reflect to print the value. -func (p *pp) printReflectValue(value reflect.Value, verb rune, depth int) { - oldValue := p.value + p.arg = nil p.value = value -BigSwitch: - switch f := value; f.Kind() { + + switch f := value; value.Kind() { case reflect.Invalid: - p.buf.WriteString(invReflectString) + if depth == 0 { + p.buf.WriteString(invReflectString) + } else { + switch verb { + case 'v': + p.buf.WriteString(nilAngleString) + default: + p.badVerb(verb) + } + } case reflect.Bool: p.fmtBool(f.Bool(), verb) case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: @@ -729,7 +716,7 @@ BigSwitch: p.buf.WriteString(f.Type().String()) if f.IsNil() { p.buf.WriteString(nilParenString) - break + return } p.buf.WriteByte('{') } else { @@ -755,12 +742,10 @@ BigSwitch: } case reflect.Struct: if p.fmt.sharpV { - p.buf.WriteString(value.Type().String()) + p.buf.WriteString(f.Type().String()) } p.buf.WriteByte('{') - v := f - t := v.Type() - for i := 0; i < v.NumField(); i++ { + for i := 0; i < f.NumField(); i++ { if i > 0 { if p.fmt.sharpV { p.buf.WriteString(commaSpaceString) @@ -769,12 +754,12 @@ BigSwitch: } } if p.fmt.plusV || p.fmt.sharpV { - if f := t.Field(i); f.Name != "" { - p.buf.WriteString(f.Name) + if name := f.Type().Field(i).Name; name != "" { + p.buf.WriteString(name) p.buf.WriteByte(':') } } - p.printValue(getField(v, i), verb, depth+1) + p.printValue(getField(f, i), verb, depth+1) } p.buf.WriteByte('}') case reflect.Interface: @@ -813,13 +798,13 @@ BigSwitch: } } p.fmtBytes(bytes, verb, typ.String()) - break + return } if p.fmt.sharpV { p.buf.WriteString(typ.String()) if f.Kind() == reflect.Slice && f.IsNil() { p.buf.WriteString(nilParenString) - break + return } p.buf.WriteByte('{') } else { @@ -841,32 +826,22 @@ BigSwitch: p.buf.WriteByte(']') } case reflect.Ptr: - v := f.Pointer() // pointer to array or slice or struct? ok at top level // but not embedded (avoid loops) - if v != 0 && depth == 0 { + if depth == 0 && f.Pointer() != 0 { switch a := f.Elem(); a.Kind() { - case reflect.Array, reflect.Slice: + case reflect.Array, reflect.Slice, reflect.Struct, reflect.Map: p.buf.WriteByte('&') p.printValue(a, verb, depth+1) - break BigSwitch - case reflect.Struct: - p.buf.WriteByte('&') - p.printValue(a, verb, depth+1) - break BigSwitch - case reflect.Map: - p.buf.WriteByte('&') - p.printValue(a, verb, depth+1) - break BigSwitch + return } } fallthrough case reflect.Chan, reflect.Func, reflect.UnsafePointer: - p.fmtPointer(value, verb) + p.fmtPointer(f, verb) default: p.unknownType(f) } - p.value = oldValue } // intFromArg gets the argNumth element of a. On return, isInt reports whether the argument has integer type. -- 2.48.1