Avoid dsymutil calls to getFileNameByIndex

[pete]

Summary

Hi Fred

getFileNameByIndex calls sys::path::append and (transitively) malloc a large number of times.

The call site in getChildDeclContext already caches resolved paths for a given FileNum if we have the file name by its index.

This patch adds a hasFileNameByIndex method. getChildDeclContext can first call this method, and if it returns true it knows it can then lookup the resolved path cache for the given file index. If we hit that cache then we don't even have to call getFileNameByIndex.

Running dsymutil on the swift executable generated from the GitHub sources, before this change took 100s on my iMac. Afterwards took 80s. So a nice 20% speedup. I ran a few times on each configuration to make sure the numbers are quite stable. I also diffed the dSYM produced before and after this patch and they are binary identical.

Thanks,
Pete

[friss]

This LGTM if you agree with my comments. Thanks for the nice speed boost!

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

191–192

I know you modeled the name on the function below, but it reads strange to me. How about just

bool hasFileAtIndex(uint64_t FileIndex) const;

(ie also drop the Kind parameter, I don't think it makes sense to test it here.)

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

630–633

I like

return FileIndex != 0 && FileIndex <= Prologue.FileNames.size() ;

better (if you drop the Kind like I said above)

641

Then this becomes:

if (Kind == FileLineInfoKind::None || !hasFileAtIndex(FileIndex))

[pete]

Thanks for the quick review Fred. Those comments make complete sense so have committed with those changes.

r276380

Cheers,
Pete

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

struct LineTable {
 
     // Returns the index of the row with file/line info for a given address,
     // or UnknownRowIndex if there is no such row.
     uint32_t lookupAddress(uint64_t address) const;
 
     bool lookupAddressRange(uint64_t address, uint64_t size,
                             std::vector<uint32_t> &result) const;
 
+    bool hasFileNameByIndex(uint64_t FileIndex,
+                            DILineInfoSpecifier::FileLineInfoKind Kind) const;
+
     // Extracts filename by its index in filename table in prologue.
     // Returns true on success.
     bool getFileNameByIndex(uint64_t FileIndex, const char *CompDir,
                             DILineInfoSpecifier::FileLineInfoKind Kind,
                             std::string &Result) const;
 
     // Fills the Result argument with the file and line information
     // corresponding to Address. Returns true on success.

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

while (seq_pos != last_seq && seq_pos->LowPC < end_addr) {
     }
 
     ++seq_pos;
   }
 
   return true;
 }
 
-bool DWARFDebugLine::LineTable::getFileNameByIndex(uint64_t FileIndex,
-                                                   const char *CompDir,
-                                                   FileLineInfoKind Kind,
-                                                   std::string &Result) const {
-  if (FileIndex == 0 || FileIndex > Prologue.FileNames.size() ||
-      Kind == FileLineInfoKind::None)
+bool
+DWARFDebugLine::LineTable::hasFileNameByIndex(uint64_t FileIndex,
+                                              FileLineInfoKind Kind) const {
+  if (FileIndex == 0 || FileIndex > Prologue.FileNames.size() ||
+      Kind == FileLineInfoKind::None)
+    return false;
+  return true;
+}
+
+bool
+DWARFDebugLine::LineTable::getFileNameByIndex(uint64_t FileIndex,
+  if (!hasFileNameByIndex(FileIndex, Kind))
     return false;
   const FileNameEntry &Entry = Prologue.FileNames[FileIndex - 1];
   const char *FileName = Entry.Name;
   if (Kind != FileLineInfoKind::AbsoluteFilePath ||
       sys::path::is_absolute(FileName)) {
     Result = FileName;
     return true;
   }

tools/dsymutil/DwarfLinker.cpp

if (Tag != dwarf::DW_TAG_namespace || !Name) {
           // unique anything in anonymous namespaces, but if we do, then
           // verify that the file and line correspond.
           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.
-          std::string File;
-          if (LT->getFileNameByIndex(
-                  FileNum, "",
-                  DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath,
-                  File)) {
+          if (LT->hasFileNameByIndex(FileNum,
+                    DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath)) {
             Line = DIE->getAttributeValueAsUnsignedConstant(
                 &U.getOrigUnit(), dwarf::DW_AT_decl_line, 0);
             // Cache the resolved paths, because calling realpath is expansive.
             StringRef ResolvedPath = U.getResolvedPath(FileNum);
             if (!ResolvedPath.empty()) {
               FileRef = ResolvedPath;
             } else {
 #ifdef HAVE_REALPATH
+              std::string File;
+              bool gotFileName =
+                LT->getFileNameByIndex(FileNum, "",
+                        DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath,
+                        File);
+              (void)gotFileName;
+              assert(gotFileName && "Must get file name from line table");
               char RealPath[PATH_MAX + 1];
               RealPath[PATH_MAX] = 0;
               if (::realpath(File.c_str(), RealPath))
                 File = RealPath;
 #endif
               FileRef = StringPool.internString(File);
               U.setResolvedPath(FileNum, FileRef);
             }