Please review "git diff origin/master..origin/pmachata/reader_hooks". If you like it fine, we'll merge the branch into master. (There was some flutter in that branch history, so you might prefer to do a --squash merge.)
It's intended to be a no-op change for now, aside from a little added length checking and the like. It paves the way for treating relocatable files more intelligently later on.
Thanks, Roland
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Roland McGrath wrote:
Please review "git diff origin/master..origin/pmachata/reader_hooks". If you like it fine, we'll merge the branch into master.
What's the performance impact? How will it be when the more sophisticated reader hooks are used? There are an awful lot of nested function calls. Although they are marked inline, are we sure gcc sees through that?
Details: don't define more than one variable per line. E.g.,
+ unsigned char *readp, *readendp;
Aside from that it looks OK.
- -- ➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
What's the performance impact?
We have not measured. I was assuming nil or close because of the inlines.
How will it be when the more sophisticated reader hooks are used?
My expectation is that these will be inlines doing:
if (dbg->reloc) return fancy_business(); return read_blah_unaligned (...);
So it should be negligible for any non-ET_REL file. For ET_REL the cost of the fine-grained fancy business has to be compared to the full cost of doing __libdwfl_relocate beforehand with however many COW faults that entails. The usage pattern where this matters today is systemtap loading numerous .ko's, but then scanning through them quickly and actually consulting only a small subset of the data so a fraction of the reloc sites will actually incur the overhead (vs a huge startup delay with libdwfl's current eager relocation of whole sections in every module).
There are an awful lot of nested function calls. Although they are marked inline, are we sure gcc sees through that?
I don't think we have looked (probably not even tried an -O2 build).
Petr, can you eyeball the generated code, compare text sizes, etc.?
Thanks, Roland
Roland McGrath wrote:
There are an awful lot of nested function calls. Although they are marked inline, are we sure gcc sees through that?
I don't think we have looked (probably not even tried an -O2 build).
When I pass no CFLAGS to configure, -O2 gets picked by default.
Petr, can you eyeball the generated code, compare text sizes, etc.?
On x86_64, -O1 through -O3, overall change in .text is 2%. Sizes of particular functions change thus on -O2:
SYMBOL OLD NEW DELTA FRACTION dwarf_formaddr 126 288 +162 2.29 dwarf_formref_die 200 407 +207 2.04 dwarf_formstring 162 365 +203 2.25 dwarf_formudata 389 569 +180 1.46 dwarf_getaranges 1144 1518 +374 1.33 dwarf_getlocation 115 118 +3 1.03 dwarf_getlocation_addr 1117 948 -169 0.85 dwarf_getpubnames 1247 1365 +118 1.09 dwarf_getsrclines 5358 5500 +142 1.03 dwarf_nextcu 453 642 +189 1.42 dwarf_ranges 694 797 +103 1.15 dwfl_module_addrsym 973 965 -8 0.99 getlocation 1585 1796 +211 1.13
I've shuffled a couple pieces of code around, and metrics got slightly better (the table got generated from that improved code). That's now committed. Though I'll have to restart the tests for that.
With -O3, the numbers (DELTA and FRACTION) are very similar (even though OLD and NEW are generally larger). With -O1, DELTAs tend to be smaller. Everything still gets inlined (or at least symbols don't show up in symbol tables).
Does the above look acceptable, or shall I delve in and try to get rid of always-true tests and such?
PM
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Petr Machata wrote:
On x86_64, -O1 through -O3, overall change in .text is 2%. Sizes of particular functions change thus on -O2: [...]
Did you add something to the hook functions similar to what will be there later on? I.e., as Roland said
if (SOMETHING) return SOMEFUNC();
This is the real test since this is the ultimate goal of this change.
- -- ➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
Ulrich Drepper wrote:
Petr Machata wrote:
On x86_64, -O1 through -O3, overall change in .text is 2%. Sizes of particular functions change thus on -O2: [...]
Did you add something to the hook functions similar to what will be there later on? I.e., as Roland said
if (SOMETHING) return SOMEFUNC();
This is the real test since this is the ultimate goal of this change.
Right. With two fancy_business functions added (for offset and for address) and called conditionally on a new, always-zero field from struct Dwarf, the metrics on x86_64, -O2 build, change thus:
dwarf_formaddr 126 366 +240 2.90 dwarf_formref_die 200 463 +263 2.31 dwarf_formstring 162 437 +275 2.70 dwarf_formudata 389 678 +289 1.74 dwarf_getaranges 1144 1700 +556 1.49 dwarf_getlocation 115 118 +3 1.03 dwarf_getlocation_addr 1117 948 -169 0.85 dwarf_getpubnames 1247 1506 +259 1.21 dwarf_getsrclines 5358 5428 +70 1.01 dwarf_nextcu 453 763 +310 1.68 dwarf_ranges 694 797 +103 1.15 getlocation 1585 1924 +339 1.21
Total change in .text is then 2.9%.
PM
I reorganized the code a bit more, trying not to really sacrifice readability and other qualities. Now (with fancy_business dummies in place, these are obviously not committed), the code size changes thus on x86_64:
dwarf_formaddr 126 263 +137 2.09 dwarf_formref_die 200 443 +243 2.21 dwarf_formstring 162 345 +183 2.13 dwarf_formudata 389 598 +209 1.54 dwarf_getaranges 1144 1610 +466 1.41 dwarf_getlocation 115 118 +3 1.03 dwarf_getlocation_addr 1117 948 -169 0.85 dwarf_getpubnames 1247 1387 +140 1.11 dwarf_getsrclines 5358 5407 +49 1.01 dwarf_nextcu 453 721 +268 1.59 dwarf_ranges 694 797 +103 1.15 getlocation 1585 1829 +244 1.15
That's similar to the numbers I posted first time around, and we're back to 2.0% of .text increase. The tests are spinning now.
I also got rid of the cosmetic thing with defining multiple variables per line. Both is on the branch.
PM
To be fair, don't these now do some valid-offset checks that we didn't have before (and had XXX comments instead)? So that 1-2% is not doing nothing.
Thanks, Roland
Roland McGrath wrote:
To be fair, don't these now do some valid-offset checks that we didn't have before (and had XXX comments instead)? So that 1-2% is not doing nothing.
Yes, true. Still the fact is that the binary overhead could be cut from almost 3% to straight 2% with a couple of source edits (that, IMHO, don't mess the code up), so why not do it...
PM
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
I've applied the patch. If the performance is too bad going format we'll have to think of something.
- -- ➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
elfutils-devel@lists.fedorahosted.org