[IncludeTree] IncludeTreeFileList optimizations

Optimize IncludeTreeFileList to make creation and iterating to have less
overhead and easier to use:

* Unify the conflict file detection code with the code that uniquing the
  file from nested FileList so there is no need to use separate data
  structures.
* Extend the conflict file content detection to iterating method
  `forEachFile` so it is easier to discover the problem with no
  overhead since no extra data structure is needed.
* Improve the IncludeTreeFileSystem constructor to share more functions
  between two versions.
* Remove an unnecessary requirement that IncludeTree::FileList must
  contain FileEntries. It can have an empty file entries list but
  provides nesting FileList accesses.

Author: cachemeifyoucan

clang/include/clang/CAS/IncludeTree.h

@@ -313,7 +313,7 @@ class IncludeTree::FileList : public IncludeTreeBase<FileList> {
   FileSizeTy getFileSize(size_t I) const;
 
   llvm::Error
-  forEachFileImpl(llvm::DenseSet<ObjectRef> &Seen,
+  forEachFileImpl(llvm::DenseMap<ObjectRef, ObjectRef> &Seen,
                   llvm::function_ref<llvm::Error(File, FileSizeTy)> Callback);
 
   static bool isValid(const ObjectProxy &Node);
@@ -864,16 +864,16 @@ class IncludeTreeRoot : public IncludeTreeBase<IncludeTreeRoot> {
 
 /// An implementation of a \p vfs::FileSystem that supports the simple queries
 /// of the preprocessor, for creating \p FileEntries using a file path, while
-/// "replaying" an \p IncludeTreeRoot. It is not intended to be a complete
+/// "replaying" an \p IncludeTree::FileList. It is not intended to be a complete
 /// implementation of a file system.
 Expected<IntrusiveRefCntPtr<llvm::vfs::FileSystem>>
-createIncludeTreeFileSystem(IncludeTreeRoot &Root);
+createIncludeTreeFileSystem(IncludeTree::FileList &List);
 
 /// Create the same IncludeTreeFileSystem but from
 /// ArrayRef<IncludeTree::FileEntry>.
 Expected<IntrusiveRefCntPtr<llvm::vfs::FileSystem>> createIncludeTreeFileSystem(
     llvm::cas::ObjectStore &CAS,
-    llvm::ArrayRef<IncludeTree::FileList::FileEntry> List);
+    llvm::ArrayRef<IncludeTree::FileList::FileEntry> Files);
 
 } // namespace cas
 } // namespace clang

clang/lib/CAS/IncludeTree.cpp

@@ -246,24 +246,49 @@ IncludeTree::FileList::getFileSize(size_t I) const {
       Data.data() + I * sizeof(FileSizeTy));
 }
 
+static llvm::Error diagnoseFileChange(IncludeTree::File F, ObjectRef Content) {
+  auto FilenameBlob = F.getFilename();
+  if (!FilenameBlob)
+    return FilenameBlob.takeError();
+  cas::ObjectStore &DB = F.getCAS();
+  std::string Filename(FilenameBlob->getData());
+  std::string OldID = DB.getID(Content).toString();
+  std::string NewID = DB.getID(F.getContentsRef()).toString();
+  return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                 "file '%s' changed during build; include-tree "
+                                 "contents changed from %s to %s",
+                                 Filename.c_str(), OldID.c_str(),
+                                 NewID.c_str());
+}
+
 llvm::Error IncludeTree::FileList::forEachFileImpl(
-    llvm::DenseSet<ObjectRef> &Seen,
+    llvm::DenseMap<ObjectRef, ObjectRef> &Seen,
     llvm::function_ref<llvm::Error(File, FileSizeTy)> Callback) {
   size_t Next = 0;
   size_t FileCount = getNumFilesCurrentList();
+
   return forEachReference([&](ObjectRef Ref) -> llvm::Error {
     size_t Index = Next++;
-    if (!Seen.insert(Ref).second)
-      return llvm::Error::success();
-
     if (Index < FileCount) {
       auto Include = File::get(getCAS(), Ref);
       if (!Include)
         return Include.takeError();
+      auto Inserted = Seen.try_emplace(Ref, Include->getContentsRef());
+      if (!Inserted.second) {
+        if (Inserted.first->second != Include->getContentsRef())
+          return diagnoseFileChange(*Include, Inserted.first->second);
+        return llvm::Error::success();
+      }
       return Callback(std::move(*Include), getFileSize(Index));
     }
 
     // Otherwise, it's a chained FileList.
+    // Use a value to itself to indicate if the filelist node has been
+    // visited or not.
+    auto Inserted = Seen.try_emplace(Ref, Ref);
+    if (!Inserted.second)
+      return llvm::Error::success();
+
     auto Proxy = getCAS().getProxy(Ref);
     if (!Proxy)
       return Proxy.takeError();
@@ -274,7 +299,7 @@ llvm::Error IncludeTree::FileList::forEachFileImpl(
 
 llvm::Error IncludeTree::FileList::forEachFile(
     llvm::function_ref<llvm::Error(File, FileSizeTy)> Callback) {
-  llvm::DenseSet<ObjectRef> Seen;
+  llvm::DenseMap<ObjectRef, ObjectRef> Seen;
   return forEachFileImpl(Seen, Callback);
 }
 
@@ -321,7 +346,7 @@ bool IncludeTree::FileList::isValid(const ObjectProxy &Node) {
     return false;
   unsigned NumFiles =
       llvm::support::endian::read<uint32_t, llvm::endianness::little>(Data.data());
-  return NumFiles != 0 && NumFiles <= Base.getNumReferences() &&
+  return NumFiles <= Base.getNumReferences() &&
          Data.size() == sizeof(uint32_t) + NumFiles * sizeof(FileSizeTy);
 }
 
@@ -1015,38 +1040,19 @@ class IncludeTreeFileSystem : public llvm::vfs::FileSystem {
 };
 } // namespace
 
-static llvm::Error diagnoseFileChange(IncludeTree::File F, ObjectRef Content) {
-  auto FilenameBlob = F.getFilename();
-  if (!FilenameBlob)
-    return FilenameBlob.takeError();
-  cas::ObjectStore &DB = F.getCAS();
-  std::string Filename(FilenameBlob->getData());
-  std::string OldID = DB.getID(Content).toString();
-  std::string NewID = DB.getID(F.getContentsRef()).toString();
-  return llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                 "file '%s' changed during build; include-tree "
-                                 "contents changed from %s to %s",
-                                 Filename.c_str(), OldID.c_str(),
-                                 NewID.c_str());
-}
-
-Expected<IntrusiveRefCntPtr<llvm::vfs::FileSystem>>
-cas::createIncludeTreeFileSystem(IncludeTreeRoot &Root) {
-  auto FileList = Root.getFileList();
-  if (!FileList)
-    return FileList.takeError();
-
-  std::vector<IncludeTree::FileList::FileEntry> Files;
-  Files.reserve(FileList->getNumReferences());
-
-  if (auto Err = FileList->forEachFile(
-          [&](IncludeTree::File File, IncludeTree::FileList::FileSizeTy Size) {
-            Files.push_back({File.getRef(), Size});
-            return llvm::Error::success();
-          }))
-    return std::move(Err);
-
-  return createIncludeTreeFileSystem(Root.getCAS(), Files);
+static std::string computeFilename(StringRef Name, IncludeTreeFileSystem &FS) {
+  SmallString<128> Filename(Name);
+  assert(Filename != ".");
+  llvm::sys::path::remove_dots(Filename);
+
+  StringRef DirName = llvm::sys::path::parent_path(Filename);
+  if (DirName.empty())
+    DirName = ".";
+  auto &DirEntry = FS.Directories[DirName];
+  if (DirEntry == llvm::sys::fs::UniqueID()) {
+    DirEntry = llvm::vfs::getNextVirtualUniqueID();
+  }
+  return Filename.str().str();
 }
 
 Expected<IntrusiveRefCntPtr<llvm::vfs::FileSystem>>
@@ -1077,20 +1083,7 @@ cas::createIncludeTreeFileSystem(
     if (!FilenameBlob)
       return FilenameBlob.takeError();
 
-    SmallString<128> Filename(FilenameBlob->getData());
-    // Strip './' in the filename to match the behaviour of ASTWriter; we
-    // also strip './' in IncludeTreeFileSystem::getPath.
-    assert(Filename != ".");
-    llvm::sys::path::remove_dots(Filename);
-
-    StringRef DirName = llvm::sys::path::parent_path(Filename);
-    if (DirName.empty())
-      DirName = ".";
-    auto &DirEntry = IncludeTreeFS->Directories[DirName];
-    if (DirEntry == llvm::sys::fs::UniqueID()) {
-      DirEntry = llvm::vfs::getNextVirtualUniqueID();
-    }
-
+    auto Filename = computeFilename(FilenameBlob->getData(), *IncludeTreeFS);
     IncludeTreeFS->Files.insert(std::make_pair(
         Filename,
         IncludeTreeFileSystem::FileEntry{File->getContentsRef(), Entry.Size,
@@ -1099,3 +1092,30 @@ cas::createIncludeTreeFileSystem(
 
   return IncludeTreeFS;
 }
+
+Expected<IntrusiveRefCntPtr<llvm::vfs::FileSystem>>
+cas::createIncludeTreeFileSystem(IncludeTree::FileList &List) {
+  auto &CAS = List.getCAS();
+  IntrusiveRefCntPtr<IncludeTreeFileSystem> IncludeTreeFS =
+      new IncludeTreeFileSystem(CAS);
+
+  auto E = List.forEachFile(
+      [&](IncludeTree::File File,
+          IncludeTree::FileList::FileSizeTy Size) -> llvm::Error {
+        auto FilenameBlob = File.getFilename();
+        if (!FilenameBlob)
+          return FilenameBlob.takeError();
+        auto Filename =
+            computeFilename(FilenameBlob->getData(), *IncludeTreeFS);
+        IncludeTreeFS->Files.insert(
+            std::make_pair(Filename, IncludeTreeFileSystem::FileEntry{
+                                         File.getContentsRef(), Size,
+                                         llvm::vfs::getNextVirtualUniqueID()}));
+        return llvm::Error::success();
+      });
+
+  if (E)
+    return std::move(E);
+
+  return IncludeTreeFS;
+}

clang/lib/Frontend/CompilerInvocation.cpp

@@ -1611,7 +1611,11 @@ createBaseFS(const FileSystemOptions &FSOpts, const FrontendOptions &FEOpts,
     auto Root = cas::IncludeTreeRoot::get(*CAS, *Ref);
     if (!Root)
       return Root.takeError();
-    return cas::createIncludeTreeFileSystem(*Root);
+    auto FileList = Root->getFileList();
+    if (!FileList)
+      return FileList.takeError();
+
+    return cas::createIncludeTreeFileSystem(*FileList);
   };
   auto makeCASFS = [&](std::shared_ptr<llvm::cas::ObjectStore> CAS,
                        llvm::cas::CASID &ID)