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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere marked 2 inline comments as done.
JDevlieghere added a comment.

In https://reviews.llvm.org/D43511#1013421, @davide wrote:

> The meta comment I have here is whether we need to segregate this is in `dsymutil` or expose that somewhere else (e.g. in `Support/`).


As long as only `dsymutil` is the only user of this code, I think it should live here.

> My feeling is that this utility could be of more general use for pathname resolution.

I agree and that's why I kept it as generic as possible. (Until I added the StringPool argument, which totally makes sense here)

> Among others, `lldb` calls realpath a bit, and maybe there are components in clang doing the same. Jonas, what do you think?

Is there any particular call to realpath you're thinking about? From what I saw there weren't that many call sites that would benefit from this optimization. It works very well for dsymutil, because it's not uncommon for several object files to live in the same directory. If you're resolving unrelated paths, you end up worse because of the caching overhead.

But maybe I missed something... If that's the case I'm more than happy to move this into support!



================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:1949
+            std::string File;
+            bool FoundFile =
+              LT->getFileNameByIndex(FileNum, "",
----------------
aprantl wrote:
> JDevlieghere wrote:
> > aprantl wrote:
> > > Might be more efficient to wrap this in an
> > > ```
> > > #ifndef NDEBUG
> > > #endif
> > > ```
> > > block.
> > I gave it a shot but I'm not convinced it's better :-) 
> Does the call to getFileNameByIndex get optimized away? If yes then that's okay, otherwise I'd prefer the NDEBUG block.
No the call is what populates the `File`. It's just the return type that's checked. Wrapping it in an IFDEF looks weird because you split off the variable and the equals sign from the call. It breaks the flow of the code and adds two lines compared to one for the (void).


https://reviews.llvm.org/D43511