This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Implement -why_live (without perf overhead)
ClosedPublic

Authored by int3 on Feb 22 2022, 6:02 PM.

Details

Reviewers
thakis
Group Reviewers
Restricted Project
Commits
rG850592ec14d0: [lld-macho] Implement -why_live (without perf overhead)
Summary

This was based off @thakis' draft in D103517: NOT FOR REVIEW: [lld/mac] why_live sketch. I employed templates to ensure
the support for -why_live wouldn't slow down the regular non-why-live code
path.

No stat sig perf difference on my 3.2 GHz 16-Core Intel Xeon W:

           base           diff           difference (95% CI)
sys_time   1.195 ± 0.015  1.199 ± 0.022  [  -0.4% ..   +1.0%]
user_time  3.716 ± 0.022  3.701 ± 0.025  [  -0.7% ..   -0.1%]
wall_time  4.606 ± 0.034  4.597 ± 0.046  [  -0.6% ..   +0.2%]
samples    44             37

Diff Detail

Event Timeline

int3 created this revision.Feb 22 2022, 6:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2022, 6:02 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: dang. · View Herald Transcript
int3 requested review of this revision.Feb 22 2022, 6:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2022, 6:02 PM
thakis accepted this revision.Feb 23 2022, 6:57 AM

Neat!

lld/MachO/MarkLive.cpp
111

can you add a comment on why this doesn't use message()?

118

errs() is unbuffered by default, no?

191

Do you want a makeEntry(nullptr, nullptr) for Undefineds here, or a nullptr?

Hm, I guess an Undefined never has relocs. Do we always have a Defined here? If so, we should probably just cast<> without conditional.

If there are non-Undefined non-Defined cases, the original question if you want makeEntry(nullptr, nullptr) stands for that :)

This revision is now accepted and ready to land.Feb 23 2022, 6:57 AM
int3 added inline comments.Feb 23 2022, 8:52 AM
lld/MachO/MarkLive.cpp
111

oh yeah... I was thinking about how we would handle parallelization in the future, and I realized that message() wasn't ideal here because it would take one lock per call, whereas we want to take one lock for the whole of printWhyLiveImpl so that the lines wouldn't get interleaved with other concurrent messages. I'll add that comment.

118

I'd have thought so, but I didn't get messages emitted before adding this flush()...

int3 updated this revision to Diff 410841.Feb 23 2022, 9:02 AM
int3 marked 2 inline comments as done.

add comment

thakis added inline comments.Feb 23 2022, 12:16 PM
lld/MachO/MarkLive.cpp
111

Oh, good point. But maybe it's nicer to accumulate the message in a string and only call message() once at the end? Then we need to hold the lock for a shorter time.

(Are our bumpptr allocators in a per-thread tls?)

191

(this is still open)

oontvoo added inline comments.
lld/MachO/MarkLive.cpp
111

(Are our bumpptr allocators in a per-thread tls?)

No - the allocators are shared (and very much not thread-safe :\ )

int3 updated this revision to Diff 411000.Feb 23 2022, 8:45 PM
int3 marked 4 inline comments as done.

update

lld/MachO/MarkLive.cpp
111

But maybe it's nicer to accumulate the message in a string and only call message() once at the end? Then we need to hold the lock for a shorter time.

Good point!

Also, in case you missed it, there's this discussion going on: https://discourse.llvm.org/t/parallel-thread-safe-algorithms-allocators-and-containers/60472

191

Oh whoops, almost forgot about this one.

We could have dylib or undefined symbols here too, yeah. In fact, I'd neglected to add a test for that... and adding that test revealed a null deref issue due to this makeEntry(nullptr, nullptr) thing. Good catch :)

I'll have makeEntry() itself do the null check so that we don't have an extra conditional in the non-why-live-enabled case.

This revision was landed with ongoing or failed builds.Feb 24 2022, 12:51 PM
This revision was automatically updated to reflect the committed changes.

Looks like this doesn't build on Linux (and windows): http://45.33.8.238/linux/69521/step_4.txt

Please take a look and revert for now if it takes a while to fix.

int3 added a comment.Feb 24 2022, 2:57 PM

Whoops, forgot to check the asserts build. Looks like I was beaten to the fix in da11f17e90a86525b8902f0812180799f8d8fca9.