code review 12815046: cmd/gc, runtime: inline append in frontend.

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%

[remyoudompheng, 2013-08-18 16:53:50]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[remyoudompheng, 2013-08-18 16:55:10]

Note that the actual CLs touches only cmd/gc and runtime:
 src/cmd/gc/builtin.c         |   3 --
 src/cmd/gc/runtime.go        |   5 --
 src/cmd/gc/walk.c            | 107 ++++++++++++++++++++++++++++++++++++++-----
 src/pkg/runtime/arch_386.h   |   1 -
 src/pkg/runtime/arch_amd64.h |   1 -
 src/pkg/runtime/arch_arm.h   |   1 -
 src/pkg/runtime/slice.c      | 103 -----------------------------------------
 7 files changed, 95 insertions(+), 126 deletions(-)

I have included CL9101048 for convenience here.

[khr, 2013-08-27 20:33:13]

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):

https://codereview.appspot.com/12815046/diff/5001/src/cmd/gc/walk.c#newcode2574
src/cmd/gc/walk.c:2574: conv(nod(OLEN, l2, N), types[TINT64]))));
Maybe for another changelist, but why does growslice take an int64 instead of an
intptr?

[cshapiro1, 2013-08-27 20:54:22]

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 type but you are loading the pointer of a string or
slice.  Can we use a pointer type instead?

[rsc, 2013-09-06 19:37:51]

will wait for the other CL so i can see the real diffs

[rsc, 2013-09-06 20:11:54]

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:
> A uintptr is not a pointer type but you are loading the pointer of a string or
> slice.  Can we use a pointer type instead?

a->etype doesn't distinguish pointers from integers.

[remyoudompheng, 2013-09-12 05:30:02]

Hello golang-dev@googlegroups.com, khr@golang.org, cshapiro@google.com,
rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[remyoudompheng, 2013-09-12 05:50:19]

Benchmark on linux/arm:

benchmark                     old ns/op    new ns/op    delta
BenchmarkAppend                     425          425   +0.00%
BenchmarkAppend1Byte                 53           40  -23.93%
BenchmarkAppend4Bytes                63           62   -1.10%
BenchmarkAppend7Bytes                71           50  -28.95%
BenchmarkAppend8Bytes                75           49  -33.95%
BenchmarkAppend15Bytes               74           54  -26.44%
BenchmarkAppend16Bytes               78           52  -32.99%
BenchmarkAppend32Bytes               67           59  -12.52%
BenchmarkAppendStr1Byte              39           41   +4.52%
BenchmarkAppendStr4Bytes             48           63  +31.12%
BenchmarkAppendStr8Bytes             60           49  -17.64%
BenchmarkAppendStr16Bytes            65           52  -19.39%
BenchmarkAppendStr32Bytes            54           59  +10.72%
BenchmarkAppendSpecialCase          372          373   +0.27%

[khr, 2013-09-12 21:56:34]

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 a bit more space than required.  You could get away with
len(l1)+len(l2)-cap(l1).  You've almost calculated that value above.  Keep it
around, or alternately you could define an alternate growslice() call that does
the right adjustment.

[remyoudompheng, 2013-09-13 10:47:44]

Hello golang-dev@googlegroups.com, khr@golang.org, cshapiro@google.com,
rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[remyoudompheng, 2013-09-13 10:49:36]

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, khr wrote:
> This allocates a bit more space than required.  You could get away with
> len(l1)+len(l2)-cap(l1).  You've almost calculated that value above.  Keep it
> around, or alternately you could define an alternate growslice() call that
does
> the right adjustment.

Done.

Note however that append() just below is also using the number of appended
elements as growslice argument.

[dave_cheney.net, 2013-09-14 02:44:45]

Could you please remail this CL

% hg clpatch 12815046
abort: codereview issue 12815046 is out of date: patch and recent changes
conflict (0f036b4b1da5->b19d3a8c9357)

[remyoudompheng, 2013-09-15 06:47:43]

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.

[rsc, 2013-09-17 00:30:08]

LGTM

thanks for bearing with us.

[rsc, 2013-09-17 00:31:26]

*** 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>