Issue 152700045: code review 152700045: cmd/gc: don't use static init to initialize small struc... (Closed)
Descriptioncmd/gc: don't use static init to initialize small structs, fields
Better to avoid the memory loads and just use immediate constants.
This especially applies to zeroing, which was being done by
copying zeros from elsewhere in the binary, even if the value
was going to be completely initialized with non-zero values.
The zero writes were optimized away but the zero loads from
the data segment were not.
Patch Set 1 #Patch Set 2 : diff -r 7a5bb1f85e8dc95ee455bde7f9aee10b6b22cd53 https://code.google.com/p/go/ #Patch Set 3 : diff -r 0a372305e9a16f42004be1a2476c694b9f481b46 https://code.google.com/p/go/ #Patch Set 4 : diff -r 74954520448d828bedfc86a510154919ffe8a390 https://code.google.com/p/go/ #Patch Set 5 : diff -r 90a7c3c86944759bd88007c09f7a311c42f4bef8 https://code.google.com/p/go/ #MessagesTotal messages: 10
Hello r (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
Any numbers for this one too? On Fri, Oct 17, 2014 at 5:17 PM, <rsc@golang.org> wrote: > Reviewers: r, > > Message: > Hello r (cc: golang-codereviews@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go/ > > > Description: > cmd/gc: don't use static init to initialize small structs, fields > > Better to avoid the memory loads and just use immediate constants. > This especially applies to zeroing, which was being done by > copying zeros from elsewhere in the binary, even if the value > was going to be completely initialized with non-zero values. > The zero writes were optimized away but the zero loads from > the data segment were not. > > Please review this at https://codereview.appspot.com/152700045/ > > Affected files (+4, -4 lines): > M src/cmd/gc/sinit.c > > > Index: src/cmd/gc/sinit.c > =================================================================== > --- a/src/cmd/gc/sinit.c > +++ b/src/cmd/gc/sinit.c > @@ -1067,7 +1067,7 @@ > if(t->etype != TSTRUCT) > fatal("anylit: not struct"); > > - if(simplename(var)) { > + if(simplename(var) && count(n->list) > 4) { > > if(ctxt == 0) { > // lay out static data > @@ -1090,7 +1090,7 @@ > } > > // initialize of not completely specified > - if(count(n->list) < structcount(t)) { > + if(simplename(var) || count(n->list) < structcount(t)) { > a = nod(OAS, var, N); > typecheck(&a, Etop); > walkexpr(&a, init); > @@ -1107,7 +1107,7 @@ > break; > } > > - if(simplename(var)) { > + if(simplename(var) && count(n->list) > 4) { > > if(ctxt == 0) { > // lay out static data > @@ -1130,7 +1130,7 @@ > } > > // initialize of not completely specified > - if(count(n->list) < t->bound) { > + if(simplename(var) || count(n->list) < t->bound) { > a = nod(OAS, var, N); > typecheck(&a, Etop); > walkexpr(&a, init); > > > -- > 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. >
Sign in to reply to this message.
We need to ask +Chris to implement try functionality for perf dashboard On Fri, Oct 17, 2014 at 7:38 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Any numbers for this one too? > > > On Fri, Oct 17, 2014 at 5:17 PM, <rsc@golang.org> wrote: >> >> Reviewers: r, >> >> Message: >> Hello r (cc: golang-codereviews@googlegroups.com), >> >> I'd like you to review this change to >> https://code.google.com/p/go/ >> >> >> Description: >> cmd/gc: don't use static init to initialize small structs, fields >> >> Better to avoid the memory loads and just use immediate constants. >> This especially applies to zeroing, which was being done by >> copying zeros from elsewhere in the binary, even if the value >> was going to be completely initialized with non-zero values. >> The zero writes were optimized away but the zero loads from >> the data segment were not. >> >> Please review this at https://codereview.appspot.com/152700045/ >> >> Affected files (+4, -4 lines): >> M src/cmd/gc/sinit.c >> >> >> Index: src/cmd/gc/sinit.c >> =================================================================== >> --- a/src/cmd/gc/sinit.c >> +++ b/src/cmd/gc/sinit.c >> @@ -1067,7 +1067,7 @@ >> if(t->etype != TSTRUCT) >> fatal("anylit: not struct"); >> >> - if(simplename(var)) { >> + if(simplename(var) && count(n->list) > 4) { >> >> if(ctxt == 0) { >> // lay out static data >> @@ -1090,7 +1090,7 @@ >> } >> >> // initialize of not completely specified >> - if(count(n->list) < structcount(t)) { >> + if(simplename(var) || count(n->list) < structcount(t)) { >> a = nod(OAS, var, N); >> typecheck(&a, Etop); >> walkexpr(&a, init); >> @@ -1107,7 +1107,7 @@ >> break; >> } >> >> - if(simplename(var)) { >> + if(simplename(var) && count(n->list) > 4) { >> >> if(ctxt == 0) { >> // lay out static data >> @@ -1130,7 +1130,7 @@ >> } >> >> // initialize of not completely specified >> - if(count(n->list) < t->bound) { >> + if(simplename(var) || count(n->list) < t->bound) { >> a = nod(OAS, var, N); >> typecheck(&a, Etop); >> walkexpr(&a, init); >> >> >> -- >> 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. > > > -- > 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.
Sign in to reply to this message.
bismarck=% benchcmp tip.txt after152700045.txt benchmark old ns/op new ns/op delta BenchmarkBinaryTree17 4956004599 4973587120 +0.35% BenchmarkFannkuch11 3901187400 3939750470 +0.99% BenchmarkFmtFprintfEmpty 103 101 -1.94% BenchmarkFmtFprintfString 282 265 -6.03% BenchmarkFmtFprintfInt 269 265 -1.49% BenchmarkFmtFprintfIntInt 481 478 -0.62% BenchmarkFmtFprintfPrefixedInt 405 396 -2.22% BenchmarkFmtFprintfFloat 544 553 +1.65% BenchmarkFmtManyArgs 1939 1963 +1.24% BenchmarkGobDecode 16457839 15813414 -3.92% BenchmarkGobEncode 13583461 13452395 -0.96% BenchmarkGzip 577405464 581972321 +0.79% BenchmarkGunzip 124151648 123367325 -0.63% BenchmarkHTTPClientServer 118234 112211 -5.09% BenchmarkJSONEncode 31880596 31018395 -2.70% BenchmarkJSONDecode 101792127 102358661 +0.56% BenchmarkMandelbrot200 6213101 6207183 -0.10% BenchmarkGoParse 6257653 6195410 -0.99% BenchmarkRegexpMatchEasy0_32 209 208 -0.48% BenchmarkRegexpMatchEasy0_1K 498 493 -1.00% BenchmarkRegexpMatchEasy1_32 186 183 -1.61% BenchmarkRegexpMatchEasy1_1K 1417 1431 +0.99% BenchmarkRegexpMatchMedium_32 336 336 +0.00% BenchmarkRegexpMatchMedium_1K 120187 123011 +2.35% BenchmarkRegexpMatchHard_32 6048 5905 -2.36% BenchmarkRegexpMatchHard_1K 192145 182526 -5.01% BenchmarkRevcomp 1034990374 1047349786 +1.19% BenchmarkTemplate 151005395 147793211 -2.13% BenchmarkTimeParse 572 576 +0.70% BenchmarkTimeFormat 608 601 -1.15% benchmark old MB/s new MB/s speedup BenchmarkGobDecode 46.64 48.54 1.04x BenchmarkGobEncode 56.51 57.06 1.01x BenchmarkGzip 33.61 33.34 0.99x BenchmarkGunzip 156.30 157.29 1.01x BenchmarkJSONEncode 60.87 62.56 1.03x BenchmarkJSONDecode 19.06 18.96 0.99x BenchmarkGoParse 9.26 9.35 1.01x BenchmarkRegexpMatchEasy0_32 152.54 153.61 1.01x BenchmarkRegexpMatchEasy0_1K 2053.63 2075.37 1.01x BenchmarkRegexpMatchEasy1_32 172.04 174.42 1.01x BenchmarkRegexpMatchEasy1_1K 722.63 715.21 0.99x BenchmarkRegexpMatchMedium_32 2.97 2.97 1.00x BenchmarkRegexpMatchMedium_1K 8.52 8.32 0.98x BenchmarkRegexpMatchHard_32 5.29 5.42 1.02x BenchmarkRegexpMatchHard_1K 5.33 5.61 1.05x BenchmarkRevcomp 245.57 242.68 0.99x BenchmarkTemplate 12.85 13.13 1.02x bismarck=%
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=28de6f41b1c7 *** cmd/gc: don't use static init to initialize small structs, fields Better to avoid the memory loads and just use immediate constants. This especially applies to zeroing, which was being done by copying zeros from elsewhere in the binary, even if the value was going to be completely initialized with non-zero values. The zero writes were optimized away but the zero loads from the data segment were not. LGTM=r R=r, bradfitz, dvyukov CC=golang-codereviews https://codereview.appspot.com/152700045
Sign in to reply to this message.
Message was sent while issue was closed.
This changed caused perf changes on linux-amd64-perf: build-1 old new delta binary-size 9601696 9543608 -0.60 build-2 old new delta binary-size 9601696 9543608 -0.60 build-4 old new delta binary-size 9601696 9543608 -0.60 build-8 old new delta binary-size 9601696 9543608 -0.60 build-16 old new delta binary-size 9601696 9543608 -0.60 http-1 old new delta gc-pause-total 1782 1863 +4.55 http-2 old new delta gc-pause-total 956 997 +4.29 http-8 old new delta gc-pause-total 304 439 +44.41 http://build.golang.org/perfdetail?commit=28de6f41b1c77c5cb9046056fb29b96b958... —gobot
Sign in to reply to this message.
Message was sent while issue was closed.
This changed caused perf changes on windows-amd64-perf: build-1 old new delta binary-size 9655808 9592320 -0.66 build-2 old new delta binary-size 9655808 9592320 -0.66 build-4 old new delta binary-size 9655808 9592320 -0.66 build-8 old new delta binary-size 9655808 9592320 -0.66 build-16 old new delta binary-size 9655808 9592320 -0.66 http://build.golang.org/perfdetail?commit=28de6f41b1c77c5cb9046056fb29b96b958... —gobot
Sign in to reply to this message.
Message was sent while issue was closed.
This CL broke Camlistore. I've filed https://code.google.com/p/go/issues/detail?id=8961 mac:camlistore.org bradfitz$ go version go version devel +28de6f41b1c7 Fri Oct 17 13:10:42 2014 -0400 darwin/amd64 mac:camlistore.org bradfitz$ go test camlistore.org/cmd/camput panic: runtime error: invalid memory address or nil pointer dereference [signal 0xb code=0x1 addr=0x0 pc=0x4065a1c] goroutine 39 [running]: camlistore.org/cmd/camput.(*blobCmd).RunCommand(0x496bb20, 0xc208031730, 0x1, 0x1, 0x0, 0x0) /Users/bradfitz/src/camlistore.org/cmd/camput/blobs.go:76 +0x25c camlistore.org/pkg/cmdmain.Main() /Users/bradfitz/src/camlistore.org/pkg/cmdmain/cmdmain.go:273 +0xb27 camlistore.org/cmd/camput.func·029() /Users/bradfitz/src/camlistore.org/cmd/camput/camput_test.go:67 +0x1f created by camlistore.org/cmd/camput.(*env).Run /Users/bradfitz/src/camlistore.org/cmd/camput/camput_test.go:69 +0x450 goroutine 1 [chan receive]: testing.RunTests(0x4763548, 0x4968300, 0x5, 0x5, 0xc208030b01) /Users/bradfitz/go/src/testing/testing.go:556 +0xad6 testing.(*M).Run(0xc20802e5a0, 0x496a980) /Users/bradfitz/go/src/testing/testing.go:485 +0x6c main.main() camlistore.org/cmd/camput/_test/_testmain.go:60 +0x1d5 goroutine 38 [select]: camlistore.org/cmd/camput.(*env).Run(0xc2080e81a0, 0xc208157d98, 0x5, 0x5, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...) /Users/bradfitz/src/camlistore.org/cmd/camput/camput_test.go:70 +0x63d camlistore.org/cmd/camput.func·033(0xc208162480, 0x3a) /Users/bradfitz/src/camlistore.org/cmd/camput/camput_test.go:224 +0x2e3 camlistore.org/cmd/camput.testWithTempDir(0xc2081053b0, 0xc208157f28) /Users/bradfitz/src/camlistore.org/cmd/camput/camput_test.go:140 +0x534 camlistore.org/cmd/camput.TestCamputBlob(0xc2081053b0) /Users/bradfitz/src/camlistore.org/cmd/camput/camput_test.go:231 +0x136 testing.tRunner(0xc2081053b0, 0x4968360) /Users/bradfitz/go/src/testing/testing.go:447 +0xbf created by testing.RunTests /Users/bradfitz/go/src/testing/testing.go:555 +0xa8b goroutine 33 [syscall, locked to thread]: runtime.goexit() /Users/bradfitz/go/src/runtime/proc.c:1651 FAIL camlistore.org/cmd/camput 0.166s
Sign in to reply to this message.
fixed by daniel morsing in https://codereview.appspot.com/154540044
Sign in to reply to this message.
|
