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

[Keith Smiley via Phabricator via llvm-commits]

keith added inline comments.


================
Comment at: lld/MachO/DriverUtils.cpp:191-194
+  CachedHashStringRef key(dylibPath);
+  auto entry = resolvedDylibs.find(key);
+  if (entry != resolvedDylibs.end())
+    return entry->second;
----------------
int3 wrote:
> keith wrote:
> > int3 wrote:
> > > 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)
> > I think I'm missing the C++-ism for how to do this, in this case the value isn't a pointer to a StringRef, can we still use it in that way or should it be changed to be that? Also right now a missing value / empty string won't ever get cached, do you think we should cache that in the fallthrough case to handle the negative cache case you're talking about?
> You don't need a pointer, just a reference :) basically I'm suggesting this (inserted after lines 193-196 in your diff)
> 
> ```
> auto entry = resolvedDylibPaths.find(key);
> if (entry != resolvedDylibPaths.end()) {
>   if (entry->second.empty())
>     return {};
>   else
>     return entry->second;
> }
> StringRef &s = resolvedDylibPaths[key]; // creates an empty string value entry for `key`
> ```
> 
> And then below would be
> 
> ```
> return s = saver.save(tbdPath.str());
> ```
ah duh thanks, i was thinking there was supposed to be some way where i was only doing a single fetch here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147960