[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

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]

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]

Fix patch

[oontvoo]

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]

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

[keith]

Fix style

lld/MachO/Driver.cpp

if (Optional<StringRef> path = findPathCombination(
       return path;
     return findPathCombination("lib" + name, config->librarySearchPaths,
                                {".a"});
   }
   return findPathCombination("lib" + name, config->librarySearchPaths,
                              {".tbd", ".dylib", ".a"});
 }
 
+static DenseMap<CachedHashStringRef, StringRef> resolvedPaths;
 static Optional<StringRef> findFramework(StringRef name) {
+  CachedHashStringRef key(name);
+  auto entry = resolvedPaths.find(key);
+  if (entry != resolvedPaths.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());
+        if (fs::exists(suffixed)) {
+          auto saved = saver.save(suffixed.str());
+          resolvedPaths[key] = saved;
+          return saved;
+        }
       }
       // Suffix lookup failed, fall through to the no-suffix case.
     }
 
-    if (Optional<StringRef> path = resolveDylibPath(symlink.str()))
+    if (Optional<StringRef> path = resolveDylibPath(symlink.str())) {
+      resolvedPaths[key] = *path;
       return 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)) {