This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Cache discovered framework paths
ClosedPublic

Authored by keith on Nov 2 2021, 3:07 PM.

Details

Reviewers
gkm
int3
Group Reviewers
Restricted Project
Commits
rG63e65de3ffc2: [lld-macho] Cache discovered framework paths
Summary

On our large iOS project this took a link from 1 minute 45 seconds to 45
seconds. For reference ld64 does the same link in ~20 seconds.

Diff Detail

Event Timeline

keith created this revision.Nov 2 2021, 3:07 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
keith requested review of this revision.Nov 2 2021, 3:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2021, 3:07 PM
int3 added a subscriber: int3.Nov 2 2021, 7:20 PM
int3 added inline comments.
lld/MachO/Driver.cpp
95

given that D113073 calls its map resolvedLibraries, maybe this should be resolvedFrameworks?

95

also we should make sure to reset this map in cleanupCallback (see D112878: [MachO] Properly reset global state)

117–119

code style is to use explicit type names instead of auto unless the type is something complicated like an iterator

also I would personally prefer return resolvedPaths[key] = second;, but up to you

keith updated this revision to Diff 384325.Nov 2 2021, 9:18 PM
keith marked 3 inline comments as done.

Update format and add cleanup clear

lld/MachO/Driver.cpp
117–119

Thanks for the feedback! Updated accordingly.

Is it possible for us to setup some automation here to flag these?

keith updated this revision to Diff 384333.Nov 2 2021, 9:30 PM

Fix patch

oontvoo added a subscriber: oontvoo.Nov 3 2021, 7:36 AM
oontvoo added inline comments.
lld/MachO/Driver.cpp
117–119

Is it possible for us to setup some automation here to flag these?

I suspect it's hard because the rules can be ambiguous and sometimes opinion-based :)
The usual "rules of thumb" I'd use is that if it's hard to tell at the callsite what the type is, then it should be spelled out. (even then there're exceptions like the iterator's types and lambda, etc)

int3 accepted this revision.Nov 3 2021, 7:47 AM

Thanks!

lld/MachO/Driver.cpp
117–119

yeah I'm not aware of a way to lint for this...

thanks for shortening the return + assignment! I was actually thinking we could go all the way and do away with saved too, i.e. just return resolvedFrameworks[key] = saver.save(suffixed.str());... then the if wouldn't need braces

126–127

ditto, we could return the assignment value and do away with the braces

This revision is now accepted and ready to land.Nov 3 2021, 7:47 AM
keith updated this revision to Diff 384529.Nov 3 2021, 11:12 AM
keith marked 2 inline comments as done.

Fix style

This revision was automatically updated to reflect the committed changes.