code review 144530045: cmd/gc: fix call order in array literal of slice litera...

cmd/gc: fix call order in array literal of slice literal of make chan

Fixes issue 8761.

[rsc, 2014-09-24 19:45:08]

Hello golang-codereviews@googlegroups.com (cc: iant, r),

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

[iant, 2014-09-24 20:20:39]

LGTM

https://codereview.appspot.com/144530045/diff/40001/src/cmd/gc/order.c
File src/cmd/gc/order.c (right):

https://codereview.appspot.com/144530045/diff/40001/src/cmd/gc/order.c#newcod...
src/cmd/gc/order.c:1039: case OMAKECHAN:
Why not OLEN, OCAP, OCOPY, ODELETE, OREAL, OIMAG, OCLOSE, etc.?

[rsc, 2014-09-30 16:27:20]

https://codereview.appspot.com/144530045/diff/40001/src/cmd/gc/order.c
File src/cmd/gc/order.c (right):

https://codereview.appspot.com/144530045/diff/40001/src/cmd/gc/order.c#newcod...
src/cmd/gc/order.c:1039: case OMAKECHAN:
On 2014/09/24 20:20:39, iant wrote:
> Why not OLEN, OCAP, OCOPY, ODELETE, OREAL, OIMAG, OCLOSE, etc.?

You are right, of course. Added a test demonstrating problems with most of
those. delete and close are okay because they cannot appear inside other
expressions.

[rsc, 2014-09-30 16:48:51]

*** Submitted as https://code.google.com/p/go/source/detail?r=2e467bc60e64 ***

cmd/gc: fix call order in array literal of slice literal of make chan

Fixes issue 8761.

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews, r
https://codereview.appspot.com/144530045

[gobot, 2014-09-30 16:53:51]

This CL appears to have broken the linux-amd64-noopt builder.
See http://build.golang.org/log/5f68b8a757093b685a0d964a71e5153789125cc2

[bradfitz, 2014-09-30 17:00:30]

I see a number of 100 * time.Milliseconds in that test. Maybe it's too low.

ok   net/rpc/jsonrpc 0.067s
ok   net/smtp 0.165s
ok   net/textproto 0.059s
ok   net/url 0.029s
ok   os 2.316s
ok   os/exec 1.232s
--- FAIL: TestNohup (0.74s)
signal_test.go:186: ran test with -send_uncaught_sighup=1 and it succeeded:
expected failure.
Output:
PASS
FAIL
FAIL os/signal 1.522s
ok   os/user 0.191s
ok   path 0.031s
ok   path/filepath 0.198s
ok   reflect 0.288s
ok   regexp 0.999s
ok   regexp/syntax 2.663s
ok   runtime 14.503s
?   runtime/cgo [no test files]



On Tue, Sep 30, 2014 at 9:53 AM, <gobot@golang.org> wrote:

> This CL appears to have broken the linux-amd64-noopt builder.
> See http://build.golang.org/log/5f68b8a757093b685a0d964a71e5153789125cc2
>
>
> https://codereview.appspot.com/144530045/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[gobot, 2014-09-30 17:21:58]

This changed caused perf changes on linux-amd64-perf:


build-1                   old          new      delta
build-cputime     10068629000  10132633000      +0.64
build-rss           171184128    172724224      +0.90
build-time        10389772131  10449524303      +0.58

build-2                   old          new      delta
build-cputime     10096630000  10208637000      +1.11
build-rss           171184128    172728320      +0.90
build-time         7410118971   7462781656      +0.71

build-4                   old          new      delta
build-rss           171188224    172724224      +0.90

build-8                   old          new      delta
build-cputime     10260640000  10356646000      +0.94
build-rss           171184128    172724224      +0.90

build-16                  old          new      delta
build-cputime     10540658000  10684667000      +1.37
build-rss           171184128    172724224      +0.90
build-time         6109507071   6164855399      +0.91

http://build.golang.org/perfdetail?commit=2e467bc60e64def06419194f48fe0d6c8b5...



—gobot

[gobot, 2014-10-01 05:53:52]

This changed caused perf changes on windows-amd64-perf:


build-1                   old          new      delta
build-rss          2589450240   2606473216      +0.66
build-time        11429306400  11534401100      +0.92

build-2                   old          new      delta
build-rss          2588176384   2605088768      +0.65

build-4                   old          new      delta
build-rss          2584358912   2606006272      +0.84
build-time         6153549200   6196587600      +0.70

build-8                   old          new      delta
build-rss          2589155328   2607415296      +0.71
build-time         5745180500   5828255000      +1.45

build-16                  old          new      delta
build-rss          2590490624   2608865280      +0.71

http://build.golang.org/perfdetail?commit=2e467bc60e64def06419194f48fe0d6c8b5...



—gobot

[dave_cheney.net, 2014-10-01 05:55:14]

I'm impressed the benchmark harness can detect such small variations.

On Wed, Oct 1, 2014 at 3:53 PM,  <gobot@golang.org> wrote:
> This changed caused perf changes on windows-amd64-perf:
>
>
> build-1                   old          new      delta
> build-rss          2589450240   2606473216      +0.66
> build-time        11429306400  11534401100      +0.92
>
> build-2                   old          new      delta
> build-rss          2588176384   2605088768      +0.65
>
> build-4                   old          new      delta
> build-rss          2584358912   2606006272      +0.84
> build-time         6153549200   6196587600      +0.70
>
> build-8                   old          new      delta
> build-rss          2589155328   2607415296      +0.71
> build-time         5745180500   5828255000      +1.45
>
> build-16                  old          new      delta
> build-rss          2590490624   2608865280      +0.71
>
>
http://build.golang.org/perfdetail?commit=2e467bc60e64def06419194f48fe0d6c8b5...
>
>
>
>
> --gobot
>
>
> https://codereview.appspot.com/144530045/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.