[go] fmt: cleanup general reflect value handling

55 views
Skip to first unread message

Martin Möhrmann (Gerrit)

unread,
Mar 20, 2016, 5:00:33 PM3/20/16
to Rob Pike, Ian Lance Taylor, golang-co...@googlegroups.com
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>

Martin Möhrmann (Gerrit)

unread,
Mar 20, 2016, 5:06:23 PM3/20/16
to Rob Pike, golang-co...@googlegroups.com
Martin Möhrmann has posted comments on this change.

fmt: cleanup general reflect value handling

Patch Set 1:

(2 comments)

https://go-review.googlesource.com/#/c/20923/1/src/fmt/fmt_test.go
File src/fmt/fmt_test.go:

Line 1000: {"%☠",
reflect.ValueOf(renamedInt(0)), "%!☠(fmt_test.renamedInt=0)"},
before this would print as: "%!☠(reflect.Value=0)"


Line 1005: {"%☠", map[float64]int{NaN:
1}, "map[%!☠(float64=NaN):%!☠(<invalid reflect.Value>)]"},
before this would print
as: "map[%!☠(float64=NaN):%!☠(map[float64]int=map[NaN:<nil>])]"


--
https://go-review.googlesource.com/20923
Gerrit-Reviewer: Martin Möhrmann <mart...@uos.de>
Gerrit-Reviewer: Rob Pike <r...@golang.org>
Gerrit-HasComments: Yes

Martin Möhrmann (Gerrit)

unread,
Mar 20, 2016, 5:09:48 PM3/20/16
to Rob Pike, golang-co...@googlegroups.com
Martin Möhrmann uploaded a new patch set:
other followup CL to find a new sweet spot 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(-)


Rob Pike (Gerrit)

unread,
Mar 20, 2016, 5:30:13 PM3/20/16
to Martin Möhrmann, Rob Pike, golang-co...@googlegroups.com
Rob Pike has posted comments on this change.

fmt: cleanup general reflect value handling

Patch Set 2:

I would prefer to find that "sweet spot" so I am not reviewing a CL that
slows down a very common operation.

--
https://go-review.googlesource.com/20923
Gerrit-Reviewer: Martin Möhrmann <mart...@uos.de>
Gerrit-Reviewer: Rob Pike <r...@golang.org>
Gerrit-HasComments: No

Martin Möhrmann (Gerrit)

unread,
Mar 20, 2016, 5:47:30 PM3/20/16
to Rob Pike, golang-co...@googlegroups.com
Martin Möhrmann has posted comments on this change.

fmt: cleanup general reflect value handling

Patch Set 2:

> I would prefer to find that "sweet spot" so I am not reviewing a CL
> that slows down a very common operation.

Would it be ok for now to leave an unused field as filler in the struct
with a comment?

I have not yet been able yet to find a reordering on my intel haswell that
does not regress performance back to a level before
https://go-review.googlesource.com/#/c/20838/.

I will have a look with another CPU and change the CL to not cause a slow
down either way.

Rob Pike (Gerrit)

unread,
Mar 20, 2016, 7:54:42 PM3/20/16
to Martin Möhrmann, Rob Pike, golang-co...@googlegroups.com
Rob Pike has posted comments on this change.

fmt: cleanup general reflect value handling

Patch Set 2:

Padding would work but i suspect a reordering would do fine.

Martin Möhrmann (Gerrit)

unread,
Mar 21, 2016, 2:21:55 AM3/21/16
to Rob Pike, golang-co...@googlegroups.com
Martin Möhrmann has posted comments on this change.

fmt: cleanup general reflect value handling

Patch Set 2:

> Padding would work but i suspect a reordering would do fine.

I found that it is the (cache) size. With reflect the pp struct is exactly
3 cache lines with 64 byte. Removing reflect.Value shrunk it to 168 bytes.
To counter this i increased internal buffer in fmt to 96 bytes so it ends
up at 192 byte again. Which should be better than wasting that space on
padding. This recovers the performance loss. Will update the test
cases/comments for overflow of intbuf and adjust the cl.

Martin Möhrmann (Gerrit)

unread,
Mar 21, 2016, 2:26:54 AM3/21/16
to Rob Pike, golang-co...@googlegroups.com
Martin Möhrmann has posted comments on this change.

fmt: cleanup general reflect value handling

Patch Set 2:

Of course there could also be other effects than e.g. cache line sharing
why this helps e.g. 192byte might use different code or memory pool in the
allocator.

Martin Möhrmann (Gerrit)

unread,
Mar 22, 2016, 5:14:56 PM3/22/16
to Rob Pike, golang-co...@googlegroups.com
Martin Möhrmann uploaded a new patch set:
https://go-review.googlesource.com/20923

fmt: cleanup reflect value handling

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 ± 1% 231ns ± 1% +0.90% (p=0.001
n=20+1
SprintfString-2 113ns ± 0% 113ns ± 1% -0.37% (p=0.002
n=18+19)
SprintfTruncateString-2 140ns ± 0% 139ns ± 1% -0.89% (p=0.000
n=16+16)
SprintfQuoteString-2 409ns ± 0% 406ns ± 0% -0.73% (p=0.000
n=19+19)
SprintfInt-2 111ns ± 1% 113ns ± 2% +1.66% (p=0.000
n=20+19)
SprintfIntInt-2 175ns ± 3% 176ns ± 3% ~ (p=0.067
n=20+19)
SprintfPrefixedInt-2 262ns ± 2% 258ns ± 3% -1.53% (p=0.001
n=20+20)
SprintfFloat-2 186ns ± 1% 188ns ± 1% +1.01% (p=0.000
n=19+18)
SprintfComplex-2 529ns ± 1% 533ns ± 2% +0.75% (p=0.004
n=20+20)
SprintfBoolean-2 100ns ± 1% 100ns ± 1% +0.48% (p=0.003
n=20+18)
SprintfHexString-2 180ns ± 1% 180ns ± 1% ~ (p=0.311
n=19+17)
SprintfHexBytes-2 187ns ± 2% 188ns ± 1% +0.75% (p=0.030
n=20+18)
SprintfBytes-2 336ns ± 0% 342ns ± 1% +1.72% (p=0.000
n=19+18)
SprintfStringer-2 376ns ± 3% 378ns ± 3% ~ (p=0.379
n=20+20)
SprintfStructure-2 815ns ± 2% 778ns ± 0% -4.63% (p=0.000
n=20+17)
FprintInt-2 150ns ± 1% 150ns ± 0% +0.30% (p=0.001
n=20+18)
FprintfBytes-2 187ns ± 0% 187ns ± 0% ~ (all samples
are equal)
FprintIntNoAlloc-2 108ns ± 0% 108ns ± 0% -0.46% (p=0.000
n=20+20)

Change-Id: Ib9a39082ed1be0f1f7499ee6fb6c9530f043e43a
---
M src/fmt/fmt_test.go
M src/fmt/print.go
2 files changed, 66 insertions(+), 63 deletions(-)

Martin Möhrmann (Gerrit)

unread,
Mar 24, 2016, 7:39:56 AM3/24/16
to Rob Pike, golang-co...@googlegroups.com
Martin Möhrmann uploaded a new patch set:
https://go-review.googlesource.com/20923

fmt: cleanup reflect value handling

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
---
M src/fmt/fmt_test.go
M src/fmt/print.go
2 files changed, 66 insertions(+), 65 deletions(-)

Martin Möhrmann (Gerrit)

unread,
Mar 24, 2016, 7:41:27 AM3/24/16
to Rob Pike, golang-co...@googlegroups.com
Martin Möhrmann has posted comments on this change.

fmt: cleanup reflect value handling

Patch Set 4:

rebased to the latest fmt with updated performance numbers and replaced the
breaks by returns in the switch.

--
https://go-review.googlesource.com/20923
Gerrit-Reviewer: Martin Möhrmann <mart...@uos.de>
Gerrit-Reviewer: Rob Pike <r...@golang.org>
Gerrit-HasComments: No

Rob Pike (Gerrit)

unread,
Mar 26, 2016, 8:18:35 PM3/26/16
to Martin Möhrmann, Rob Pike, golang-co...@googlegroups.com
Rob Pike has posted comments on this change.

fmt: cleanup reflect value handling

Patch Set 4: Run-TryBot+1

Gobot Gobot (Gerrit)

unread,
Mar 26, 2016, 8:18:51 PM3/26/16
to Martin Möhrmann, Rob Pike, golang-co...@googlegroups.com
Gobot Gobot has posted comments on this change.

fmt: cleanup reflect value handling

Patch Set 4:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=6eac72f8

--
https://go-review.googlesource.com/20923
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gobot Gobot (Gerrit)

unread,
Mar 26, 2016, 8:20:50 PM3/26/16
to Martin Möhrmann, Rob Pike, golang-co...@googlegroups.com
Gobot Gobot has posted comments on this change.

fmt: cleanup reflect value handling

Patch Set 4:

Build is still in progress...
This change failed on plan9-386:
See
https://storage.googleapis.com/go-build-log/6eac72f8/plan9-386_c7954962.log

Consult https://build.golang.org/ to see whether it's a new failure. Other
builds still in progress; subsequent failure notices suppressed until final
report.

Gobot Gobot (Gerrit)

unread,
Mar 26, 2016, 8:40:47 PM3/26/16
to Martin Möhrmann, Rob Pike, golang-co...@googlegroups.com
Gobot Gobot has posted comments on this change.

fmt: cleanup reflect value handling

Patch Set 4: TryBot-Result-1

1 of 15 TryBots failed:
Failed on plan9-386:
https://storage.googleapis.com/go-build-log/6eac72f8/plan9-386_c7954962.log

Consult https://build.golang.org/ to see whether they are new failures.

Rob Pike (Gerrit)

unread,
Mar 26, 2016, 8:50:20 PM3/26/16
to Martin Möhrmann, Rob Pike, Gobot Gobot, golang-co...@googlegroups.com
Rob Pike has posted comments on this change.

fmt: cleanup reflect value handling

Patch Set 4: Code-Review+2

Much as I appreciate the improvements you have made, I hope we are
approaching the end of these CLs. I would like the code to settle for a
while.

Rob Pike (Gerrit)

unread,
Mar 26, 2016, 8:50:39 PM3/26/16
to Martin Möhrmann, Rob Pike, golang-...@googlegroups.com, Gobot Gobot, golang-co...@googlegroups.com
Rob Pike has submitted this change and it was merged.

fmt: cleanup reflect value handling

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 <r...@golang.org>
Reviewed-by: Rob Pike <r...@golang.org>
---
M src/fmt/fmt_test.go
M src/fmt/print.go
2 files changed, 66 insertions(+), 65 deletions(-)

Approvals:
Rob Pike: Looks good to me, approved; Run TryBots

Objections:
Gobot Gobot: TryBots failed
Reply all
Reply to author
Forward
0 new messages