Issue 7106050: code review 7106050: cmd/5l: reduce the size of Prog and Sym (Closed)
|
Created:
12 years, 6 months ago by dave Modified:
12 years, 5 months ago Reviewers:
CC:
minux1, remyoudompheng, rsc, golang-dev Visibility:
Public. |
Descriptioncmd/5l: reduce the size of Prog and Sym
Prog
* Remove the unused Prog* dlink
* note that align is also unused, but removing it does not help due to alignment issues.
Saves 4 bytes, sizeof(Prog): 84 => 80.
Sym
* Align {u,}char fields on word boundaries
Saves 4 bytes, sizeof(Sym): 136 => 132.
Tested on linux/arm and freebsd/arm.
Patch Set 1 #Patch Set 2 : diff -r fce9621c9927 https://code.google.com/p/go #Patch Set 3 : diff -r fce9621c9927 https://code.google.com/p/go #Patch Set 4 : diff -r fce9621c9927 https://code.google.com/p/go #Patch Set 5 : diff -r a5410435a57d https://go.googlecode.com/hg/ #Patch Set 6 : diff -r a5410435a57d https://go.googlecode.com/hg/ #MessagesTotal messages: 17
Hello minux.ma@gmail.com, remyoudompheng@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Please hold off, I think I've found an additional 8 unused bytes in Prog. On Tue, Jan 15, 2013 at 8:58 PM, <dave@cheney.net> wrote: > Reviewers: minux, remyoudompheng, rsc, > > Message: > Hello minux.ma@gmail.com, remyoudompheng@gmail.com, rsc@golang.org (cc: > golang-dev@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > cmd/5l: reduce the size of Prog and Sym > > Prog > * Remove the unused Prog* dlink > * note that align is also unused, but removing it does not help due to > alignment issues. > > Saves 4 bytes, sizeof(Prog): 84 => 80 > > Sym > * Align {u,}char fields on word boundaries > > Saves 4 bytes, under gcc sizeof(Sym): 136 => 132. Under clang 3.2 on > freebsd the size of Sym *increased* from 128 => 132, which I *think* is > due to freebsd using the armv6 model, and gcc probably using armv4t, so > will not allow unaligned shorts and words. I think on balance this is > probably a fair tradeoff as unaligned access on arm is not free. > > Tested on linux/arm and freebsd/arm. > > Please review this at https://codereview.appspot.com/7106050/ > > Affected files: > M src/cmd/5l/l.h > > > Index: src/cmd/5l/l.h > =================================================================== > --- a/src/cmd/5l/l.h > +++ b/src/cmd/5l/l.h > @@ -108,7 +108,6 @@ > } u0; > Prog* cond; > Prog* link; > - Prog* dlink; > int32 pc; > int32 line; > int32 spadj; > @@ -117,7 +116,7 @@ > uchar as; > uchar scond; > uchar reg; > - uchar align; > + uchar align; // unused > }; > > #define regused u0.u0regused > @@ -136,8 +135,6 @@ > uchar reachable; > uchar dynexport; > uchar leaf; > - uchar stkcheck; > - uchar hide; > int32 dynid; > int32 plt; > int32 got; > @@ -148,6 +145,8 @@ > int32 elfsym; > uchar special; > uchar fnptr; // used as fn ptr > + uchar stkcheck; > + uchar hide; > Sym* hash; // in hash table > Sym* allsym; // in all symbol list > Sym* next; // in text or data list > >
Sign in to reply to this message.
On Tue, Jan 15, 2013 at 5:58 PM, <dave@cheney.net> wrote: > Saves 4 bytes, under gcc sizeof(Sym): 136 => 132. Under clang 3.2 on > freebsd the size of Sym *increased* from 128 => 132, which I *think* is > I've tested latest version of clang on Linux/ARM, and sizeof(Sym) correctly shrink from 136 bytes to 132 bytes. I don't think FreeBSD/ARM should have any deviation from this. > due to freebsd using the armv6 model, and gcc probably using armv4t, so > will not allow unaligned shorts and words. by default, clang shouldn't use unaligned load/stores, and should align all fields to its natural alignment, so I don't understand why clang is able to pack the old Sym structure into 128 bytes while gcc can't.
Sign in to reply to this message.
I've found another 8 bytes that can be saved. Here is a before / after
comparison of 5l linking godoc.
Before:
Command being timed: "/home/dfc/go/pkg/tool/linux_arm/5l -o
/tmp/go-build464152087/cmd/godoc/_obj/a.out -L /tmp/go-build464152087
/tmp/go-build464152087/cmd/godoc.a"
User time (seconds): 4.11
System time (seconds): 0.33
Percent of CPU this job got: 99%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:04.46
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 113964
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 28684
Voluntary context switches: 1
Involuntary context switches: 186
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0
After:
Command being timed: "/home/dfc/go/pkg/tool/linux_arm/5l -o
/tmp/go-build464152087/cmd/godoc/_obj/a.out -L /tmp/go-build464152087
/tmp/go-build464152087/cmd/godoc.a"
User time (seconds): 3.89
System time (seconds): 0.31
Percent of CPU this job got: 99%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:04.20
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 99432
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 25051
Voluntary context switches: 0
Involuntary context switches: 21
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0
14mb saved, not too shabby.
On Tue, Jan 15, 2013 at 9:20 PM, minux <minux.ma@gmail.com> wrote:
>
>
> On Tue, Jan 15, 2013 at 5:58 PM, <dave@cheney.net> wrote:
>>
>> Saves 4 bytes, under gcc sizeof(Sym): 136 => 132. Under clang 3.2 on
>> freebsd the size of Sym *increased* from 128 => 132, which I *think* is
>
> I've tested latest version of clang on Linux/ARM, and sizeof(Sym) correctly
> shrink from 136 bytes to 132 bytes.
>
> I don't think FreeBSD/ARM should have any deviation from this.
>>
>> due to freebsd using the armv6 model, and gcc probably using armv4t, so
>> will not allow unaligned shorts and words.
>
> by default, clang shouldn't use unaligned load/stores, and should align
> all fields to its natural alignment, so I don't understand why clang is able
> to pack the old Sym structure into 128 bytes while gcc can't.
Sign in to reply to this message.
Hello minux.ma@gmail.com, remyoudompheng@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/7106050/diff/4004/src/cmd/5l/l.h File src/cmd/5l/l.h (left): https://codereview.appspot.com/7106050/diff/4004/src/cmd/5l/l.h#oldcode77 src/cmd/5l/l.h:77: Sym* gotype; i think gotype is needed if we want to generate DWARF debuginfo with correct variable typing.
Sign in to reply to this message.
Are you talking about the lack of proper dwarf symbols on arm ? If so, could we add this field back at the point someone is working on that? On 15 Jan 2013 21:30, <minux.ma@gmail.com> wrote: > > https://codereview.appspot.**com/7106050/diff/4004/src/cmd/**5l/l.h<https://c... > File src/cmd/5l/l.h (left): > > https://codereview.appspot.**com/7106050/diff/4004/src/cmd/** > 5l/l.h#oldcode77<https://codereview.appspot.com/7106050/diff/4004/src/cmd/5l/l.h#oldcode77> > src/cmd/5l/l.h:77: Sym* gotype; > i think gotype is needed if we want to generate > DWARF debuginfo with correct variable typing. > > https://codereview.appspot.**com/7106050/<https://codereview.appspot.com/7106... >
Sign in to reply to this message.
On Tue, Jan 15, 2013 at 6:52 PM, Dave Cheney <dave@cheney.net> wrote: > Are you talking about the lack of proper dwarf symbols on arm ? If so, > could we add this field back at the point someone is working on that? > Yes. Sure.
Sign in to reply to this message.
Thanks. Maybe by that time other savings will have materialised to make the size of Prog less important. Oh, and ponies. On 15 Jan 2013 21:59, "minux" <minux.ma@gmail.com> wrote: > > On Tue, Jan 15, 2013 at 6:52 PM, Dave Cheney <dave@cheney.net> wrote: > >> Are you talking about the lack of proper dwarf symbols on arm ? If so, >> could we add this field back at the point someone is working on that? >> > Yes. Sure. >
Sign in to reply to this message.
Did you find out why clang on FreeBSD is able to pack our struct smaller than gcc on Linux/ARM?
Sign in to reply to this message.
On 2013/01/15 18:45:02, minux wrote: > Did you find out why clang on FreeBSD is able to pack our struct smaller > than gcc on Linux/ARM? I cannot replicate the original sizeof(Sym) => 128 result, I must have made a mistake originally. I have updated the description to remove this incorrect statement.
Sign in to reply to this message.
LGTM.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/7106050/diff/4004/src/cmd/5l/l.h File src/cmd/5l/l.h (left): https://codereview.appspot.com/7106050/diff/4004/src/cmd/5l/l.h#oldcode77 src/cmd/5l/l.h:77: Sym* gotype; On 2013/01/15 10:30:56, minux wrote: > i think gotype is needed if we want to generate > DWARF debuginfo with correct variable typing. please keep gotype.
Sign in to reply to this message.
>> i think gotype is needed if we want to generate >> DWARF debuginfo with correct variable typing. > > > please keep gotype. Of the 12 bytes save, dropping the pair of unused (at the moment) gotype fields is 8 bytes. I know they may be useful in the future, but right now the cost 10% of the memory footprint when linking godoc.
Sign in to reply to this message.
I expect it to be important in the near future. I don't want to make something better only to have to undo it later. I have some other ideas for making the linker memory footprint go down. Russ
Sign in to reply to this message.
fair enough, that is good to know. Currently these two defines #define datasize reg #define textflag reg use the otherwise unsed (I belive) Prog.reg field. If they could be moved somewhere else, Prog.reg and Prog.align (also unused) could move removed from the struct, which would save a word due to alignment. On Sat, Jan 19, 2013 at 8:22 AM, Russ Cox <rsc@golang.org> wrote: > I expect it to be important in the near future. I don't want to make > something better only to have to undo it later. > I have some other ideas for making the linker memory footprint go down. > > Russ
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=9dacae220eca *** cmd/5l: reduce the size of Prog and Sym Prog * Remove the unused Prog* dlink * note that align is also unused, but removing it does not help due to alignment issues. Saves 4 bytes, sizeof(Prog): 84 => 80. Sym * Align {u,}char fields on word boundaries Saves 4 bytes, sizeof(Sym): 136 => 132. Tested on linux/arm and freebsd/arm. R=minux.ma, remyoudompheng, rsc CC=golang-dev https://codereview.appspot.com/7106050
Sign in to reply to this message.
|
