Issue 78870047: code review 78870047: unicode: rearrange static data to decrease binary size (Closed)
Descriptionunicode: rearrange static data to decrease binary size
Using a single, shared backing array across RangeTables
reduces binary size by ~33k. No API, godoc, or functional
changes.
While we're in there, combine repeated var declarations into
a var block for prettier gofmt output, and tweak the
generated code to be closer to its final, gofmt'd form.
I tested that there are no functional changes by confirming
that the output of `go run dumptables.go | sort` is unchanged.
dumptables.go: http://play.golang.org/p/wqHVlEJYli
Fixes issue 7600.
Update issue 6853
Rearranging static unicode data cut 33k and made further
savings available via issue 7599.
Patch Set 1 #Patch Set 2 : diff -r e8ca57933a8f https://code.google.com/p/go #Patch Set 3 : diff -r e8ca57933a8f https://code.google.com/p/go #
Total comments: 12
Patch Set 4 : diff -r e8ca57933a8f https://code.google.com/p/go #Patch Set 5 : diff -r e8ca57933a8f https://code.google.com/p/go #
Total comments: 1
MessagesTotal messages: 13
Hello 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.
point of order, place Update issue 6853 at the top of the description so the rest of the description is copied into issue 6853, may as well move Fixes issue 7600. there as well On Sat, Mar 22, 2014 at 10:53 AM, <josharian@gmail.com> wrote: > Reviewers: golang-codereviews, > > Message: > Hello golang-codereviews@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > unicode: rearrange static data to decrease binary size > > Using a single, shared backing array across RangeTables > reduces binary size by ~33k. No API, godoc, or functional > changes. > > While we're in there, combine repeated var declarations into > a var block for prettier gofmt output, and tweak the > generated code to be closer to its final, gofmt'd form. > > I tested that there are no functional changes by confirming > that the output of `go run dumptables.go | sort` is unchanged. > dumptables.go: http://play.golang.org/p/wqHVlEJYli > > Fixes issue 7600. > > Update issue 6853 > > Rearranging static unicode data cut 33k and made further > savings available via issue 7599. > > Please review this at https://codereview.appspot.com/78870047/ > > Affected files (+4619, -5442 lines): > M src/pkg/unicode/maketables.go > M src/pkg/unicode/tables.go > > > -- > 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.
I intentionally put that shorter summary at the bottom for issue 6853, figuring that the other details weren't of interest there. If you think it'd be better, though, I'd be happy to move the Update line up and cut the mini-summary. LMK. On Fri, Mar 21, 2014 at 5:30 PM, Dave Cheney <dave@cheney.net> wrote: > point of order, place Update issue 6853 at the top of the description > so the rest of the description is copied into issue 6853, may as well > move Fixes issue 7600. there as well > > On Sat, Mar 22, 2014 at 10:53 AM, <josharian@gmail.com> wrote: >> Reviewers: golang-codereviews, >> >> Message: >> Hello golang-codereviews@googlegroups.com, >> >> I'd like you to review this change to >> https://code.google.com/p/go >> >> >> Description: >> unicode: rearrange static data to decrease binary size >> >> Using a single, shared backing array across RangeTables >> reduces binary size by ~33k. No API, godoc, or functional >> changes. >> >> While we're in there, combine repeated var declarations into >> a var block for prettier gofmt output, and tweak the >> generated code to be closer to its final, gofmt'd form. >> >> I tested that there are no functional changes by confirming >> that the output of `go run dumptables.go | sort` is unchanged. >> dumptables.go: http://play.golang.org/p/wqHVlEJYli >> >> Fixes issue 7600. >> >> Update issue 6853 >> >> Rearranging static unicode data cut 33k and made further >> savings available via issue 7599. >> >> Please review this at https://codereview.appspot.com/78870047/ >> >> Affected files (+4619, -5442 lines): >> M src/pkg/unicode/maketables.go >> M src/pkg/unicode/tables.go >> >> >> -- >> 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.
I'm a big fan of including as many details as possible. Let's see what others think On Sat, Mar 22, 2014 at 11:34 AM, Josh Bleecher Snyder <josharian@gmail.com>wrote: > I intentionally put that shorter summary at the bottom for issue 6853, > figuring that the other details weren't of interest there. If you > think it'd be better, though, I'd be happy to move the Update line up > and cut the mini-summary. LMK. > > On Fri, Mar 21, 2014 at 5:30 PM, Dave Cheney <dave@cheney.net> wrote: > > point of order, place Update issue 6853 at the top of the description > > so the rest of the description is copied into issue 6853, may as well > > move Fixes issue 7600. there as well > > > > On Sat, Mar 22, 2014 at 10:53 AM, <josharian@gmail.com> wrote: > >> Reviewers: golang-codereviews, > >> > >> Message: > >> Hello golang-codereviews@googlegroups.com, > >> > >> I'd like you to review this change to > >> https://code.google.com/p/go > >> > >> > >> Description: > >> unicode: rearrange static data to decrease binary size > >> > >> Using a single, shared backing array across RangeTables > >> reduces binary size by ~33k. No API, godoc, or functional > >> changes. > >> > >> While we're in there, combine repeated var declarations into > >> a var block for prettier gofmt output, and tweak the > >> generated code to be closer to its final, gofmt'd form. > >> > >> I tested that there are no functional changes by confirming > >> that the output of `go run dumptables.go | sort` is unchanged. > >> dumptables.go: http://play.golang.org/p/wqHVlEJYli > >> > >> Fixes issue 7600. > >> > >> Update issue 6853 > >> > >> Rearranging static unicode data cut 33k and made further > >> savings available via issue 7599. > >> > >> Please review this at https://codereview.appspot.com/78870047/ > >> > >> Affected files (+4619, -5442 lines): > >> M src/pkg/unicode/maketables.go > >> M src/pkg/unicode/tables.go > >> > >> > >> -- > >> 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. > > -- > 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.
> I'm a big fan of including as many details as possible. Let's see what others think Ok. Replying so that this shows up on the dashboard as needing review. (Want to suggest/assign a reviewer?)
Sign in to reply to this message.
R=r Rob likes Unicode and large binaries. On Mar 24, 2014 10:50 AM, <josharian@gmail.com> wrote: > I'm a big fan of including as many details as possible. Let's see what >> > others think > > Ok. Replying so that this shows up on the dashboard as needing review. > (Want to suggest/assign a reviewer?) > > https://codereview.appspot.com/78870047/ > > -- > 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.
The change looks pretty good. I need to think about whether this should wait for 1.4 https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables.go File src/pkg/unicode/maketables.go (right): https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:556: // allRange[16|32] accumulate all ranges that will be used if you're going to use a regexp, use a regexp. allRange(16|32) https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:557: // into shared backing arrays. This makes binaries smaller. what does "This" refer to? i would prefer a more informative discussion of the strategy as a preamble to this section of the code https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:564: // Additions to a rangeTable modify allRange[16|32]. modify the allRange tables. https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:566: // used until previous rangeTables are done being used. s/done being used/complete/ https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:582: // No range contains U+FFFF as an instance, so split you need to verify this condition because it will cause trouble if it becomes violated. https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:584: // the invariant that R32 contains only >= 1<<16. s/R32/allRange32/
Sign in to reply to this message.
As always, thanks for the thoughtful review. PTAL. I don't feel strongly about 1.3 vs 1.4, although I do believe this change to be very safe. The only negative side-effect I'm aware of is a 0.2 millisecond increase in binary start time on the slower'n'molasses Raspberry Pi. Fixing issue 7599 (1.4) will fix that. https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables.go File src/pkg/unicode/maketables.go (right): https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:556: // allRange[16|32] accumulate all ranges that will be used On 2014/03/25 02:14:26, r wrote: > if you're going to use a regexp, use a regexp. > > allRange(16|32) Ack (in both senses of the word). Fixed. https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:557: // into shared backing arrays. This makes binaries smaller. On 2014/03/25 02:14:26, r wrote: > what does "This" refer to? > > i would prefer a more informative discussion of the strategy as a preamble to > this section of the code Done. https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:564: // Additions to a rangeTable modify allRange[16|32]. On 2014/03/25 02:14:26, r wrote: > modify the allRange tables. Done. https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:566: // used until previous rangeTables are done being used. On 2014/03/25 02:14:26, r wrote: > s/done being used/complete/ Done. https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:582: // No range contains U+FFFF as an instance, so split On 2014/03/25 02:14:26, r wrote: > you need to verify this condition because it will cause trouble if it becomes > violated. Done. https://codereview.appspot.com/78870047/diff/40001/src/pkg/unicode/maketables... src/pkg/unicode/maketables.go:584: // the invariant that R32 contains only >= 1<<16. On 2014/03/25 02:14:26, r wrote: > s/R32/allRange32/ Done.
Sign in to reply to this message.
https://codereview.appspot.com/78870047/diff/80001/src/pkg/unicode/tables.go File src/pkg/unicode/tables.go (right): https://codereview.appspot.com/78870047/diff/80001/src/pkg/unicode/tables.go#... src/pkg/unicode/tables.go:1209: var () could be removed.
Sign in to reply to this message.
On 2014/03/25 18:02:19, bradfitz wrote: > https://codereview.appspot.com/78870047/diff/80001/src/pkg/unicode/tables.go > File src/pkg/unicode/tables.go (right): > > https://codereview.appspot.com/78870047/diff/80001/src/pkg/unicode/tables.go#... > src/pkg/unicode/tables.go:1209: var () > could be removed. To not generate it, we'd need to buffer the intermediate output. I think that that'd be uglier than just leaving it there, but happy to make the fix if you'd prefer. I'd rather gofmt remove it for us: https://code.google.com/p/go/issues/detail?id=7631
Sign in to reply to this message.
NOT LGTM After thinking this over and talking to rsc and iant, this is probably a bad idea. If a program only needs one of these tables, it will now be required to load the entire array just to access a slice. If the real problem is just bad packing/breakage at link time, that's where we should fix this.
Sign in to reply to this message.
> NOT LGTM > > After thinking this over and talking to rsc and iant, this is probably a bad > idea. If a program only needs one of these tables, it will now be required to > load the entire array just to access a slice. Ahhhhh. I should have guessed that the OS loads symbols lazily. (So much to learn!) > If the real problem is just bad packing/breakage at link time, that's where we should fix this. Yes, I think that we can get most of the benefits here through better layout of static data. See: https://code.google.com/p/go/issues/detail?id=7599 https://code.google.com/p/go/issues/detail?id=7637 https://code.google.com/p/go/issues/detail?id=7384 It appears to me now that there are significant savings available, both from slices and from strings, but that it is going to require some careful planning and design. I'd love to work on this for Go 1.4 -- seems like a fun, substantive project -- but I would definitely need some mentorship. Perhaps we (for some value of "we") can discuss live at Gophercon, and I can follow up by writing a design doc. Back-burnered for now.
Sign in to reply to this message.
|
