[PATCH] D43511: [dsymutil] Be smarter in caching calls to realpath
Adrian Prantl via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 20 08:51:55 PST 2018
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
More information about the llvm-commits
mailing list