code review 6260051: runtime: replace runtime·rnd function with ROUND macro (issue 6260051)

[r...@golang.org]

Reviewers: r,

Message:
Hello r (cc: golan...@googlegroups.com),

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


Description:
runtime: replace runtime·rnd function with ROUND macro

It's sad to introduce a new macro, but rnd shows up consistently
in profiles, and the function call overwhelms the two arithmetic
instructions it performs.

Please review this at http://codereview.appspot.com/6260051/

Affected files:
M src/cmd/gc/reflect.c
M src/pkg/runtime/chan.c
M src/pkg/runtime/hashmap.c
M src/pkg/runtime/iface.c
M src/pkg/runtime/print.c
M src/pkg/runtime/runtime.c
M src/pkg/runtime/runtime.h

[Brad Fitzpatrick]

Lot of whitespace changes in here, which you don't normally do.  Editor settings changed?

[r...@golang.org]

LGTM

http://codereview.appspot.com/6260051/

[Russ Cox]

On Tue, May 29, 2012 at 1:52 PM, Brad Fitzpatrick <brad...@golang.org> wrote:
> Lot of whitespace changes in here, which you don't normally do.  Editor
> settings changed?

no, but i noticed i had left a space on a line somewhere and so i ran my script
that cuts the trailing space from all my edited files. oh well.

russ

[Rob 'Commander' Pike]

editor settings? we don't need no stinking editor settings.

-rob

[r...@golang.org]

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

runtime: replace runtime·rnd function with ROUND macro

It's sad to introduce a new macro, but rnd shows up consistently
in profiles, and the function call overwhelms the two arithmetic
instructions it performs.

R=r
CC=golang-dev
http://codereview.appspot.com/6260051


http://codereview.appspot.com/6260051/

[da...@cheney.net]

Thank you for submitting this russ. I did a benchmark run comparing last
night to +tip including this fix, on the whole the results are very
encouraging.

There is one regression, which is BenchmarkSyscall.

pando(~/go/src/pkg/runtime) % ~/go/misc/benchcmp {old,new}.txt
benchmark old ns/op new ns/op delta
BenchmarkAppend 5927 5905 -0.37%
BenchmarkAppendSpecialCase 5634 5537 -1.72%
BenchmarkSelectUncontended 3484 3401 -2.38%
BenchmarkSelectContended 3518 3388 -3.70%
BenchmarkSelectNonblock 1398 1030 -26.32%
BenchmarkChanUncontended 610 610 +0.00%
BenchmarkChanContended 611 611 +0.00%
BenchmarkChanSync 2005 2062 +2.84%
BenchmarkChanProdCons0 1888 1927 +2.07%
BenchmarkChanProdCons10 926 928 +0.22%
BenchmarkChanProdCons100 651 651 +0.00%
BenchmarkChanProdConsWork0 14568 12589 -13.58%
BenchmarkChanProdConsWork10 12214 11535 -5.56%
BenchmarkChanProdConsWork100 11687 11251 -3.73%
BenchmarkChanCreation 10525 10303 -2.11%
BenchmarkChanSem 621 621 +0.00%
BenchmarkCallClosure 27 27 +0.00%
BenchmarkCallClosure1 32 32 +0.00%
BenchmarkCallClosure2 2024 1978 -2.27%
BenchmarkCallClosure3 2022 2020 -0.10%
BenchmarkCallClosure4 2025 1996 -1.43%
BenchmarkComplex128DivNormal 1074 1098 +2.23%
BenchmarkComplex128DivNisNaN 1029 1003 -2.53%
BenchmarkComplex128DivDisNaN 1009 1027 +1.78%
BenchmarkComplex128DivNisInf 675 659 -2.37%
BenchmarkComplex128DivDisInf 612 610 -0.33%
BenchmarkMapUint8Get1 724 522 -27.90%
BenchmarkMapUint8Get2 734 528 -28.07%
BenchmarkMapUint8Put 827 651 -21.28%
BenchmarkMapUint16Get1 730 522 -28.49%
BenchmarkMapUint16Get2 739 530 -28.28%
BenchmarkMapUint16Put 830 651 -21.57%
BenchmarkMapUint32Get1 749 541 -27.77%
BenchmarkMapUint32Get2 743 547 -26.38%
BenchmarkMapUint32Put 887 682 -23.11%
BenchmarkMapUint64Get1 792 600 -24.24%
BenchmarkMapUint64Get2 801 602 -24.84%
BenchmarkMapUint64Put 902 725 -19.62%
BenchmarkConvT2E 220 68 -68.95%
BenchmarkConvT2EBig 2148 1965 -8.52%
BenchmarkConvT2I 672 495 -26.34%
BenchmarkConvI2E 38 38 +0.00%
BenchmarkConvI2I 481 466 -3.12%
BenchmarkAssertE2T 74 74 +0.27%
BenchmarkAssertE2TBig 76 76 +0.26%
BenchmarkAssertE2I 493 490 -0.61%
BenchmarkAssertI2T 76 78 +2.08%
BenchmarkAssertI2I 491 494 +0.61%
BenchmarkAssertI2E 38 38 +0.52%
BenchmarkAssertE2E 41 41 +2.20%
BenchmarkFinalizer 3661 3121 -14.75%
BenchmarkFinalizerRun 105578 104028 -1.47%
BenchmarkStackGrowth 5937 5835 -1.72%
BenchmarkSyscall 241 241 +0.00%
BenchmarkSyscallWork 9275 12002 +29.40%
BenchmarkIfaceCmp100 2326 1971 -15.26%
BenchmarkIfaceCmpNil100 1566 1722 +9.96%
BenchmarkSoftMod 220 224 +1.82%
BenchmarkSoftDiv 225 237 +5.33%


http://codereview.appspot.com/6260051/

[Russ Cox]

Surely not my fault.

[Dave Cheney]

Sadly it appears so,

rev: 8bc69f008178243a228e7ded986730fc020bc9f3

PASS
BenchmarkSyscall 10000000 208 ns/op
BenchmarkSyscallWork 200000 9243 ns/op

rev 722bb90ae3eed04664e9b4cb06e460ce20d993e1

PASS
BenchmarkSyscall 10000000 241 ns/op
BenchmarkSyscallWork 200000 11964 ns/op

On Wed, May 30, 2012 at 8:28 AM, Russ Cox <r...@golang.org> wrote:
> Surely not my fault.

[Russ Cox]

On Tue, May 29, 2012 at 6:38 PM, Dave Cheney <da...@cheney.net> wrote:
> Sadly it appears so,
>
> rev: 8bc69f008178243a228e7ded986730fc020bc9f3
>
> PASS
> BenchmarkSyscall        10000000               208 ns/op
> BenchmarkSyscallWork      200000              9243 ns/op
>
> rev 722bb90ae3eed04664e9b4cb06e460ce20d993e1
>
> PASS
> BenchmarkSyscall        10000000               241 ns/op
> BenchmarkSyscallWork      200000             11964 ns/op

I'm still very skeptical. What architecture?

Russ

[Dave Cheney]

linux/arm, I can't detect a regression on recent linux/amd64

[Dave Cheney]

This may not be a real regression, the test is spending a lot of it's
time in _div. See attached pprof.

[Russ Cox]

BenchmarkSyscall is timing a tight loop that does Entersyscall/Exitsyscall.
BenchmarkSyscallWork is timing a loop that does those but does a lot
of * and / between them.

If they are affected by the introduction of the ROUND macro, it can
only be because the functions moved. If you want to dig, I would try
to get a 5l -a dump (go test -c -ldflags -a I think) of the two and
compare the loops. If you insert extra math between foo := 42 and the
inner loop (say, foo++, foo--, etc) you can probably move the loop a
bit and see if you observe differences. Or maybe it's Entersyscall and
Exitsyscall moving that matters.

This seems to be the way of modern hardware: performance optimization
is like playing whac-a-mole. On balance, I think ROUND is a win
compared to not having it.

You also mentioned convT2I. We have a different plan for that.

Russ

[Dave Cheney]

> This seems to be the way of modern hardware: performance optimization
> is like playing whac-a-mole. On balance, I think ROUND is a win
> compared to not having it.

I agree, it has improved benchmarks across the board for arm (and I'm
sure for 8/6 as well, but too a lesser degree), fmt for example shows
an improvement (and this is before r's big commit)

benchmark old ns/op new ns/op delta

BenchmarkSprintfEmpty 1857 1698 -8.56%
BenchmarkSprintfString 10336 10390 +0.52%
BenchmarkSprintfInt 7484 7328 -2.08%
BenchmarkSprintfIntInt 10073 9819 -2.52%
BenchmarkSprintfPrefixedInt 19262 18993 -1.40%
BenchmarkSprintfFloat 21008 20586 -2.01%
BenchmarkScanInts 22697440 22241480 -2.01%
BenchmarkScanRecursiveInt 31775520 30975880 -2.52%

> You also mentioned convT2I. We have a different plan for that.

Excellent.