[PATCH] D43511: [dsymutil] Be smarter in caching calls to realpath

[Frederic Riss via Phabricator via llvm-commits]

friss accepted this revision.
friss added inline comments.


================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:125
+private:
+  StringMap<SmallString<256>> ResolvedPaths;
+};
----------------
JDevlieghere wrote:
> friss wrote:
> > aprantl wrote:
> > > aprantl wrote:
> > > > I don't know how many entries this StringMap is going to have, but this looks expensive in terms of memory usage. Perhaps consider using a StringMap<std::string> instead?
> > > This is now interning a temporary result that is getting interned again at the call site?
> > > 
> > > I think it would make more sense to intern the string that is entered into the StringMap and make it a StringMap<StringRef>. Or both? Fred?
> > > This is now interning a temporary result that is getting interned again at the call site?
> > > 
> > > I think it would make more sense to intern the string that is entered into the StringMap and make it a StringMap<StringRef>. Or both? Fred?
> > 
> > Yes, that's what I had mind, sorry for not having been clear. internString will provide permanent storage. I would never create a std::string, call internString on ResolvedPath and store a StringRef in the StringMap.
> I missed this comment, the string being interened again at the call site was a bug and has since been removed.
> 
> Unless I'm missing something, storing the interened string in the map makes no sense. We're not caching the full path, only the *parent path* which is shared between objects living in the same directory. That's the whole point of this (what is now caleld second level) cache. I've already updated the documentation to make this more explicit. 
> 
> Please let me know if you think there's still an issue with the current implementation. 
Of course, you're correct.


https://reviews.llvm.org/D43511