[clang] [clang][modules] Avoid allocations when reading blob paths (PR #113984)

Jan Svoboda via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 29 13:26:56 PDT 2024


https://github.com/jansvoboda11 updated https://github.com/llvm/llvm-project/pull/113984

>From 709cc1c8c41603c4d9eb637cb0453521029af201 Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Mon, 28 Oct 2024 16:59:13 -0700
Subject: [PATCH 1/2] [clang][modules] Avoid allocations when reading blob
 paths

When reading paths from bitstream blobs, `ASTReader` always performs up to three allocations:
 1. Convert the `StringRef` blob to `std::string` right away.
 2. Concatenate the module file prefix directory and the relative path within `ASTReader::ResolveImportedPath()`.
 3. Call `std::string::assign()` on the allocated blob with the concatenation result.

 This patch makes is so that we avoid allocations altogether (amortized). This works by:
 1. Avoiding conversion of the `StringRef` blob into `std::string`.
 2. Keeping one "global" buffer to perform the concatenation with.
 3. Returning `StringRef` pointing into the global buffer and being careful to not store this anywhere where it can outlive the underlying global buffer allocation.

 Note that in some places, we don't store paths as blobs, but rather as records that get VBR-encoded. This makes the allocation in (1) unavoidable. I plan to fix this in a follow-up PR by changing the PCM format to store those as offsets into the blob part of the record.

 Similarly, there are some data structures that store the deserialized paths as `std::string`. If we don't access them frequently, it might be better to store just the unresolved `StringRef` and resolve it on demand, again using some kind of shared buffer to prevent allocations.

 This improves `clang-scan-deps` performance on my workload by <TBD>%.
---
 clang/include/clang/Serialization/ASTReader.h | 17 ++++-
 clang/lib/Serialization/ASTReader.cpp         | 70 ++++++++-----------
 2 files changed, 44 insertions(+), 43 deletions(-)

diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 070c1c9a54f48c..3c8bef484d0133 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -1341,9 +1341,22 @@ class ASTReader
   serialization::InputFile getInputFile(ModuleFile &F, unsigned ID,
                                         bool Complain = true);
 
+  /// Buffer we use as temporary storage backing resolved paths.
+  SmallString<256> PathBuf;
+
 public:
-  void ResolveImportedPath(ModuleFile &M, std::string &Filename);
-  static void ResolveImportedPath(std::string &Filename, StringRef Prefix);
+  StringRef ResolveImportedPath(StringRef Path, ModuleFile &M) {
+    return ResolveImportedPath(PathBuf, Path, M);
+  }
+  StringRef ResolveImportedPath(StringRef Path, StringRef Prefix) {
+    return ResolveImportedPath(PathBuf, Path, Prefix);
+  }
+  static StringRef ResolveImportedPath(SmallVectorImpl<char> &Buffer,
+                                       StringRef Path, ModuleFile &M) {
+    return ResolveImportedPath(Buffer, Path, M.BaseDirectory);
+  }
+  static StringRef ResolveImportedPath(SmallVectorImpl<char> &Buffer,
+                                       StringRef Path, StringRef Prefix);
 
   /// Returns the first key declaration for the given declaration. This
   /// is one that is formerly-canonical (or still canonical) and whose module
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 8d8f9378cfeabe..19f7f2be7f9011 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2050,8 +2050,7 @@ const FileEntry *HeaderFileInfoTrait::getFile(const internal_key_type &Key) {
     return nullptr;
   }
 
-  std::string Resolved = std::string(Key.Filename);
-  Reader.ResolveImportedPath(M, Resolved);
+  StringRef Resolved = Reader.ResolveImportedPath(Key.Filename, M);
   if (auto File = FileMgr.getOptionalFileRef(Resolved))
     return *File;
   return nullptr;
@@ -2150,9 +2149,9 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
     ModuleMap &ModMap =
         Reader.getPreprocessor().getHeaderSearchInfo().getModuleMap();
 
-    std::string Filename = std::string(key.Filename);
+    StringRef Filename = key.Filename;
     if (key.Imported)
-      Reader.ResolveImportedPath(M, Filename);
+      Filename = Reader.ResolveImportedPath(Filename, M);
     if (auto FE = FileMgr.getOptionalFileRef(Filename)) {
       // FIXME: NameAsWritten
       Module::Header H = {std::string(key.Filename), "", *FE};
@@ -2520,11 +2519,10 @@ InputFileInfo ASTReader::getInputFileInfo(ModuleFile &F, unsigned ID) {
   std::tie(R.FilenameAsRequested, R.Filename) = [&]() {
     uint16_t AsRequestedLength = Record[7];
 
-    std::string NameAsRequested = Blob.substr(0, AsRequestedLength).str();
-    std::string Name = Blob.substr(AsRequestedLength).str();
-
-    ResolveImportedPath(F, NameAsRequested);
-    ResolveImportedPath(F, Name);
+    std::string NameAsRequested =
+        ResolveImportedPath(Blob.substr(0, AsRequestedLength), F).str();
+    std::string Name =
+        ResolveImportedPath(Blob.substr(AsRequestedLength), F).str();
 
     if (Name.empty())
       Name = NameAsRequested;
@@ -2753,20 +2751,15 @@ InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) {
 /// If we are loading a relocatable PCH or module file, and the filename
 /// is not an absolute path, add the system or module root to the beginning of
 /// the file name.
-void ASTReader::ResolveImportedPath(ModuleFile &M, std::string &Filename) {
-  // Resolve relative to the base directory, if we have one.
-  if (!M.BaseDirectory.empty())
-    return ResolveImportedPath(Filename, M.BaseDirectory);
-}
-
-void ASTReader::ResolveImportedPath(std::string &Filename, StringRef Prefix) {
-  if (Filename.empty() || llvm::sys::path::is_absolute(Filename) ||
-      Filename == "<built-in>" || Filename == "<command line>")
-    return;
+StringRef ASTReader::ResolveImportedPath(SmallVectorImpl<char> &Buffer,
+                                         StringRef Path, StringRef Prefix) {
+  if (Prefix.empty() || Path.empty() || llvm::sys::path::is_absolute(Path) ||
+      Path == "<built-in>" || Path == "<command line>")
+    return Path;
 
-  SmallString<128> Buffer;
-  llvm::sys::path::append(Buffer, Prefix, Filename);
-  Filename.assign(Buffer.begin(), Buffer.end());
+  Buffer.clear();
+  llvm::sys::path::append(Buffer, Prefix, Path);
+  return {Buffer.data(), Buffer.size()};
 }
 
 static bool isDiagnosedResult(ASTReader::ASTReadResult ARR, unsigned Caps) {
@@ -3194,8 +3187,8 @@ ASTReader::ReadControlBlock(ModuleFile &F,
     case ORIGINAL_FILE:
       F.OriginalSourceFileID = FileID::get(Record[0]);
       F.ActualOriginalSourceFileName = std::string(Blob);
-      F.OriginalSourceFileName = F.ActualOriginalSourceFileName;
-      ResolveImportedPath(F, F.OriginalSourceFileName);
+      F.OriginalSourceFileName =
+          ResolveImportedPath(F.ActualOriginalSourceFileName, F).str();
       break;
 
     case ORIGINAL_FILE_ID:
@@ -5484,6 +5477,7 @@ bool ASTReader::readASTFileControlBlock(
   RecordData Record;
   std::string ModuleDir;
   bool DoneWithControlBlock = false;
+  SmallString<256> PathBuf;
   while (!DoneWithControlBlock) {
     Expected<llvm::BitstreamEntry> MaybeEntry = Stream.advance();
     if (!MaybeEntry) {
@@ -5566,8 +5560,8 @@ bool ASTReader::readASTFileControlBlock(
       break;
     case MODULE_MAP_FILE: {
       unsigned Idx = 0;
-      auto Path = ReadString(Record, Idx);
-      ResolveImportedPath(Path, ModuleDir);
+      std::string PathStr = ReadString(Record, Idx);
+      StringRef Path = ResolveImportedPath(PathBuf, PathStr, ModuleDir);
       Listener.ReadModuleMapFile(Path);
       break;
     }
@@ -5615,8 +5609,7 @@ bool ASTReader::readASTFileControlBlock(
           break;
         case INPUT_FILE:
           bool Overridden = static_cast<bool>(Record[3]);
-          std::string Filename = std::string(Blob);
-          ResolveImportedPath(Filename, ModuleDir);
+          StringRef Filename = ResolveImportedPath(PathBuf, Blob, ModuleDir);
           shouldContinue = Listener.visitInputFile(
               Filename, isSystemFile, Overridden, /*IsExplicitModule*/false);
           break;
@@ -5653,8 +5646,9 @@ bool ASTReader::readASTFileControlBlock(
         // Skip Size, ModTime and Signature
         Idx += 1 + 1 + ASTFileSignature::size;
         std::string ModuleName = ReadString(Record, Idx);
-        std::string Filename = ReadString(Record, Idx);
-        ResolveImportedPath(Filename, ModuleDir);
+        std::string FilenameStr = ReadString(Record, Idx);
+        StringRef Filename =
+            ResolveImportedPath(PathBuf, FilenameStr, ModuleDir);
         Listener.visitImport(ModuleName, Filename);
       }
       break;
@@ -5908,8 +5902,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
       // FIXME: This doesn't work for framework modules as `Filename` is the
       //        name as written in the module file and does not include
       //        `Headers/`, so this path will never exist.
-      std::string Filename = std::string(Blob);
-      ResolveImportedPath(F, Filename);
+      StringRef Filename = ResolveImportedPath(Blob, F);
       if (auto Umbrella = PP.getFileManager().getOptionalFileRef(Filename)) {
         if (!CurrentModule->getUmbrellaHeaderAsWritten()) {
           // FIXME: NameAsWritten
@@ -5938,16 +5931,14 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
       break;
 
     case SUBMODULE_TOPHEADER: {
-      std::string HeaderName(Blob);
-      ResolveImportedPath(F, HeaderName);
+      StringRef HeaderName = ResolveImportedPath(Blob, F);
       CurrentModule->addTopHeaderFilename(HeaderName);
       break;
     }
 
     case SUBMODULE_UMBRELLA_DIR: {
       // See comments in SUBMODULE_UMBRELLA_HEADER
-      std::string Dirname = std::string(Blob);
-      ResolveImportedPath(F, Dirname);
+      StringRef Dirname = ResolveImportedPath(Blob, F);
       if (auto Umbrella =
               PP.getFileManager().getOptionalDirectoryRef(Dirname)) {
         if (!CurrentModule->getUmbrellaDirAsWritten()) {
@@ -9605,16 +9596,13 @@ std::string ASTReader::ReadString(const RecordDataImpl &Record, unsigned &Idx) {
 std::string ASTReader::ReadPath(ModuleFile &F, const RecordData &Record,
                                 unsigned &Idx) {
   std::string Filename = ReadString(Record, Idx);
-  ResolveImportedPath(F, Filename);
-  return Filename;
+  return ResolveImportedPath(Filename, F).str();
 }
 
 std::string ASTReader::ReadPath(StringRef BaseDirectory,
                                 const RecordData &Record, unsigned &Idx) {
   std::string Filename = ReadString(Record, Idx);
-  if (!BaseDirectory.empty())
-    ResolveImportedPath(Filename, BaseDirectory);
-  return Filename;
+  return ResolveImportedPath(Filename, BaseDirectory).str();
 }
 
 VersionTuple ASTReader::ReadVersionTuple(const RecordData &Record,

>From 1b70df2215a30d7c8df8fa7b7116160aa7c4ba29 Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Tue, 29 Oct 2024 13:26:34 -0700
Subject: [PATCH 2/2] Manage ownership and assert on overlapping lifetimes

---
 clang/include/clang/Serialization/ASTReader.h | 39 ++++++++++++++++---
 clang/lib/Serialization/ASTReader.cpp         | 34 ++++++++--------
 2 files changed, 52 insertions(+), 21 deletions(-)

diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 3c8bef484d0133..6da1a56d1321d9 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -50,6 +50,7 @@
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Bitstream/BitstreamReader.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/Timer.h"
 #include "llvm/Support/VersionTuple.h"
 #include <cassert>
@@ -1342,19 +1343,47 @@ class ASTReader
                                         bool Complain = true);
 
   /// Buffer we use as temporary storage backing resolved paths.
-  SmallString<256> PathBuf;
+  std::optional<SmallString<256>> PathBuf{{}};
+
+  /// A RAII wrapper around \c StringRef that temporarily takes ownership of the
+  /// underlying buffer and gives it back on destruction.
+  class TemporarilyOwnedStringRef {
+    StringRef String;
+    llvm::SaveAndRestore<std::optional<SmallString<256>>> TemporaryLoan;
+
+  public:
+    TemporarilyOwnedStringRef(StringRef S, std::optional<SmallString<256>> &Buf)
+        : String(S), TemporaryLoan(Buf, {}) {}
+
+    /// Returns the wrapped \c StringRef that must be outlived by \c this.
+    const StringRef *operator->() const { return &String; }
+    /// Returns the wrapped \c StringRef that must be outlived by \c this.
+    const StringRef &operator*() const { return String; }
+  };
 
 public:
-  StringRef ResolveImportedPath(StringRef Path, ModuleFile &M) {
-    return ResolveImportedPath(PathBuf, Path, M);
+  /// Resolve \c Path in the context of module file \c M. The return value must
+  /// be destroyed before another call to \c ResolveImportPath.
+  TemporarilyOwnedStringRef ResolveImportedPath(StringRef Path, ModuleFile &M) {
+    return ResolveImportedPath(Path, M.BaseDirectory);
   }
-  StringRef ResolveImportedPath(StringRef Path, StringRef Prefix) {
-    return ResolveImportedPath(PathBuf, Path, Prefix);
+  /// Resolve \c Path in the context of the \c Prefix directory. The return
+  /// value must be destroyed before another call to \c ResolveImportPath.
+  TemporarilyOwnedStringRef ResolveImportedPath(StringRef Path,
+                                                StringRef Prefix) {
+    assert(PathBuf && "Multiple overlapping calls to ResolveImportedPath");
+    StringRef ResolvedPath = ResolveImportedPath(*PathBuf, Path, Prefix);
+    return {ResolvedPath, PathBuf};
   }
+
+  /// Resolve \c Path in the context of module file \c M. The \c Buffer must
+  /// outlive the returned \c StringRef.
   static StringRef ResolveImportedPath(SmallVectorImpl<char> &Buffer,
                                        StringRef Path, ModuleFile &M) {
     return ResolveImportedPath(Buffer, Path, M.BaseDirectory);
   }
+  /// Resolve \c Path in the context of the \c Prefix directory. The \c Buffer
+  /// must outlive the returned \c StringRef.
   static StringRef ResolveImportedPath(SmallVectorImpl<char> &Buffer,
                                        StringRef Path, StringRef Prefix);
 
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 19f7f2be7f9011..8a0b123da503d8 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -2050,8 +2050,8 @@ const FileEntry *HeaderFileInfoTrait::getFile(const internal_key_type &Key) {
     return nullptr;
   }
 
-  StringRef Resolved = Reader.ResolveImportedPath(Key.Filename, M);
-  if (auto File = FileMgr.getOptionalFileRef(Resolved))
+  auto Resolved = Reader.ResolveImportedPath(Key.Filename, M);
+  if (auto File = FileMgr.getOptionalFileRef(*Resolved))
     return *File;
   return nullptr;
 }
@@ -2149,9 +2149,11 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
     ModuleMap &ModMap =
         Reader.getPreprocessor().getHeaderSearchInfo().getModuleMap();
 
-    StringRef Filename = key.Filename;
-    if (key.Imported)
-      Filename = Reader.ResolveImportedPath(Filename, M);
+    auto ResolvedFilename =
+        key.Imported
+            ? std::make_optional(Reader.ResolveImportedPath(key.Filename, M))
+            : std::nullopt;
+    auto Filename = ResolvedFilename ? **ResolvedFilename : key.Filename;
     if (auto FE = FileMgr.getOptionalFileRef(Filename)) {
       // FIXME: NameAsWritten
       Module::Header H = {std::string(key.Filename), "", *FE};
@@ -2520,9 +2522,9 @@ InputFileInfo ASTReader::getInputFileInfo(ModuleFile &F, unsigned ID) {
     uint16_t AsRequestedLength = Record[7];
 
     std::string NameAsRequested =
-        ResolveImportedPath(Blob.substr(0, AsRequestedLength), F).str();
+        ResolveImportedPath(Blob.substr(0, AsRequestedLength), F)->str();
     std::string Name =
-        ResolveImportedPath(Blob.substr(AsRequestedLength), F).str();
+        ResolveImportedPath(Blob.substr(AsRequestedLength), F)->str();
 
     if (Name.empty())
       Name = NameAsRequested;
@@ -3188,7 +3190,7 @@ ASTReader::ReadControlBlock(ModuleFile &F,
       F.OriginalSourceFileID = FileID::get(Record[0]);
       F.ActualOriginalSourceFileName = std::string(Blob);
       F.OriginalSourceFileName =
-          ResolveImportedPath(F.ActualOriginalSourceFileName, F).str();
+          ResolveImportedPath(F.ActualOriginalSourceFileName, F)->str();
       break;
 
     case ORIGINAL_FILE_ID:
@@ -5902,8 +5904,8 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
       // FIXME: This doesn't work for framework modules as `Filename` is the
       //        name as written in the module file and does not include
       //        `Headers/`, so this path will never exist.
-      StringRef Filename = ResolveImportedPath(Blob, F);
-      if (auto Umbrella = PP.getFileManager().getOptionalFileRef(Filename)) {
+      auto Filename = ResolveImportedPath(Blob, F);
+      if (auto Umbrella = PP.getFileManager().getOptionalFileRef(*Filename)) {
         if (!CurrentModule->getUmbrellaHeaderAsWritten()) {
           // FIXME: NameAsWritten
           ModMap.setUmbrellaHeaderAsWritten(CurrentModule, *Umbrella, Blob, "");
@@ -5931,16 +5933,16 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
       break;
 
     case SUBMODULE_TOPHEADER: {
-      StringRef HeaderName = ResolveImportedPath(Blob, F);
-      CurrentModule->addTopHeaderFilename(HeaderName);
+      auto HeaderName = ResolveImportedPath(Blob, F);
+      CurrentModule->addTopHeaderFilename(*HeaderName);
       break;
     }
 
     case SUBMODULE_UMBRELLA_DIR: {
       // See comments in SUBMODULE_UMBRELLA_HEADER
-      StringRef Dirname = ResolveImportedPath(Blob, F);
+      auto Dirname = ResolveImportedPath(Blob, F);
       if (auto Umbrella =
-              PP.getFileManager().getOptionalDirectoryRef(Dirname)) {
+              PP.getFileManager().getOptionalDirectoryRef(*Dirname)) {
         if (!CurrentModule->getUmbrellaDirAsWritten()) {
           // FIXME: NameAsWritten
           ModMap.setUmbrellaDirAsWritten(CurrentModule, *Umbrella, Blob, "");
@@ -9596,13 +9598,13 @@ std::string ASTReader::ReadString(const RecordDataImpl &Record, unsigned &Idx) {
 std::string ASTReader::ReadPath(ModuleFile &F, const RecordData &Record,
                                 unsigned &Idx) {
   std::string Filename = ReadString(Record, Idx);
-  return ResolveImportedPath(Filename, F).str();
+  return ResolveImportedPath(Filename, F)->str();
 }
 
 std::string ASTReader::ReadPath(StringRef BaseDirectory,
                                 const RecordData &Record, unsigned &Idx) {
   std::string Filename = ReadString(Record, Idx);
-  return ResolveImportedPath(Filename, BaseDirectory).str();
+  return ResolveImportedPath(Filename, BaseDirectory)->str();
 }
 
 VersionTuple ASTReader::ReadVersionTuple(const RecordData &Record,



More information about the cfe-commits mailing list