[clang][modules][deps] Create more efficient API for visitation of `ModuleFile` inputs

[jansvoboda11]

Summary

The current ASTReader::visitInputFiles() function calls into FileManager to create FileEntryRef objects. This ends up being fairly costly in clang-scan-deps, where we mostly only care about file paths.

This patch introduces new ASTReader API that gives clients access to just the serialized paths. Since the scanner needs both the as-requested path and the on-disk one (and doesn't want to transform the former into the latter via FileManager), this patch starts serializing both of them into the PCM file if they differ.

This increases the size of scanning PCMs by 0.1% and speeds up scanning by 5%.

[jansvoboda11]

Remove leftover std::string constructor

[benlangmuir]

CC @vsapsai since he was also making name vs. name-as-requested change in modules

clang/include/clang/Serialization/ModuleFile.h

72

Is there something using this new split? It seems like every user is using && for these

clang/lib/Serialization/ASTReader.cpp

2411

It's not clear to me why this one changed

9323

Can we rewrite visitInputFiles on top of this?

[jansvoboda11]

clang/include/clang/Serialization/ModuleFile.h

72

You're right. I recall needing this on (now probably abandoned) patch, so I thought I might as well add the extra bit now, sine I'm changing the format anyways. I'm fine with removing this.

clang/lib/Serialization/ASTReader.cpp

2411

This actually maintains the same semantics - FI.Filename was previously the as-requested path, now it's the on-disk path. Without changing this line, FileManager::getFileRef() would get called with the on-disk path, meaning consumers of this function would get the incorrect path when calling FileEntryRef::getNameAsRequested() on the returned file. I recall one clang-scan-deps test failing because of it.

9323

I can try consolidating these in a follow-up if you're fine with that.

[benlangmuir]

clang/include/clang/Serialization/ModuleFile.h

72

It's fine, I was just making sure I didn't miss something.

[jansvoboda11]

clang/lib/Serialization/ASTReader.cpp

2411

Just remembered: in ModuleDepCollector.cpp, we call ModuleMap::getModuleMapFileForUniquing(). This calls SourceMgr.getFileEntryRefForID() based on Module::DefinitionLoc, which triggers deserialization of the associated source location entry and ends up calling this function right here. We'd end up with the on-disk module map path for modular dependencies.

[vsapsai]

The existing FilenameAsRequested usage looks good to me. Unfortunately, don't know how to make sure all places where FilenameAsRequested is required instead of Filename are updated.

[jansvoboda11]

Landing with one more use of Filename converted to FilenameAsRequested (in call to Listener.visitInputFile()). The only remaining usages of Filename is now in the scanner (intentional) and in ASTReader when deciding whether an InputFileInfo has already been deserialized or not (default-initialized InputFileInfo has an empty Filename).

clang/include/clang/Serialization/ASTBitCodes.h

 /// Whenever the AST file format changes in a way that makes it
 /// incompatible with previous versions (such that a reader
 /// designed for the previous version could not support reading
 /// the new version), this number should be increased.
 ///
 /// Version 4 of AST files also requires that the version control branch and
 /// revision match exactly, since there is no backward compatibility of
 /// AST files at this time.
-const unsigned VERSION_MAJOR = 27;
+const unsigned VERSION_MAJOR = 28;
 
 /// AST file minor version number supported by this version of
 /// Clang.
 ///
 /// Whenever the AST format changes in a way that is still
 /// compatible with previous versions (such that a reader designed
 /// for the previous version could still support reading the new
 /// version by ignoring new kinds of subblocks), this number

clang/include/clang/Serialization/ASTReader.h

public:
 
   /// Cursors for comments blocks.
   SmallVector<std::pair<llvm::BitstreamCursor,
                         serialization::ModuleFile *>, 8> CommentsCursors;
 
   /// Loads comments ranges.
   void ReadComments() override;
 
+  /// Visit all the input file infos of the given module file.
+  void visitInputFileInfos(
+      serialization::ModuleFile &MF, bool IncludeSystem,
+      llvm::function_ref<void(const serialization::InputFileInfo &IFI,
+                              bool IsSystem)>
+          Visitor);
+
   /// Visit all the input files of the given module file.
   void visitInputFiles(serialization::ModuleFile &MF,
                        bool IncludeSystem, bool Complain,
           llvm::function_ref<void(const serialization::InputFile &IF,
                                   bool isSystem)> Visitor);
 
   /// Visit all the top-level module maps loaded when building the given module
   /// file.

clang/include/clang/Serialization/ModuleFile.h

enum ModuleKind {
   MK_MainFile,
 
   /// File is from a prebuilt module path.
   MK_PrebuiltModule
 };
 
 /// The input file info that has been loaded from an AST file.
 struct InputFileInfo {
+  std::string FilenameAsRequested;
   std::string Filename;
   uint64_t ContentHash;
   off_t StoredSize;
   time_t StoredTime;
   bool Overridden;
   bool Transient;
-  bool TopLevelModuleMap;
+  bool TopLevel;
+  bool ModuleMap;
 };
 
 /// The input file that has been loaded from this AST file, along with
 /// bools indicating whether this was an overridden buffer or if it was
 /// out-of-date or not-found.
 class InputFile {
   enum {
     Overridden = 1,

clang/lib/Serialization/ASTReader.cpp

InputFileInfo ASTReader::getInputFileInfo(ModuleFile &F, unsigned ID) {
   }
 
   assert(Record[0] == ID && "Bogus stored ID or offset");
   InputFileInfo R;
   R.StoredSize = static_cast<off_t>(Record[1]);
   R.StoredTime = static_cast<time_t>(Record[2]);
   R.Overridden = static_cast<bool>(Record[3]);
   R.Transient = static_cast<bool>(Record[4]);
-  R.TopLevelModuleMap = static_cast<bool>(Record[5]);
-  R.Filename = std::string(Blob);
-  ResolveImportedPath(F, R.Filename);
+  R.TopLevel = static_cast<bool>(Record[5]);
+  R.ModuleMap = static_cast<bool>(Record[6]);
+  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);
+
+    if (Name.empty())
+      Name = NameAsRequested;
+
+    return std::make_pair(std::move(NameAsRequested), std::move(Name));
+  }();
 
   Expected<llvm::BitstreamEntry> MaybeEntry = Cursor.advance();
   if (!MaybeEntry) // FIXME this drops errors on the floor.
     consumeError(MaybeEntry.takeError());
   llvm::BitstreamEntry Entry = MaybeEntry.get();
   assert(Entry.Kind == llvm::BitstreamEntry::Record &&
          "expected record type for input file hash");
 
if (llvm::Error Err = Cursor.JumpToBit(F.InputFileOffsets[ID - 1])) {
     consumeError(std::move(Err));
   }
 
   InputFileInfo FI = getInputFileInfo(F, ID);
   off_t StoredSize = FI.StoredSize;
   time_t StoredTime = FI.StoredTime;
   bool Overridden = FI.Overridden;
   bool Transient = FI.Transient;
-  StringRef Filename = FI.Filename;
+  StringRef Filename = FI.FilenameAsRequested;
   uint64_t StoredContentHash = FI.ContentHash;
 
   // For standard C++ modules, we don't need to check the inputs.
   bool SkipChecks = F.StandardCXXModule;
 
   OptionalFileEntryRefDegradesToFileEntryPtr File = OptionalFileEntryRef(
       expectedToOptional(FileMgr.getFileRef(Filename, /*OpenFile=*/false)));
 
case llvm::BitstreamEntry::EndBlock: {
         Listener->visitModuleFile(F.FileName, F.Kind);
 
       if (Listener && Listener->needsInputFileVisitation()) {
         unsigned N = Listener->needsSystemInputFileVisitation() ? NumInputs
                                                                 : NumUserInputs;
         for (unsigned I = 0; I < N; ++I) {
           bool IsSystem = I >= NumUserInputs;
           InputFileInfo FI = getInputFileInfo(F, I + 1);
-          Listener->visitInputFile(FI.Filename, IsSystem, FI.Overridden,
-                                   F.Kind == MK_ExplicitModule ||
-                                   F.Kind == MK_PrebuiltModule);
+          Listener->visitInputFile(
+              FI.FilenameAsRequested, IsSystem, FI.Overridden,
+              F.Kind == MK_ExplicitModule || F.Kind == MK_PrebuiltModule);
         }
       }
 
       return Success;
     }
 
     case llvm::BitstreamEntry::SubBlock:
       switch (Entry.ID) {
for (RawComment *C : Comments) {
             SourceMgr.getDecomposedLoc(CommentLoc);
         if (Loc.first.isValid())
           Context.Comments.OrderedComments[Loc.first].emplace(Loc.second, C);
       }
     }
   }
 }
 
+void ASTReader::visitInputFileInfos(
+    serialization::ModuleFile &MF, bool IncludeSystem,
+    llvm::function_ref<void(const serialization::InputFileInfo &IFI,
+                            bool IsSystem)>
+        Visitor) {
+  unsigned NumUserInputs = MF.NumUserInputFiles;
+  unsigned NumInputs = MF.InputFilesLoaded.size();
+  assert(NumUserInputs <= NumInputs);
+  unsigned N = IncludeSystem ? NumInputs : NumUserInputs;
+  for (unsigned I = 0; I < N; ++I) {
+    bool IsSystem = I >= NumUserInputs;
+    InputFileInfo IFI = getInputFileInfo(MF, I+1);
+    Visitor(IFI, IsSystem);
+  }
+}
+
 void ASTReader::visitInputFiles(serialization::ModuleFile &MF,
                                 bool IncludeSystem, bool Complain,
                     llvm::function_ref<void(const serialization::InputFile &IF,
                                             bool isSystem)> Visitor) {
   unsigned NumUserInputs = MF.NumUserInputFiles;
   unsigned NumInputs = MF.InputFilesLoaded.size();
   assert(NumUserInputs <= NumInputs);
   unsigned N = IncludeSystem ? NumInputs : NumUserInputs;
   for (unsigned I = 0; I < N; ++I) {
     bool IsSystem = I >= NumUserInputs;
     InputFile IF = getInputFile(MF, I+1, Complain);
     Visitor(IF, IsSystem);
   }
 }
 
 void ASTReader::visitTopLevelModuleMaps(
     serialization::ModuleFile &MF,
     llvm::function_ref<void(FileEntryRef FE)> Visitor) {
   unsigned NumInputs = MF.InputFilesLoaded.size();
   for (unsigned I = 0; I < NumInputs; ++I) {
     InputFileInfo IFI = getInputFileInfo(MF, I + 1);
-    if (IFI.TopLevelModuleMap)
+    if (IFI.TopLevel && IFI.ModuleMap)
       if (auto FE = getInputFile(MF, I + 1).getFile())
         Visitor(*FE);
   }
 }
 
 void ASTReader::finishPendingActions() {
   while (
       !PendingIdentifierInfos.empty() || !PendingDeducedFunctionTypes.empty() ||

clang/lib/Serialization/ASTWriter.cpp

 namespace  {
 
 /// An input file.
 struct InputFileEntry {
   FileEntryRef File;
   bool IsSystemFile;
   bool IsTransient;
   bool BufferOverridden;
-  bool IsTopLevelModuleMap;
+  bool IsTopLevel;
+  bool IsModuleMap;
   uint32_t ContentHash[2];
 
   InputFileEntry(FileEntryRef File) : File(File) {}
 };
 
 } // namespace
 
 void ASTWriter::WriteInputFiles(SourceManager &SourceMgr,
                                 HeaderSearchOptions &HSOpts) {
   using namespace llvm;
 
   Stream.EnterSubblock(INPUT_FILES_BLOCK_ID, 4);
 
   // Create input-file abbreviation.
   auto IFAbbrev = std::make_shared<BitCodeAbbrev>();
   IFAbbrev->Add(BitCodeAbbrevOp(INPUT_FILE));
   IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // ID
   IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 12)); // Size
   IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 32)); // Modification time
   IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Overridden
   IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Transient
+  IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Top-level
   IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Module map
-  IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // File name
+  IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 16)); // Name as req. len
+  IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name as req. + name
   unsigned IFAbbrevCode = Stream.EmitAbbrev(std::move(IFAbbrev));
 
   // Create input file hash abbreviation.
   auto IFHAbbrev = std::make_shared<BitCodeAbbrev>();
   IFHAbbrev->Add(BitCodeAbbrevOp(INPUT_FILE_HASH));
   IFHAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
   IFHAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
   unsigned IFHAbbrevCode = Stream.EmitAbbrev(std::move(IFHAbbrev));
for (unsigned I = 1, N = SourceMgr.local_sloc_entry_size(); I != N; ++I) {
     // Do not emit input files that do not affect current module.
     if (!IsSLocAffecting[I])
       continue;
 
     InputFileEntry Entry(*Cache->OrigEntry);
     Entry.IsSystemFile = isSystem(File.getFileCharacteristic());
     Entry.IsTransient = Cache->IsTransient;
     Entry.BufferOverridden = Cache->BufferOverridden;
-    Entry.IsTopLevelModuleMap = isModuleMap(File.getFileCharacteristic()) &&
-                                File.getIncludeLoc().isInvalid();
+    Entry.IsTopLevel = File.getIncludeLoc().isInvalid();
+    Entry.IsModuleMap = isModuleMap(File.getFileCharacteristic());
 
     auto ContentHash = hash_code(-1);
     if (PP->getHeaderSearchInfo()
             .getHeaderSearchOpts()
             .ValidateASTInputFilesContent) {
       auto MemBuff = Cache->getBufferIfLoaded();
       if (MemBuff)
         ContentHash = hash_value(MemBuff->getBuffer());
for (const auto &Entry : SortedFiles) {
     InputFileID = InputFileOffsets.size();
 
     if (!Entry.IsSystemFile)
       ++UserFilesNum;
 
     // Emit size/modification time for this file.
     // And whether this file was overridden.
     {
+      SmallString<128> NameAsRequested = Entry.File.getNameAsRequested();
+      SmallString<128> Name = Entry.File.getName();
+
+      PreparePathForOutput(NameAsRequested);
+      PreparePathForOutput(Name);
+
+      if (Name == NameAsRequested)
+        Name.clear();
+
       RecordData::value_type Record[] = {
           INPUT_FILE,
           InputFileOffsets.size(),
           (uint64_t)Entry.File.getSize(),
           (uint64_t)getTimestampForOutput(Entry.File),
           Entry.BufferOverridden,
           Entry.IsTransient,
-          Entry.IsTopLevelModuleMap};
+          Entry.IsTopLevel,
+          Entry.IsModuleMap,
+          NameAsRequested.size()};
 
-      EmitRecordWithPath(IFAbbrevCode, Record, Entry.File.getNameAsRequested());
+      Stream.EmitRecordWithBlob(IFAbbrevCode, Record,
+                                (NameAsRequested + Name).str());
     }
 
     // Emit content hash for this file.
     {
       RecordData::value_type Record[] = {INPUT_FILE_HASH, Entry.ContentHash[0],
                                          Entry.ContentHash[1]};
       Stream.EmitRecordWithAbbrev(IFHAbbrevCode, Record);
     }

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

if (ModuleMap) {
     SmallString<128> Path = ModuleMap->getNameAsRequested();
     ModMapInfo.canonicalizeModuleMapPath(Path);
     MD.ClangModuleMapFile = std::string(Path);
   }
 
   serialization::ModuleFile *MF =
       MDC.ScanInstance.getASTReader()->getModuleManager().lookup(
           M->getASTFile());
-  MDC.ScanInstance.getASTReader()->visitInputFiles(
-      *MF, true, true, [&](const serialization::InputFile &IF, bool isSystem) {
+  MDC.ScanInstance.getASTReader()->visitInputFileInfos(
+      *MF, /*IncludeSystem=*/true,
+      [&](const serialization::InputFileInfo &IFI, bool IsSystem) {
         // __inferred_module.map is the result of the way in which an implicit
         // module build handles inferred modules. It adds an overlay VFS with
         // this file in the proper directory and relies on the rest of Clang to
         // handle it like normal. With explicitly built modules we don't need
         // to play VFS tricks, so replace it with the correct module map.
-        if (IF.getFile()->getName().endswith("__inferred_module.map")) {
+        if (StringRef(IFI.Filename).endswith("__inferred_module.map")) {
           MDC.addFileDep(MD, ModuleMap->getName());
           return;
         }
-        MDC.addFileDep(MD, IF.getFile()->getName());
+        MDC.addFileDep(MD, IFI.Filename);
       });
 
   llvm::DenseSet<const Module *> SeenDeps;
   addAllSubmodulePrebuiltDeps(M, MD, SeenDeps);
   addAllSubmoduleDeps(M, MD, SeenDeps);
   addAllAffectingClangModules(M, MD, SeenDeps);
 
-  MDC.ScanInstance.getASTReader()->visitTopLevelModuleMaps(
-      *MF, [&](FileEntryRef FE) {
-        if (FE.getNameAsRequested().endswith("__inferred_module.map"))
+  MDC.ScanInstance.getASTReader()->visitInputFileInfos(
+      *MF, /*IncludeSystem=*/true,
+      [&](const serialization::InputFileInfo &IFI, bool IsSystem) {
+        if (!(IFI.TopLevel && IFI.ModuleMap))
           return;
-        MD.ModuleMapFileDeps.emplace_back(FE.getNameAsRequested());
+        if (StringRef(IFI.FilenameAsRequested)
+                .endswith("__inferred_module.map"))
+          return;
+        MD.ModuleMapFileDeps.emplace_back(IFI.FilenameAsRequested);
       });
 
   CompilerInvocation CI = MDC.makeInvocationForModuleBuildWithoutOutputs(
       MD, [&](CompilerInvocation &BuildInvocation) {
         if (MDC.OptimizeArgs)
           optimizeHeaderSearchOpts(BuildInvocation.getHeaderSearchOpts(),
                                    *MDC.ScanInstance.getASTReader(), *MF);
       });