code review 6280049: cmd/gc: inline convT2E when T is uintptr-shaped.

cmd/gc: inline convT2E when T is uintptr-shaped.

GOARCH=amd64 benchmarks

src/pkg/runtime
benchmark                  old ns/op    new ns/op    delta
BenchmarkConvT2ESmall             10           10   +1.00%
BenchmarkConvT2EUintptr            9            0  -92.07%
BenchmarkConvT2EBig               74           74   -0.27%
BenchmarkConvT2I                  27           26   -3.62%
BenchmarkConvI2E                   4            4   -7.05%
BenchmarkConvI2I                  20           19   -2.99%

test/bench/go1
benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17    5930908000   5937260000   +0.11%
BenchmarkFannkuch11      3927057000   3933556000   +0.17%
BenchmarkGobDecode         21998090     21870620   -0.58%
BenchmarkGobEncode         12725310     12734480   +0.07%
BenchmarkGzip             567617600    567892800   +0.05%
BenchmarkGunzip           178284100    178706900   +0.24%
BenchmarkJSONEncode        87693550     86794300   -1.03%
BenchmarkJSONDecode       314212600    324115000   +3.15%
BenchmarkMandelbrot200      7016640      7073766   +0.81%
BenchmarkParse              7852100      7892085   +0.51%
BenchmarkRevcomp         1285663000   1286147000   +0.04%
BenchmarkTemplate         566823800    567606200   +0.14%

I'm not entirely sure why the JSON* numbers have changed, but
eyeballing the profile suggests that it could be spending less
and more time in runtime.{new,old}stack, so it could simply be
stack-split boundary noise.

[nigeltao, 2012-06-12 05:26:44]

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

I'd like you to review this change to
https://code.google.com/p/go/

[dave_cheney.net, 2012-06-12 07:00:40]

This is great Nigel! I'm surprised that it made so little difference in the go1
benches.

[dave_cheney.net, 2012-06-12 07:45:34]

linux/arm

benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17   36973266000  36960083000   -0.04%
BenchmarkFannkuch11     34931763000  35084259000   +0.44%
BenchmarkGobDecode        120079050    118865950   -1.01%
BenchmarkGobEncode         61419060     61716320   +0.48%
BenchmarkGzip            5549774000   5683991000   +2.42%
BenchmarkGunzip          1190766000   1193939000   +0.27%
BenchmarkJSONEncode       798712200    801367200   +0.33%
BenchmarkJSONDecode      2729737000   2808990000   +2.90%
BenchmarkMandelbrot200     45623780     45781860   +0.35%
BenchmarkParse             58031620     57316280   -1.23%
BenchmarkRevcomp          139642400    145401000   +4.12%
BenchmarkTemplate        5806061000   5811341000   +0.09%

benchmark                  old MB/s     new MB/s  speedup
BenchmarkGobDecode             6.39         6.46    1.01x
BenchmarkGobEncode            12.50        12.44    1.00x
BenchmarkGzip                  3.50         3.41    0.97x
BenchmarkGunzip               16.30        16.25    1.00x
BenchmarkJSONEncode            2.43         2.42    1.00x
BenchmarkJSONDecode            0.71         0.69    0.97x
BenchmarkParse                 1.00         1.01    1.01x
BenchmarkRevcomp              18.20        17.48    0.96x
BenchmarkTemplate              0.33         0.33    1.00x

Weird. Trips through itab genrally account for at minimum 3% of a benchmark. I
don't understand why not having make that call makes things slower ...

[nigeltao, 2012-06-12 08:13:20]

2012/6/12  <dave@cheney.net>:
> Weird. Trips through itab genrally account for at minimum 3% of a
> benchmark. I don't understand why not having make that call makes things
> slower ...

convT2E doesn't construct an itab: the empty interface has no methods,
and stores the type pointer directly in the interface value.

Optimizing convT2I might have a larger impact, especially on arm, but
it's also a larger change, and I'm still fairly new to cmd/gc.

[dave_cheney.net, 2012-06-12 11:05:56]

Here is a random data point (produced with the printf hammer)

deadwood(~/go/test/bench/go1) % sort b.txt | uniq -c | sort -nr
5553540 convT2I
5058305 assertE2T
1586114 assertI2T2
 923607 assertI2T
 217773 assertE2I2
 208061 convT2E
 153808 assertE2T2
     87 assertI2I2
     55 convI2I


On Tue, Jun 12, 2012 at 6:13 PM, Nigel Tao <nigeltao@golang.org> wrote:
> 2012/6/12  <dave@cheney.net>:
>> Weird. Trips through itab genrally account for at minimum 3% of a
>> benchmark. I don't understand why not having make that call makes things
>> slower ...
>
> convT2E doesn't construct an itab: the empty interface has no methods,
> and stores the type pointer directly in the interface value.
>
> Optimizing convT2I might have a larger impact, especially on arm, but
> it's also a larger change, and I'm still fairly new to cmd/gc.

[bsiegert, 2012-06-12 12:13:11]

On Tue, Jun 12, 2012 at 7:26 AM,  <nigeltao@golang.org> wrote:
> BenchmarkConvT2EUintptr            9            0  -92.07%

So it takes no time at all now?

--Benny.

[dsymonds, 2012-06-12 12:15:47]

On Tue, Jun 12, 2012 at 10:13 PM, Benny Siegert <bsiegert@gmail.com> wrote:

> On Tue, Jun 12, 2012 at 7:26 AM,  <nigeltao@golang.org> wrote:
>> BenchmarkConvT2EUintptr            9            0  -92.07%
>
> So it takes no time at all now?

It's inlined, and the resolution printed there is only nanoseconds. I
could see this being compiled to a single MOV instruction between two
registers, and that can believably take under a nanosecond (averaged
out). The percentage puts the mean at 0.7137 ns.


Dave.

[rsc, 2012-06-12 13:24:51]

The JSON numbers change any time you look at them funny.

[nigeltao, 2012-06-12 23:50:55]

On 12 June 2012 22:13, Benny Siegert <bsiegert@gmail.com> wrote:
> On Tue, Jun 12, 2012 at 7:26 AM,  <nigeltao@golang.org> wrote:
>> BenchmarkConvT2EUintptr            9            0  -92.07%
>
> So it takes no time at all now?

The benchcmp output cut off the fractional digits. Before/after "go
test" output:
BenchmarkConvT2EUintptr	200000000	         9.33 ns/op
BenchmarkConvT2EUintptr	2000000000	         0.74 ns/op

[nigeltao, 2012-06-12 23:53:10]

On 12 June 2012 21:05, Dave Cheney <dave@cheney.net> wrote:
> Here is a random data point (produced with the printf hammer)

Nice hammer, that one.


> deadwood(~/go/test/bench/go1) % sort b.txt | uniq -c | sort -nr
> 5553540 convT2I
> 5058305 assertE2T
> 1586114 assertI2T2
>  923607 assertI2T
>  217773 assertE2I2
>  208061 convT2E
>  153808 assertE2T2
>     87 assertI2I2
>     55 convI2I

Are these numbers from a clean tree or with this patch?

[dave_cheney.net, 2012-06-12 23:55:48]

Clean tree. I'll rebuild with your patch and post the results.

On Wed, Jun 13, 2012 at 9:53 AM, Nigel Tao <nigeltao@golang.org> wrote:
> On 12 June 2012 21:05, Dave Cheney <dave@cheney.net> wrote:
>> Here is a random data point (produced with the printf hammer)
>
> Nice hammer, that one.
>
>
>> deadwood(~/go/test/bench/go1) % sort b.txt | uniq -c | sort -nr
>> 5553540 convT2I
>> 5058305 assertE2T
>> 1586114 assertI2T2
>>  923607 assertI2T
>>  217773 assertE2I2
>>  208061 convT2E
>>  153808 assertE2T2
>>     87 assertI2I2
>>     55 convI2I
>
> Are these numbers from a clean tree or with this patch?

[nigeltao, 2012-06-13 00:01:09]

On 12 June 2012 22:15, David Symonds <dsymonds@golang.org> wrote:
> It's inlined, and the resolution printed there is only nanoseconds. I
> could see this being compiled to a single MOV instruction between two
> registers, and that can believably take under a nanosecond (averaged
> out). The percentage puts the mean at 0.7137 ns.

Yeah, it's two MOVs.

$ cat iface.go
package main

var i interface{}

func main() {
	i = uintptr(1)
}



New codegen.

$ go tool 6g -S -o /dev/null iface.go | sed -e '/init/q'

--- prog list "main" ---
0000 (iface.go:5) TEXT    main+0(SB),$0-0
0001 (iface.go:6) MOVQ    $type.uintptr+0(SB),i+0(SB)
0002 (iface.go:6) MOVQ    $1,i+8(SB)
0003 (iface.go:7) RET     ,

--- prog list "init" ---



Old codegen.

$ go tool 6g -S -o /dev/null iface.go | sed -e '/init/q'

--- prog list "main" ---
0000 (iface.go:5) TEXT    main+0(SB),$32-0
0001 (iface.go:6) MOVQ    $type.uintptr+0(SB),(SP)
0002 (iface.go:6) MOVQ    $1,8(SP)
0003 (iface.go:6) CALL    ,runtime.convT2E+0(SB)
0004 (iface.go:6) MOVQ    16(SP),BX
0005 (iface.go:6) MOVQ    BX,i+0(SB)
0006 (iface.go:6) MOVQ    24(SP),BX
0007 (iface.go:6) MOVQ    BX,i+8(SB)
0008 (iface.go:7) RET     ,

--- prog list "init" ---

[dave_cheney.net, 2012-06-13 00:05:20]

deadwood(~/go/test/bench/go1) % sort b2.txt | uniq -c | sort -nr
5506080 convT2I
4627295 assertE2T
1585844 assertI2T2
 923397 assertI2T
 217773 assertE2I2
 153808 assertE2T2
  26109 convT2E
     57 assertI2I2
     55 convI2I
      1 testing: warning: no tests to run

Here is the hammers, http://codereview.appspot.com/6303072

[rsc, 2012-06-13 15:57:42]

http://codereview.appspot.com/6280049/diff/11007/test/convT2E.go
File test/convT2E.go (right):

http://codereview.appspot.com/6280049/diff/11007/test/convT2E.go#newcode7
test/convT2E.go:7: // Test conversion from non-interface types to the empty
interface.
That sounds great, but I don't understand why there are channel ops below.
If you ignore the channel stuff, the got test is only checking a single
conversion.

I would have expected checking of all the values, as in:

if z != iz { panic("z") }
if p != ip { panic("p") }
and so on

I suspect the channel thing is mostly unrelated; add a comment there about why
it's here.

[rsc, 2012-06-13 16:00:41]

LGTM

Please update the test.

[nigeltao, 2012-06-14 00:53:37]

*** Submitted as http://code.google.com/p/go/source/detail?r=8c461c433cb5 ***

cmd/gc: inline convT2E when T is uintptr-shaped.

GOARCH=amd64 benchmarks

src/pkg/runtime
benchmark                  old ns/op    new ns/op    delta
BenchmarkConvT2ESmall             10           10   +1.00%
BenchmarkConvT2EUintptr            9            0  -92.07%
BenchmarkConvT2EBig               74           74   -0.27%
BenchmarkConvT2I                  27           26   -3.62%
BenchmarkConvI2E                   4            4   -7.05%
BenchmarkConvI2I                  20           19   -2.99%

test/bench/go1
benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17    5930908000   5937260000   +0.11%
BenchmarkFannkuch11      3927057000   3933556000   +0.17%
BenchmarkGobDecode         21998090     21870620   -0.58%
BenchmarkGobEncode         12725310     12734480   +0.07%
BenchmarkGzip             567617600    567892800   +0.05%
BenchmarkGunzip           178284100    178706900   +0.24%
BenchmarkJSONEncode        87693550     86794300   -1.03%
BenchmarkJSONDecode       314212600    324115000   +3.15%
BenchmarkMandelbrot200      7016640      7073766   +0.81%
BenchmarkParse              7852100      7892085   +0.51%
BenchmarkRevcomp         1285663000   1286147000   +0.04%
BenchmarkTemplate         566823800    567606200   +0.14%

I'm not entirely sure why the JSON* numbers have changed, but
eyeballing the profile suggests that it could be spending less
and more time in runtime.{new,old}stack, so it could simply be
stack-split boundary noise.

R=rsc, dave, bsiegert, dsymonds
CC=golang-dev
http://codereview.appspot.com/6280049