[lld-macho] Cache discovered framework paths

[keith]

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.

[int3]

lld/MachO/Driver.cpp

109

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

109

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

132–134

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]

Update format and add cleanup clear

lld/MachO/Driver.cpp

132–134

Thanks for the feedback! Updated accordingly.

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

[keith]

Fix patch

[oontvoo]

lld/MachO/Driver.cpp

132–134

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]

Thanks!

lld/MachO/Driver.cpp

132–134

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

137–138

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

[keith]

Fix style

lld/MachO/Driver.cpp

static Optional<StringRef> findLibrary(StringRef name) {
 
   Optional<StringRef> path = doFind();
   if (path)
     resolvedLibraries[key] = *path;
 
   return path;
 }
 
+static DenseMap<CachedHashStringRef, StringRef> resolvedFrameworks;
 static Optional<StringRef> findFramework(StringRef name) {
+  CachedHashStringRef key(name);
+  auto entry = resolvedFrameworks.find(key);
+  if (entry != resolvedFrameworks.end())
+    return entry->second;
+
   SmallString<260> symlink;
   StringRef suffix;
   std::tie(name, suffix) = name.split(",");
   for (StringRef dir : config->frameworkSearchPaths) {
     symlink = dir;
     path::append(symlink, name + ".framework", name);
 
     if (!suffix.empty()) {
       // NOTE: we must resolve the symlink before trying the suffixes, because
       // there are no symlinks for the suffixed paths.
       SmallString<260> location;
       if (!fs::real_path(symlink, location)) {
         // only append suffix if realpath() succeeds
         Twine suffixed = location + suffix;
         if (fs::exists(suffixed))
-          return saver.save(suffixed.str());
+          return resolvedFrameworks[key] = saver.save(suffixed.str());
       }
       // Suffix lookup failed, fall through to the no-suffix case.
     }
 
     if (Optional<StringRef> path = resolveDylibPath(symlink.str()))
-      return path;
+      return resolvedFrameworks[key] = *path;
   }
   return {};
 }
 
 static bool warnIfNotDirectory(StringRef option, StringRef path) {
   if (!fs::exists(path)) {
     warn("directory not found for option -" + option + path);
     return false;
   } else if (!fs::is_directory(path)) {

 bool macho::link(ArrayRef<const char *> argsArr, bool canExitEarly,
                  raw_ostream &stdoutOS, raw_ostream &stderrOS) {
   lld::stdoutOS = &stdoutOS;
   lld::stderrOS = &stderrOS;
 
   errorHandler().cleanupCallback = []() {
     freeArena();
 
+    resolvedFrameworks.clear();
     resolvedLibraries.clear();
     concatOutputSections.clear();
     inputFiles.clear();
     inputSections.clear();
     loadedArchives.clear();
     syntheticSections.clear();
     thunkMap.clear();