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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:103
 
+/// Small helper that resolves and caches file paths.
+class CachedPathResolver {
----------------
Can you add some of the explanatory text from the review description here? I.e., that realpath is expensive?


================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:106
+public:
+  /// Resolve a path and cache its result.
+  std::string resolve(std::string Path) {
----------------
Resolve a path by calling realpath() ...


================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:122
+    sys::path::append(ResolvedPath, FileName);
+    return StringRef(ResolvedPath);
+  }
----------------
would it be more efficient to instead return the SmallString<256> directly, so it gets allocated by the caller and we avoid the copy upon return? I haven't looked at what the caller is doing with it.


================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:125
+private:
+  StringMap<SmallString<256>> ResolvedPaths;
+};
----------------
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?


================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:1949
+            std::string File;
+            bool FoundFile =
+              LT->getFileNameByIndex(FileNum, "",
----------------
Might be more efficient to wrap this in an
```
#ifndef NDEBUG
#endif
```
block.


https://reviews.llvm.org/D43511