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

Issue 12815046: code review 12815046: cmd/gc, runtime: inline append in frontend. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by remyoudompheng
Modified:
10 years, 6 months ago
Reviewers:
khr, rsc
CC:
golang-dev, khr, cshapiro1, rsc, dfc
Visibility:
Public.

Description

cmd/gc, runtime: inline append in frontend. A new transformation during walk turns append calls into a combination of growslice and memmove. benchmark old ns/op new ns/op delta BenchmarkAppend 141 141 +0.00% BenchmarkAppend1Byte 18 11 -39.56% BenchmarkAppend4Bytes 19 10 -42.63% BenchmarkAppend7Bytes 18 10 -42.16% BenchmarkAppend8Bytes 18 10 -40.44% BenchmarkAppend15Bytes 19 11 -41.67% BenchmarkAppend16Bytes 19 11 -41.97% BenchmarkAppend32Bytes 23 14 -38.82% BenchmarkAppendStr1Byte 14 10 -23.78% BenchmarkAppendStr4Bytes 14 11 -21.13% BenchmarkAppendStr8Bytes 14 10 -25.17% BenchmarkAppendStr16Bytes 19 11 -41.45% BenchmarkAppendStr32Bytes 18 14 -19.44% BenchmarkAppendSpecialCase 62 63 +1.77%

Patch Set 1 #

Patch Set 2 : diff -r 9b8afda16f02 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 9b8afda16f02 https://go.googlecode.com/hg/ #

Total comments: 3

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

Total comments: 2

Patch Set 5 : diff -r 0f036b4b1da5 https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r a6d1a3f0411a https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+97 lines, -127 lines) Patch
M src/cmd/gc/builtin.c View 1 2 3 1 chunk +0 lines, -3 lines 0 comments Download
M src/cmd/gc/runtime.go View 1 2 3 1 chunk +0 lines, -5 lines 0 comments Download
M src/cmd/gc/walk.c View 1 2 3 4 2 chunks +97 lines, -13 lines 0 comments Download
M src/pkg/runtime/arch_386.h View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M src/pkg/runtime/arch_amd64.h View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M src/pkg/runtime/arch_arm.h View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M src/pkg/runtime/slice.c View 1 1 chunk +0 lines, -103 lines 0 comments Download

Messages

Total messages: 15
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/
10 years, 7 months ago (2013-08-18 16:53:50 UTC) #1
remyoudompheng
Note that the actual CLs touches only cmd/gc and runtime: src/cmd/gc/builtin.c | 3 -- src/cmd/gc/runtime.go ...
10 years, 7 months ago (2013-08-18 16:55:10 UTC) #2
khr
Looks good, make sure to address 9101048's comments first. No benchmarks? https://codereview.appspot.com/12815046/diff/5001/src/cmd/gc/walk.c File src/cmd/gc/walk.c (right): ...
10 years, 7 months ago (2013-08-27 20:33:13 UTC) #3
cshapiro1
https://codereview.appspot.com/12815046/diff/5001/src/cmd/5g/gsubr.c File src/cmd/5g/gsubr.c (right): https://codereview.appspot.com/12815046/diff/5001/src/cmd/5g/gsubr.c#newcode1369 src/cmd/5g/gsubr.c:1369: a->etype = simtype[TUINTPTR]; A uintptr is not a pointer ...
10 years, 7 months ago (2013-08-27 20:54:22 UTC) #4
rsc
will wait for the other CL so i can see the real diffs
10 years, 6 months ago (2013-09-06 19:37:51 UTC) #5
rsc
https://codereview.appspot.com/12815046/diff/5001/src/cmd/5g/gsubr.c File src/cmd/5g/gsubr.c (right): https://codereview.appspot.com/12815046/diff/5001/src/cmd/5g/gsubr.c#newcode1369 src/cmd/5g/gsubr.c:1369: a->etype = simtype[TUINTPTR]; On 2013/08/27 20:54:23, cshapiro1 wrote: > ...
10 years, 6 months ago (2013-09-06 20:11:54 UTC) #6
remyoudompheng
Hello golang-dev@googlegroups.com, khr@golang.org, cshapiro@google.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 6 months ago (2013-09-12 05:30:02 UTC) #7
remyoudompheng
Benchmark on linux/arm: benchmark old ns/op new ns/op delta BenchmarkAppend 425 425 +0.00% BenchmarkAppend1Byte 53 ...
10 years, 6 months ago (2013-09-12 05:50:19 UTC) #8
khr
LGTM. https://codereview.appspot.com/12815046/diff/26001/src/cmd/gc/walk.c File src/cmd/gc/walk.c (right): https://codereview.appspot.com/12815046/diff/26001/src/cmd/gc/walk.c#newcode2582 src/cmd/gc/walk.c:2582: // s = growslice(T, s, len(l2)) This allocates ...
10 years, 6 months ago (2013-09-12 21:56:34 UTC) #9
remyoudompheng
Hello golang-dev@googlegroups.com, khr@golang.org, cshapiro@google.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 6 months ago (2013-09-13 10:47:44 UTC) #10
remyoudompheng
https://codereview.appspot.com/12815046/diff/26001/src/cmd/gc/walk.c File src/cmd/gc/walk.c (right): https://codereview.appspot.com/12815046/diff/26001/src/cmd/gc/walk.c#newcode2582 src/cmd/gc/walk.c:2582: // s = growslice(T, s, len(l2)) On 2013/09/12 21:56:35, ...
10 years, 6 months ago (2013-09-13 10:49:36 UTC) #11
dfc
Could you please remail this CL % hg clpatch 12815046 abort: codereview issue 12815046 is ...
10 years, 6 months ago (2013-09-14 02:44:45 UTC) #12
remyoudompheng
Hello golang-dev@googlegroups.com, khr@golang.org, cshapiro@google.com, rsc@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 6 months ago (2013-09-15 06:47:43 UTC) #13
rsc
LGTM thanks for bearing with us.
10 years, 6 months ago (2013-09-17 00:30:08 UTC) #14
rsc
10 years, 6 months ago (2013-09-17 00:31:26 UTC) #15
*** Submitted as https://code.google.com/p/go/source/detail?r=c111e30c49a4 ***

cmd/gc, runtime: inline append in frontend.

A new transformation during walk turns append calls
into a combination of growslice and memmove.

benchmark                     old ns/op    new ns/op    delta
BenchmarkAppend                     141          141   +0.00%
BenchmarkAppend1Byte                 18           11  -39.56%
BenchmarkAppend4Bytes                19           10  -42.63%
BenchmarkAppend7Bytes                18           10  -42.16%
BenchmarkAppend8Bytes                18           10  -40.44%
BenchmarkAppend15Bytes               19           11  -41.67%
BenchmarkAppend16Bytes               19           11  -41.97%
BenchmarkAppend32Bytes               23           14  -38.82%
BenchmarkAppendStr1Byte              14           10  -23.78%
BenchmarkAppendStr4Bytes             14           11  -21.13%
BenchmarkAppendStr8Bytes             14           10  -25.17%
BenchmarkAppendStr16Bytes            19           11  -41.45%
BenchmarkAppendStr32Bytes            18           14  -19.44%
BenchmarkAppendSpecialCase           62           63   +1.77%

R=golang-dev, khr, cshapiro, rsc, dave
CC=golang-dev
https://codereview.appspot.com/12815046

Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.

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