[clang][deps] Only cache files with specific extension

[jansvoboda11]

Summary

In the scanner's VFS, we cache all files by default and only avoid caching stat failures for certain files. This tanks the performance of scanning with pre-populated module cache. When there is a stale PCM file, it gets cached by the scanner at the start and the rebuilt version never makes it through the VFS again. The TU invocation that rebuilds the PCM only sees the copy in its InMemoryModuleCache, which is invisible to other invocations. This means the PCM gets rebuilt for every TU given to the scanner.

This patch fixes the situation by flipping the default, only caching files that are known to be important, and letting everything else fall through to the underlying VFS.

[ributzka]

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

160–161

s/Whitelist/Allowlist

[Bigcheese]

This doesn't appear to behave as expected for PP_CacheFailure or PP_ScanFile without PP_CacheSuccess. Looks like that will still cache. Should probably just assert that's not the computed policy.

Looks good with those changes.

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

160–161

I think we can just change this comment to something like:

Determine caching and scanning behavior based on file extension.

164–176

If you use LLVM_MARK_AS_BITMASK_ENUM I think you can remove these casts.

[jansvoboda11]

Remove configurable CacheSuccess, rename CacheFailure to CacheStatFailure.

[Bigcheese]

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

272–291

Alternative that gets rid of the asserts as you just can't construct it incorrectly. I didn't compile this locally, but this should make it so you can't use {} to construct one externally, and you either use:

PathPolicy::cache(ScanFile::Yes, CacheStatFailure::No) or
PathPolicy::bypassCache()

when constructing them.

[jansvoboda11]

Use named constructors instead of an argument to PathPolicy, add unit tests.

[jansvoboda11]

Use brace initialization in named constructors

[jansvoboda11]

Disable brace initialization

[jansvoboda11]

Improve member documentation

[Bigcheese]

lgtm, thanks!

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

public:
   StringRef getContents() const { return Entry.getOriginalContents(); }
 
   std::optional<ArrayRef<dependency_directives_scan::Directive>>
   getDirectiveTokens() const {
     return Entry.getDirectiveTokens();
   }
 };
 
+enum class ScanFile { Yes, No };
+enum class CacheStatFailure { Yes, No };
+
+struct PathPolicy {
+  unsigned Enable : 1; // Implies caching of all open and stat results.
+  unsigned ScanFile : 1;
+  unsigned CacheStatFailure : 1; // Explicitly disables stat failure caching.
+
+  static PathPolicy fallThrough() { return {false, false, false}; }
+
+  static PathPolicy cache(enum ScanFile SF,
+                          enum CacheStatFailure CSF = CacheStatFailure::Yes) {
+    return {true, SF == ScanFile::Yes, CSF == CacheStatFailure::Yes};
+  }
+
+private:
+  PathPolicy(bool E, bool SF, bool CSF)
+      : Enable(E), ScanFile(SF), CacheStatFailure(CSF) {}
+};
+
+/// Determine caching and scanning behavior based on file extension.
+PathPolicy getPolicy(StringRef Filename);
+
 /// A virtual file system optimized for the dependency discovery.
 ///
 /// It is primarily designed to work with source files whose contents was
 /// preprocessed to remove any tokens that are unlikely to affect the dependency
 /// computation.
 ///
 /// This is not a thread safe VFS. A single instance is meant to be used only in
 /// one thread. Multiple instances are allowed to service multiple threads
 /// running in parallel.
 class DependencyScanningWorkerFilesystem : public llvm::vfs::ProxyFileSystem {
 public:
   DependencyScanningWorkerFilesystem(
       DependencyScanningFilesystemSharedCache &SharedCache,
       IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
       : ProxyFileSystem(std::move(FS)), SharedCache(SharedCache) {}
 
   llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override;
   llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
   openFileForRead(const Twine &Path) override;
 
   /// Returns entry for the given filename.
   ///
   /// Attempts to use the local and shared caches first, then falls back to
   /// using the underlying filesystem.
-  llvm::ErrorOr<EntryRef>
-  getOrCreateFileSystemEntry(StringRef Filename,
-                             bool DisableDirectivesScanning = false);
+  llvm::ErrorOr<EntryRef> getOrCreateFileSystemEntry(StringRef Filename) {
+    return getOrCreateFileSystemEntry(Filename, getPolicy(Filename));
+  }
 
 private:
-  /// Check whether the file should be scanned for preprocessor directives.
-  bool shouldScanForDirectives(StringRef Filename);
+  /// Same as the public version, but with explicit PathPolicy parameter.
+  llvm::ErrorOr<EntryRef> getOrCreateFileSystemEntry(StringRef Filename,
+                                                     PathPolicy Policy);
 
   /// For a filename that's not yet associated with any entry in the caches,
   /// uses the underlying filesystem to either look up the entry based in the
   /// shared cache indexed by unique ID, or creates new entry from scratch.
   llvm::ErrorOr<const CachedFileSystemEntry &>
-  computeAndStoreResult(StringRef Filename);
+  computeAndStoreResult(StringRef Filename, PathPolicy Policy);
 
   /// Scan for preprocessor directives for the given entry if necessary and
   /// returns a wrapper object with reference semantics.
   EntryRef scanForDirectivesIfNecessary(const CachedFileSystemEntry &Entry,
-                                        StringRef Filename, bool Disable);
+                                        StringRef Filename, PathPolicy Policy);
 
   /// Represents a filesystem entry that has been stat-ed (and potentially read)
   /// and that's about to be inserted into the cache as `CachedFileSystemEntry`.
   struct TentativeEntry {
     llvm::vfs::Status Status;
     std::unique_ptr<llvm::MemoryBuffer> Contents;
 
     TentativeEntry(llvm::vfs::Status Status,

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

DependencyScanningWorkerFilesystem::readFile(StringRef Filename) {
   // If the file size changed between read and stat, pretend it didn't.
   if (Stat.getSize() != Buffer->getBufferSize())
     Stat = llvm::vfs::Status::copyWithNewSize(Stat, Buffer->getBufferSize());
 
   return TentativeEntry(Stat, std::move(Buffer));
 }
 
 EntryRef DependencyScanningWorkerFilesystem::scanForDirectivesIfNecessary(
-    const CachedFileSystemEntry &Entry, StringRef Filename, bool Disable) {
-  if (Entry.isError() || Entry.isDirectory() || Disable ||
-      !shouldScanForDirectives(Filename))
+    const CachedFileSystemEntry &Entry, StringRef Filename, PathPolicy Policy) {
+  if (Entry.isError() || Entry.isDirectory() || !Policy.ScanFile)
     return EntryRef(Filename, Entry);
 
   CachedFileContents *Contents = Entry.getCachedContents();
   assert(Contents && "contents not initialized");
 
   // Double-checked locking.
   if (Contents->DepDirectives.load())
     return EntryRef(Filename, Entry);

 
 const CachedFileSystemEntry &
 DependencyScanningFilesystemSharedCache::CacheShard::
     getOrInsertEntryForFilename(StringRef Filename,
                                 const CachedFileSystemEntry &Entry) {
   std::lock_guard<std::mutex> LockGuard(CacheLock);
   return *EntriesByFilename.insert({Filename, &Entry}).first->getValue();
 }
 
-/// Whitelist file extensions that should be minimized, treating no extension as
-/// a source file that should be minimized.
-///
-/// This is kinda hacky, it would be better if we knew what kind of file Clang
-/// was expecting instead.
-static bool shouldScanForDirectivesBasedOnExtension(StringRef Filename) {
+PathPolicy clang::tooling::dependencies::getPolicy(StringRef Filename) {
   StringRef Ext = llvm::sys::path::extension(Filename);
   if (Ext.empty())
-    return true; // C++ standard library
-  return llvm::StringSwitch<bool>(Ext)
-      .CasesLower(".c", ".cc", ".cpp", ".c++", ".cxx", true)
-      .CasesLower(".h", ".hh", ".hpp", ".h++", ".hxx", true)
-      .CasesLower(".m", ".mm", true)
-      .CasesLower(".i", ".ii", ".mi", ".mmi", true)
-      .CasesLower(".def", ".inc", true)
-      .Default(false);
-}
-
-static bool shouldCacheStatFailures(StringRef Filename) {
-  StringRef Ext = llvm::sys::path::extension(Filename);
-  if (Ext.empty())
-    return false; // This may be the module cache directory.
-  // Only cache stat failures on files that are not expected to change during
-  // the build.
-  StringRef FName = llvm::sys::path::filename(Filename);
-  if (FName == "module.modulemap" || FName == "module.map")
-    return true;
-  return shouldScanForDirectivesBasedOnExtension(Filename);
-}
-
-bool DependencyScanningWorkerFilesystem::shouldScanForDirectives(
-    StringRef Filename) {
-  return shouldScanForDirectivesBasedOnExtension(Filename);
+    return PathPolicy::cache(ScanFile::Yes, CacheStatFailure::No);
+  // clang-format off
+  return llvm::StringSwitch<PathPolicy>(Ext)
+      .CasesLower(".c", ".cc", ".cpp", ".c++", ".cxx", PathPolicy::cache(ScanFile::Yes))
+      .CasesLower(".h", ".hh", ".hpp", ".h++", ".hxx", PathPolicy::cache(ScanFile::Yes))
+      .CasesLower(".m", ".mm",                         PathPolicy::cache(ScanFile::Yes))
+      .CasesLower(".i", ".ii", ".mi", ".mmi",          PathPolicy::cache(ScanFile::Yes))
+      .CasesLower(".def", ".inc",                      PathPolicy::cache(ScanFile::Yes))
+      .CasesLower(".modulemap", ".map",      PathPolicy::cache(ScanFile::No))
+      .CasesLower(".framework", ".apinotes", PathPolicy::cache(ScanFile::No))
+      .CasesLower(".yaml", ".json", ".hmap", PathPolicy::cache(ScanFile::No))
+      .Default(PathPolicy::fallThrough());
+  // clang-format on
 }
 
 const CachedFileSystemEntry &
 DependencyScanningWorkerFilesystem::getOrEmplaceSharedEntryForUID(
     TentativeEntry TEntry) {
   auto &Shard = SharedCache.getShardForUID(TEntry.Status.getUniqueID());
   return Shard.getOrEmplaceEntryForUID(TEntry.Status.getUniqueID(),
                                        std::move(TEntry.Status),
                                        std::move(TEntry.Contents));
 }
 
 const CachedFileSystemEntry *
 DependencyScanningWorkerFilesystem::findEntryByFilenameWithWriteThrough(
     StringRef Filename) {
   if (const auto *Entry = LocalCache.findEntryByFilename(Filename))
     return Entry;
   auto &Shard = SharedCache.getShardForFilename(Filename);
   if (const auto *Entry = Shard.findEntryByFilename(Filename))
     return &LocalCache.insertEntryForFilename(Filename, *Entry);
   return nullptr;
 }
 
 llvm::ErrorOr<const CachedFileSystemEntry &>
-DependencyScanningWorkerFilesystem::computeAndStoreResult(StringRef Filename) {
+DependencyScanningWorkerFilesystem::computeAndStoreResult(StringRef Filename,
+                                                          PathPolicy Policy) {
   llvm::ErrorOr<llvm::vfs::Status> Stat = getUnderlyingFS().status(Filename);
   if (!Stat) {
-    if (!shouldCacheStatFailures(Filename))
+    if (!Policy.CacheStatFailure)
       return Stat.getError();
     const auto &Entry =
         getOrEmplaceSharedEntryForFilename(Filename, Stat.getError());
     return insertLocalEntryForFilename(Filename, Entry);
   }
 
   if (const auto *Entry = findSharedEntryByUID(*Stat))
     return insertLocalEntryForFilename(Filename, *Entry);
const CachedFileSystemEntry *SharedEntry = [&]() {
     return &getOrEmplaceSharedEntryForFilename(Filename, TEntry.getError());
   }();
 
   return insertLocalEntryForFilename(Filename, *SharedEntry);
 }
 
 llvm::ErrorOr<EntryRef>
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
-    StringRef Filename, bool DisableDirectivesScanning) {
+    StringRef Filename, PathPolicy Policy) {
   if (const auto *Entry = findEntryByFilenameWithWriteThrough(Filename))
-    return scanForDirectivesIfNecessary(*Entry, Filename,
-                                        DisableDirectivesScanning)
-        .unwrapError();
-  auto MaybeEntry = computeAndStoreResult(Filename);
+    return scanForDirectivesIfNecessary(*Entry, Filename, Policy).unwrapError();
+  auto MaybeEntry = computeAndStoreResult(Filename, Policy);
   if (!MaybeEntry)
     return MaybeEntry.getError();
-  return scanForDirectivesIfNecessary(*MaybeEntry, Filename,
-                                      DisableDirectivesScanning)
+  return scanForDirectivesIfNecessary(*MaybeEntry, Filename, Policy)
       .unwrapError();
 }
 
 llvm::ErrorOr<llvm::vfs::Status>
 DependencyScanningWorkerFilesystem::status(const Twine &Path) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
+  PathPolicy Policy = getPolicy(Filename);
+  if (!Policy.Enable)
+    return getUnderlyingFS().status(Path);
 
-  llvm::ErrorOr<EntryRef> Result = getOrCreateFileSystemEntry(Filename);
+  llvm::ErrorOr<EntryRef> Result = getOrCreateFileSystemEntry(Filename, Policy);
   if (!Result)
     return Result.getError();
   return Result->getStatus();
 }
 
 namespace {
 
 /// The VFS that is used by clang consumes the \c CachedFileSystemEntry using
DepScanFile::create(EntryRef Entry) {
   return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(
       std::unique_ptr<llvm::vfs::File>(std::move(Result)));
 }
 
 llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
 DependencyScanningWorkerFilesystem::openFileForRead(const Twine &Path) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
+  PathPolicy Policy = getPolicy(Filename);
+  if (!Policy.Enable)
+    return getUnderlyingFS().openFileForRead(Path);
 
-  llvm::ErrorOr<EntryRef> Result = getOrCreateFileSystemEntry(Filename);
+  llvm::ErrorOr<EntryRef> Result = getOrCreateFileSystemEntry(Filename, Policy);
   if (!Result)
     return Result.getError();
   return DepScanFile::create(Result.get());
 }

clang/unittests/Tooling/DependencyScannerTest.cpp

TEST(DependencyScanner, ScanDepsWithFS) {
   std::string DepFile;
   ASSERT_THAT_ERROR(
       ScanTool.getDependencyFile(CommandLine, CWD).moveInto(DepFile),
       llvm::Succeeded());
   using llvm::sys::path::convert_to_slash;
   EXPECT_EQ(convert_to_slash(DepFile),
             "test.cpp.o: /root/test.cpp /root/header.h\n");
 }
+
+// Note: We want to test caching in DependencyScanningWorkerFilesystem. To do
+// that, we need to be able to mutate the underlying file system. However,
+// InMemoryFileSystem does not allow changing the contents of a file after it's
+// been created.
+// To simulate the behavior, we create two separate in-memory file systems, each
+// containing different version of the same file. We pass those to two scanning
+// file systems that share the same cache.
+
+TEST(DependencyScanningFileSystemTest, CacheFileContentsEnabled) {
+  DependencyScanningFilesystemSharedCache SharedCache;
+
+  StringRef Path = "/root/source.c";
+  auto Contents1 = llvm::MemoryBuffer::getMemBuffer("contents1");
+  auto Contents2 = llvm::MemoryBuffer::getMemBuffer("contents2");
+
+  {
+    auto InMemoryFS =
+        llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+    ASSERT_TRUE(InMemoryFS->addFile(Path, 0, std::move(Contents1)));
+    DependencyScanningWorkerFilesystem ScanningFS(SharedCache, InMemoryFS);
+    auto File = ScanningFS.openFileForRead(Path);
+    ASSERT_TRUE(File);
+    auto Buffer = (*File)->getBuffer("Buffer for /root/source.c.");
+    ASSERT_TRUE(Buffer);
+    auto Contents = (*Buffer)->getBuffer();
+    EXPECT_EQ(Contents, "contents1");
+  }
+
+  {
+    auto InMemoryFS =
+        llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+    ASSERT_TRUE(InMemoryFS->addFile(Path, 0, std::move(Contents2)));
+    DependencyScanningWorkerFilesystem ScanningFS(SharedCache, InMemoryFS);
+    auto File = ScanningFS.openFileForRead(Path);
+    ASSERT_TRUE(File);
+    auto Buffer = (*File)->getBuffer("Buffer for /root/source.c.");
+    ASSERT_TRUE(Buffer);
+    auto Contents = (*Buffer)->getBuffer();
+    EXPECT_EQ(Contents, "contents1");
+  }
+}
+
+TEST(DependencyScanningFileSystemTest, CacheFileContentsDisabled) {
+  DependencyScanningFilesystemSharedCache SharedCache;
+
+  StringRef Path = "/root/module.pcm";
+  auto Contents1 = llvm::MemoryBuffer::getMemBuffer("contents1");
+  auto Contents2 = llvm::MemoryBuffer::getMemBuffer("contents2");
+
+  {
+    auto InMemoryFS =
+        llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+    ASSERT_TRUE(InMemoryFS->addFile(Path, 0, std::move(Contents1)));
+    DependencyScanningWorkerFilesystem ScanningFS(SharedCache, InMemoryFS);
+    auto File = ScanningFS.openFileForRead(Path);
+    ASSERT_TRUE(File);
+    auto Buffer = (*File)->getBuffer("Buffer for /root/module.pcm.");
+    ASSERT_TRUE(Buffer);
+    auto Contents = (*Buffer)->getBuffer();
+    EXPECT_EQ(Contents, "contents1");
+  }
+
+  {
+    auto InMemoryFS =
+        llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+    ASSERT_TRUE(InMemoryFS->addFile(Path, 0, std::move(Contents2)));
+    DependencyScanningWorkerFilesystem ScanningFS(SharedCache, InMemoryFS);
+    auto File = ScanningFS.openFileForRead(Path);
+    ASSERT_TRUE(File);
+    auto Buffer = (*File)->getBuffer("Buffer for /root/module.pcm.");
+    ASSERT_TRUE(Buffer);
+    auto Contents = (*Buffer)->getBuffer();
+    EXPECT_EQ(Contents, "contents2");
+  }
+}
+
+TEST(DependencyScanningFileSystemTest, CacheStatFailureEnabled) {
+  DependencyScanningFilesystemSharedCache SharedCache;
+  auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+  DependencyScanningWorkerFilesystem ScanningFS(SharedCache, InMemoryFS);
+
+  StringRef Path = "/root/source.c";
+
+  auto Stat1 = ScanningFS.status(Path);
+  EXPECT_FALSE(Stat1);
+
+  auto Contents = llvm::MemoryBuffer::getMemBuffer("contents");
+  InMemoryFS->addFile(Path, 0, std::move(Contents));
+
+  auto Stat2 = ScanningFS.status(Path);
+  EXPECT_FALSE(Stat2);
+}
+
+TEST(DependencyScanningFileSystemTest, CacheStatFailureDisabledFile) {
+  DependencyScanningFilesystemSharedCache SharedCache;
+  auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+  DependencyScanningWorkerFilesystem ScanningFS(SharedCache, InMemoryFS);
+
+  StringRef Path = "/root/vector";
+
+  auto Stat1 = ScanningFS.status(Path);
+  EXPECT_FALSE(Stat1);
+
+  auto Contents = llvm::MemoryBuffer::getMemBuffer("contents");
+  InMemoryFS->addFile(Path, 0, std::move(Contents));
+
+  auto Stat2 = ScanningFS.status(Path);
+  EXPECT_TRUE(Stat2);
+}
+
+TEST(DependencyScanningFileSystemTest, CacheStatFailureDisabledDirectory) {
+  DependencyScanningFilesystemSharedCache SharedCache;
+  auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+  DependencyScanningWorkerFilesystem ScanningFS(SharedCache, InMemoryFS);
+
+  StringRef Path = "/root/dir";
+
+  auto Stat1 = ScanningFS.status(Path);
+  EXPECT_FALSE(Stat1);
+
+  auto Contents = llvm::MemoryBuffer::getMemBuffer("contents");
+  InMemoryFS->addFile("/root/dir/file", 0, std::move(Contents));
+
+  auto Stat2 = ScanningFS.status(Path);
+  EXPECT_TRUE(Stat2);
+}