[PATCH] D147960: [lld-macho] Cache discovered dylib paths

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

Thanks for looking into this!



================
Comment at: lld/MachO/DriverUtils.cpp:189
 
+static DenseMap<CachedHashStringRef, StringRef> resolvedDylibs;
 std::optional<StringRef> macho::resolveDylibPath(StringRef dylibPath) {
----------------
nit: can we call this `resolvedDylibPaths`? Especially since there's `loadedDylibs` below which is actually storing `DylibFile *`s as values

also maybe the comments on lines 218-219 could be hoisted up above this declaration


================
Comment at: lld/MachO/DriverUtils.cpp:191-194
+  CachedHashStringRef key(dylibPath);
+  auto entry = resolvedDylibs.find(key);
+  if (entry != resolvedDylibs.end())
+    return entry->second;
----------------
how do you feel about using a similar approach to `loadDylib` below where we take a reference to the map value and then modify it? Aside from saving us two lines of `resolvedDylibs[key] = file`, it also serves as a negative cache -- if the key exists but the value is an empty string, then we can just return `std::nullopt`

(I notice we don't actually do this negative cache checking in `loadDylib`, but I guess in that case it matters less since we don't expect that cache lookup to fail in the common error-free case)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147960/new/

https://reviews.llvm.org/D147960