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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere updated this revision to Diff 135211.
JDevlieghere marked an inline comment as done.
JDevlieghere edited the summary of this revision.
JDevlieghere added a comment.

Originally I removed the first level of caching (based on the file index in the line table), which turned out to make things slower. That makes sense, because it's really fast (indexed) and because we don't have to read the line table again for files we've already seen.

Results of running dsymutil on WebkitCore:

  LT caching only
  85.76 real        72.54 user        12.99 sys
  85.69 real        72.62 user        12.84 sys
  85.93 real        72.66 user        13.11 sys
  
  ParentPath caching only
  97.68 real        92.57 user         4.94 sys
  96.27 real        91.38 user         4.72 sys
  96.67 real        91.64 user         4.82 sys
  
  Both LT and ParentPath caching
  75.84 real        70.81 user         4.77 sys
  76.27 real        71.26 user         4.83 sys
  76.22 real        71.33 user         4.75 sys


https://reviews.llvm.org/D43511

Files:
  llvm/tools/dsymutil/DwarfLinker.cpp


Index: llvm/tools/dsymutil/DwarfLinker.cpp
===================================================================
--- llvm/tools/dsymutil/DwarfLinker.cpp
+++ llvm/tools/dsymutil/DwarfLinker.cpp
@@ -100,6 +100,37 @@
 
 namespace {
 
+/// Small helper that resolves and caches file paths. This helps reduce the
+/// number of calls to realpath which is expensive. We assume the input are
+/// files, and cache the realpath of their parent. This way we can quickly
+/// resolve different files under the same path.
+class CachedPathResolver {
+public:
+  /// Resolve a path by calling realpath and cache its result. The returned
+  /// StringRef is interned in the given \p StringPool.
+  StringRef resolve(std::string Path, NonRelocatableStringpool &StringPool) {
+    StringRef FileName = sys::path::filename(Path);
+    SmallString<PATH_MAX> ParentPath = sys::path::parent_path(Path);
+
+    // If the ParentPath has not yet been resolved, resolve and cache it for
+    // future look-ups.
+    if (!ResolvedPaths.count(ParentPath)) {
+      SmallString<PATH_MAX> RealPath;
+      sys::fs::real_path(ParentPath, RealPath);
+      ResolvedPaths.insert({ParentPath, StringRef(RealPath).str()});
+    }
+
+    // Join the file name again with the resolved path.
+    SmallString<PATH_MAX> ResolvedPath(ResolvedPaths[ParentPath]);
+    sys::path::append(ResolvedPath, FileName);
+    return StringPool.internString(ResolvedPath);
+  }
+
+private:
+  StringMap<std::string> ResolvedPaths;
+};
+
+
 /// Retrieve the section named \a SecName in \a Obj.
 ///
 /// To accommodate for platform discrepancies, the name passed should be
@@ -234,6 +265,8 @@
   DeclContext Root;
   DeclContext::Map Contexts;
 
+  /// Cache resolved paths from the line table.
+  CachedPathResolver PathResolver;
 public:
   /// Get the child of \a Context described by \a DIE in \a Unit. The
   /// required strings will be interned in \a StringPool.
@@ -1945,30 +1978,24 @@
           if (!Name && Tag == dwarf::DW_TAG_namespace)
             FileNum = 1;
 
-          // FIXME: Passing U.getOrigUnit().getCompilationDir()
-          // instead of "" would allow more uniquing, but for now, do
-          // it this way to match dsymutil-classic.
           if (LT->hasFileAtIndex(FileNum)) {
             Line = dwarf::toUnsigned(DIE.find(dwarf::DW_AT_decl_line), 0);
-            // Cache the resolved paths, because calling realpath is expansive.
+            // Cache the resolved paths based on the index in the line table,
+            // because calling realpath is expansive.
             StringRef ResolvedPath = U.getResolvedPath(FileNum);
             if (!ResolvedPath.empty()) {
               FileRef = ResolvedPath;
             } else {
               std::string File;
-              bool gotFileName =
-                LT->getFileNameByIndex(FileNum, "",
-                        DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath,
-                        File);
-              (void)gotFileName;
-              assert(gotFileName && "Must get file name from line table");
-#ifdef HAVE_REALPATH
-              char RealPath[PATH_MAX + 1];
-              RealPath[PATH_MAX] = 0;
-              if (::realpath(File.c_str(), RealPath))
-                File = RealPath;
-#endif
-              FileRef = StringPool.internString(File);
+              bool FoundFileName = LT->getFileNameByIndex(
+                  FileNum, U.getOrigUnit().getCompilationDir(),
+                  DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath,
+                  File);
+              (void)FoundFileName;
+              assert(FoundFileName && "Must get file name from line table");
+              // Second level of caching, this time based on the file's parent
+              // path.
+              FileRef = PathResolver.resolve(File, StringPool);
               U.setResolvedPath(FileNum, FileRef);
             }
           }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43511.135211.patch
Type: text/x-patch
Size: 3914 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180221/275af245/attachment.bin>