Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(144)

Issue 9360043: code review 9360043: cmd/gc: avoid a temporary when inlining s = append(s, x)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 11 months ago by remyoudompheng
Modified:
5 years, 8 months ago
CC:
golang-dev
Visibility:
Public.

Description

cmd/gc: avoid a temporary when inlining s = append(s, x) pkg strconv: benchmark old ns/op new ns/op delta BenchmarkAppendFloatDecimal 203 198 -2.46% BenchmarkAppendFloat 363 348 -4.13% BenchmarkAppendFloatExp 335 307 -8.36% BenchmarkAppendFloatNegExp 335 318 -5.07% BenchmarkAppendFloatBig 526 480 -8.75% BenchmarkAppendFloat32Integer 203 216 +6.40% BenchmarkAppendFloat32ExactFraction 307 293 -4.56% BenchmarkAppendFloat32Point 367 349 -4.90% BenchmarkAppendFloat32Exp 318 302 -5.03% BenchmarkAppendFloat32NegExp 322 306 -4.97% BenchmarkAppendFloat64Fixed1 243 247 +1.65% BenchmarkAppendFloat64Fixed2 283 293 +3.53% BenchmarkAppendFloat64Fixed3 248 264 +6.45% BenchmarkAppendFloat64Fixed4 287 277 -3.48% BenchmarkAppendInt 5676 5640 -0.63% BenchmarkAppendUint 1448 1445 -0.21% pkg time: benchmark old ns/op new ns/op delta BenchmarkFormat 876 884 +0.91% BenchmarkFormatNow 832 845 +1.56% Stack size is reduced in many cases: strconv before: (src/pkg/strconv/ftoa.go:334) TEXT fmtE+0(SB),$344-120 (src/pkg/strconv/ftoa.go:401) TEXT fmtF+0(SB),$232-112 (src/pkg/strconv/quote.go:13) TEXT quoteWith+0(SB),$560-40 (src/pkg/strconv/quote.go:293) TEXT Unquote+0(SB),$200-48 strconv after: (src/pkg/strconv/ftoa.go:334) TEXT fmtE+0(SB),$104-120 (src/pkg/strconv/ftoa.go:401) TEXT fmtF+0(SB),$88-112 (src/pkg/strconv/quote.go:13) TEXT quoteWith+0(SB),$208-40 (src/pkg/strconv/quote.go:293) TEXT Unquote+0(SB),$176-48 time before: (src/pkg/time/format.go:290) TEXT appendUint+0(SB),$272-64 (src/pkg/time/format.go:341) TEXT formatNano+0(SB),$152-72 (src/pkg/time/format.go:382) TEXT Time.Format+0(SB),$720-56 time after: (src/pkg/time/format.go:290) TEXT appendUint+0(SB),$200-64 (src/pkg/time/format.go:341) TEXT formatNano+0(SB),$128-72 (src/pkg/time/format.go:382) TEXT Time.Format+0(SB),$416-56

Patch Set 1 #

Patch Set 2 : diff -r 30c566874b83 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 30c566874b83 https://go.googlecode.com/hg/ #

Total comments: 6

Patch Set 4 : diff -r bce231eb0fdd https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -5 lines) Patch
M src/cmd/gc/walk.c View 1 2 3 5 chunks +17 lines, -5 lines 0 comments Download

Messages

Total messages: 11
remyoudompheng
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
5 years, 11 months ago (2013-05-12 15:11:21 UTC) #1
bradfitz
https://codereview.appspot.com/9360043/diff/4001/src/cmd/gc/walk.c File src/cmd/gc/walk.c (right): https://codereview.appspot.com/9360043/diff/4001/src/cmd/gc/walk.c#newcode2505 src/cmd/gc/walk.c:2505: // The caller may supply its own storage space ...
5 years, 11 months ago (2013-05-12 17:58:36 UTC) #2
DMorsing
https://codereview.appspot.com/9360043/diff/4001/src/cmd/gc/walk.c File src/cmd/gc/walk.c (right): https://codereview.appspot.com/9360043/diff/4001/src/cmd/gc/walk.c#newcode573 src/cmd/gc/walk.c:573: // optimization of the common pattern x = append(x, ...
5 years, 11 months ago (2013-05-12 18:33:13 UTC) #3
remyoudompheng
Hello golang-dev@googlegroups.com, bradfitz@golang.org, daniel.morsing@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
5 years, 11 months ago (2013-05-18 08:36:01 UTC) #4
remyoudompheng
https://codereview.appspot.com/9360043/diff/4001/src/cmd/gc/walk.c File src/cmd/gc/walk.c (right): https://codereview.appspot.com/9360043/diff/4001/src/cmd/gc/walk.c#newcode573 src/cmd/gc/walk.c:573: // optimization of the common pattern x = append(x, ...
5 years, 11 months ago (2013-05-18 08:36:03 UTC) #5
DMorsing
LGTM On 2013/05/18 08:36:03, remyoudompheng wrote: > https://codereview.appspot.com/9360043/diff/4001/src/cmd/gc/walk.c > File src/cmd/gc/walk.c (right): > > https://codereview.appspot.com/9360043/diff/4001/src/cmd/gc/walk.c#newcode573 ...
5 years, 11 months ago (2013-05-18 11:08:42 UTC) #6
dfc
The results aren't quite so impressive on linux/arm (chromebook) benchmark old ns/op new ns/op delta ...
5 years, 11 months ago (2013-05-19 02:02:37 UTC) #7
bradfitz
Remy, ping. Status of this?
5 years, 9 months ago (2013-07-23 16:46:54 UTC) #8
rsc
I think this ends up being correct, because of the calls to cheapexpr, but I ...
5 years, 8 months ago (2013-08-01 21:12:54 UTC) #9
remyoudompheng
On 2013/08/01 21:12:54, rsc wrote: > I think this ends up being correct, because of ...
5 years, 8 months ago (2013-08-14 06:21:24 UTC) #10
remyoudompheng
5 years, 8 months ago (2013-08-14 06:22:09 UTC) #11
R=close

This would have been the source of endless weird potential corruption and the
temp merging optimization takes care much better of stack size reduction.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b