Reviewers: Rob Pike
Martin Möhrmann would like you to review this change by Martin Möhrmann:
https://go-review.googlesource.com/20923
fmt: cleanup general reflect value handling
This CL focuses on simplifying code around the big reflect switch
in printReflectValue that deals with using reflect to print values.
Followup CLs will focus on further cleanup and improvement of the
reflect switch itself.
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.
Fix a bug that would incorrectly print the whole map instead of
just the value for a key if the returned value by the map for
the key was an invalid reflect value.
Remove p.value and instead use p.arg to store reflect values to be
printed in case an unsupported verb is encountered. Always unpack
reflect values to print the unsupported verb error string similar
to how normal printing handles reflect values. This changes the
behavior to print the real type of the value wrapped in a
reflect.Value if the value was supplied to an unsupported verb
instead of printing "reflect.Value=" inside the error string.
The removal of p.value has a negative effect on performance due
to struct alignment. We can revisit this in an extra CL after any
other followup CL to find a new sweetspot that regains the performance
introduced by the last change to the struct in CL 20838.
Remove getField helper function that would provide a fast path to
handle interface fields instead of calling printValue directly on
the field. Use a direct call to printValue for consistency with other
types such as maps, arrays, slices and pointers instead. A later CL
will focus on speeding up interface handling so that all these types
can benefit from such an optimization.
Cleanup and consolidate the code of reflect.Ptr case to avoid
the fallthrough and BigSwitch label.
name old time/op new time/op delta
SprintfStringer-2 367ns ± 2% 383ns ± 3% +4.20% (p=0.000 n=20+19)
Change-Id: Ib9a39082ed1be0f1f7499ee6fb6c9530f043e43a
---
M src/fmt/fmt_test.go
M src/fmt/print.go
2 files changed, 66 insertions(+), 80 deletions(-)
diff --git a/src/fmt/fmt_test.go b/src/fmt/fmt_test.go
index be7299c..bf167cf 100644
--- a/src/fmt/fmt_test.go
+++ b/src/fmt/fmt_test.go
@@ -977,6 +977,8 @@
// invalid reflect.Value doesn't crash.
{"%v", reflect.Value{}, "<invalid reflect.Value>"},
+ {"%v", &reflect.Value{}, "<invalid Value>"},
+ {"%v", SI{reflect.Value{}}, "{<invalid Value>}"},
// Tests to check that not supported verbs generate an error string.
{"%☠", nil, "%!☠(<nil>)"},
@@ -995,6 +997,13 @@
{"%☠", &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{}, "<invalid reflect.Value>"},
+ {"%☠", map[float64]int{NaN: 1}, "map[%!☠(float64=NaN):%!☠(<invalid
reflect.Value>)]"},
+ {"%☠", F(3), "<☠=F(3)>"}, // F is special and works for any verb.
}
// zeroFill generates zero-filled strings of the specified width. The
length
@@ -1262,6 +1271,15 @@
})
}
+func BenchmarkSprintfStringer(b *testing.B) {
+ stringer := I(12345)
+ b.RunParallel(func(pb *testing.PB) {
+ for pb.Next() {
+ Sprintf("%v", stringer)
+ }
+ })
+}
+
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 0064ab3..eec001e 100644
--- a/src/fmt/print.go
+++ b/src/fmt/print.go
@@ -108,9 +108,6 @@
buf buffer
// arg holds the current item, as an interface{}.
arg interface{}
- // value holds the current item, as a reflect.Value, and will be
- // the zero Value if the item has not been reflected.
- value reflect.Value
// reordered records whether the format string used argument reordering.
reordered bool
// goodArgNum records whether the most recent reordering directive was
valid.
@@ -139,7 +136,6 @@
}
p.buf = p.buf[:0]
p.arg = nil
- p.value = reflect.Value{}
ppFree.Put(p)
}
@@ -265,17 +261,6 @@
return s
}
-// getField gets the i'th field of the struct value.
-// If the field is itself is an interface, return a value for
-// the thing inside the interface, not the interface itself.
-func getField(v reflect.Value, i int) reflect.Value {
- val := v.Field(i)
- if val.Kind() == reflect.Interface && !val.IsNil() {
- val = val.Elem()
- }
- return val
-}
-
// tooLarge reports whether the magnitude of the integer is
// too large to be used as a formatting width or precision.
func tooLarge(x int) bool {
@@ -313,17 +298,21 @@
p.buf.WriteString(percentBangString)
p.buf.WriteRune(verb)
p.buf.WriteByte('(')
- switch {
- case p.arg != nil:
- p.buf.WriteString(reflect.TypeOf(p.arg).String())
- p.buf.WriteByte('=')
- p.printArg(p.arg, 'v')
- case p.value.IsValid():
- p.buf.WriteString(p.value.Type().String())
- p.buf.WriteByte('=')
- p.printValue(p.value, 'v', 0)
- default:
+ if p.arg == nil {
p.buf.WriteString(nilAngleString)
+ } else {
+ switch value := p.arg.(type) {
+ case reflect.Value:
+ if value.IsValid() {
+ p.buf.WriteString(value.Type().String())
+ p.buf.WriteByte('=')
+ }
+ p.printValue(value, 'v', 0)
+ default:
+ p.buf.WriteString(reflect.TypeOf(value).String())
+ p.buf.WriteByte('=')
+ p.printArg(value, 'v')
+ }
}
p.buf.WriteByte(')')
p.erroring = false
@@ -595,7 +584,6 @@
func (p *pp) printArg(arg interface{}, verb rune) {
p.arg = arg
- p.value = reflect.Value{}
if arg == nil {
switch verb {
@@ -657,57 +645,44 @@
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
-}
-
-// 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() {
- p.arg = value.Interface()
- }
- 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.value = value
-BigSwitch:
+// 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) {
+ p.arg = value
+
+ // 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
+ }
+ }
+
switch f := value; f.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:
@@ -774,7 +749,7 @@
p.buf.WriteByte(':')
}
}
- p.printValue(getField(v, i), verb, depth+1)
+ p.printValue(f.Field(i), verb, depth+1)
}
p.buf.WriteByte('}')
case reflect.Interface:
@@ -846,27 +821,20 @@
// but not embedded (avoid loops)
if v != 0 && depth == 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
+ default:
+ p.fmtPointer(value, verb)
}
+ } else {
+ p.fmtPointer(value, verb)
}
- fallthrough
case reflect.Chan, reflect.Func, reflect.UnsafePointer:
p.fmtPointer(value, 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.
--
https://go-review.googlesource.com/20923
Gerrit-Reviewer: Rob Pike <
r...@golang.org>