This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho][nfc] Reduce size of icfEqClass hash
ClosedPublic

Authored by int3 on Mar 5 2022, 9:51 AM.

Details

Reviewers
MaskRay
thakis
Group Reviewers
Restricted Project
Commits
rGad1c32e9b383: [lld-macho][nfc] Reduce size of icfEqClass hash
Summary

... from a uint64_t to a uint32_t. (LLD-ELF uses a uint32_t too.)

About a 1.7% reduction in peak RSS when linking chromium_framework on my
3.2 GHz 16-Core Intel Xeon W Mac Pro, and no stat sig change in wall
time.

         </Users/jezng/test2.sh ["before"]>  </Users/jezng/test2.sh ["after"]>  difference (95% CI)
RSS      1003036672.000 ± 9891065.259        985539505.231 ± 10272748.749       [  -2.3% ..   -1.2%]
samples  27                                  26

           base           diff           difference (95% CI)
sys_time   1.277 ± 0.023  1.277 ± 0.024  [  -0.9% ..   +0.9%]
user_time  6.682 ± 0.046  6.598 ± 0.043  [  -1.6% ..   -0.9%]
wall_time  5.904 ± 0.062  5.895 ± 0.063  [  -0.7% ..   +0.4%]
samples    46             28

No appreciable change (~0.01%) in number of equals comparisons either:

Before:

ld64.lld: ICF needed 8 iterations
ld64.lld: equalsConstant() called 701643 times
ld64.lld: equalsVariable() called 3438526 times

After:

ld64.lld: ICF needed 8 iterations
ld64.lld: equalsConstant() called 701729 times
ld64.lld: equalsVariable() called 3438526 times

Diff Detail

Event Timeline

int3 created this revision.Mar 5 2022, 9:51 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 5 2022, 9:51 AM
int3 requested review of this revision.Mar 5 2022, 9:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2022, 9:51 AM
int3 edited the summary of this revision. (Show Details)Mar 5 2022, 3:32 PM
thakis added a subscriber: thakis.Mar 5 2022, 3:50 PM
thakis added inline comments.
lld/MachO/ICF.cpp
278–279

what happened to the dylibSym branch?

281–283

if you want to rename this variable (why?), do that in a separate commit (doesn't need a review)

MaskRay accepted this revision.Mar 5 2022, 8:16 PM
MaskRay added a subscriber: MaskRay.

LGTM barring the comment. You may want to change std::vector<Reloc> to SmallVector<Reloc, 0> to make it smaller.

This revision is now accepted and ready to land.Mar 5 2022, 8:16 PM
int3 marked an inline comment as done.Mar 6 2022, 11:59 PM

both the changes you commented on were accidentally included in this diff... will split them out

lld/MachO/ICF.cpp
278–279

oops, I didn't intend to fold that change into this commit. I'll commit this separately

but as for the rationale... the existing hashing of stubsHelperIndex is actually a no-op because ICF runs before dylib symbols get their stubs index assigned. I guess we could consider hashing the name + filename of the DylibSymbol instead, but I'm not sure the overhead's worth it... also LLD/ELF only hashes their Defined symbols as well.

int3 added inline comments.Mar 7 2022, 12:01 AM
lld/MachO/ICF.cpp
281–283

why?

It's shadowing the isec defined on line 275, which I found a bit confusing to read

int3 updated this revision to Diff 413356.Mar 7 2022, 12:11 AM
int3 marked an inline comment as done.

split out unrelated changes

thakis accepted this revision.Mar 7 2022, 5:23 AM

thanks!

int3 added a comment.Mar 7 2022, 9:28 AM

You may want to change std::vector<Reloc> to SmallVector<Reloc, 0> to make it smaller.

Good idea! I'll benchmark it in a separate diff.

This revision was landed with ongoing or failed builds.Mar 7 2022, 9:38 AM
This revision was automatically updated to reflect the committed changes.
thakis added inline comments.Mar 30 2022, 1:30 PM
lld/MachO/ICF.cpp
373

Should this be a uint32_t now? (doesn't matter in practice; I just found it confusing when I was reading the code just now)

int3 added inline comments.Mar 30 2022, 1:33 PM
lld/MachO/ICF.cpp
373

yep, forgot to change this