[clang][deps] Use the caching VFS even in the 'preprocess' mode

[jansvoboda11]

The dependency scanner worker's VFS originally unconditionally did two things: file system access caching and dependency directives extraction. That's why clang-scan-deps -mode preprocess avoided using the VFS entirely. Since then, the dependency directives extraction was made lazy/on-demand/optional, meaning it should be possible to use only the caching parts of the VFS. This PR does exactly that, speeding up clang-scan-deps -mode preprocess on my config of Clang/LLVM from ~80s to ~38s. (For comparison, clang-scan-deps -mode preprocess-dependency-directives runs in ~13s.)

(The real motivation was to simplify the VFS handling in the scanner, this is just a nice side-effect.)

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)

Changes

The dependency scanner worker's VFS originally unconditionally did two things: file system access caching and dependency directives extraction. That's why clang-scan-deps -mode preprocess avoided using the VFS entirely. Since then, the dependency directives extraction was made lazy/on-demand/optional, meaning it should be possible to use only the caching parts of the VFS. This PR does exactly that, speeding up clang-scan-deps -mode preprocess on my config of Clang/LLVM from ~80s to ~38s. (For comparison, clang-scan-deps -mode preprocess-dependency-directives runs in ~13s.)

(The real motivation was to simplify the VFS handling in the scanner, this is just a nice side-effect.)

---

Full diff: https://github.com/llvm/llvm-project/pull/168970.diff

3 Files Affected:

• (modified) clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h (+13-18)
• (modified) clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.cpp (+3-3)
• (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp (+21-18)

diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
index e2c353a254bf3..65c943ec06484 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
@@ -86,9 +86,9 @@ class DependencyScanningWorker {
   /// Construct a dependency scanning worker.
   ///
   /// @param Service The parent service. Must outlive the worker.
-  /// @param FS The filesystem for the worker to use.
+  /// @param BaseFS The filesystem for the worker to use.
   DependencyScanningWorker(DependencyScanningService &Service,
-                           llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS);
+                           IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS);
 
   ~DependencyScanningWorker();
 
@@ -157,31 +157,26 @@ class DependencyScanningWorker {
                                        DependencyActionController &Controller);
   bool finalizeCompilerInstance();
 
-  llvm::vfs::FileSystem &getVFS() const { return *BaseFS; }
+  llvm::vfs::FileSystem &getVFS() const { return *DepFS; }
 
 private:
   /// The parent dependency scanning service.
   DependencyScanningService &Service;
   std::shared_ptr<PCHContainerOperations> PCHContainerOps;
-  /// The file system to be used during the scan.
-  /// This is either \c FS passed in the constructor (when performing canonical
-  /// preprocessing), or \c DepFS (when performing dependency directives scan).
-  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS;
-  /// When performing dependency directives scan, this is the caching (and
-  /// dependency-directives-extracting) filesystem overlaid on top of \c FS
-  /// (passed in the constructor).
-  llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS;
+  /// This is the caching (and optionally dependency-directives-providing) VFS
+  /// overlaid on top of the base VFS passed in the constructor.
+  IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS;
 
   friend CompilerInstanceWithContext;
   std::unique_ptr<CompilerInstanceWithContext> CIWithContext;
 
-  /// Private helper functions.
-  bool scanDependencies(StringRef WorkingDirectory,
-                        const std::vector<std::string> &CommandLine,
-                        DependencyConsumer &Consumer,
-                        DependencyActionController &Controller,
-                        DiagnosticConsumer &DC,
-                        llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS);
+  /// Actually carries out the scan. If \c OverlayFS is provided, it must be
+  /// based on top of DepFS.
+  bool scanDependencies(
+      StringRef WorkingDirectory, const std::vector<std::string> &CommandLine,
+      DependencyConsumer &Consumer, DependencyActionController &Controller,
+      DiagnosticConsumer &DC,
+      IntrusiveRefCntPtr<llvm::vfs::FileSystem> OverlayFS = nullptr);
 };
 
 } // end namespace dependencies
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.cpp
index 657547d299abd..bdb457401bc73 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.cpp
@@ -524,8 +524,8 @@ bool dependencies::initializeScanCompilerInstance(
   // Create a new FileManager to match the invocation's FileSystemOptions.
   ScanInstance.createFileManager();
 
-  // Use the dependency scanning optimized file system if requested to do so.
-  if (DepFS) {
+  // Use DepFS for getting the dependency directives if requested to do so.
+  if (Service.getMode() == ScanningMode::DependencyDirectivesScan) {
     DepFS->resetBypassedPathPrefix();
     SmallString<256> ModulesCachePath;
     normalizeModuleCachePath(ScanInstance.getFileManager(),
@@ -717,7 +717,7 @@ bool CompilerInstanceWithContext::initialize(DiagnosticConsumer *DC) {
   }
 
   std::tie(OverlayFS, CommandLine) = initVFSForByNameScanning(
-      Worker.BaseFS, CommandLine, CWD, "ScanningByName");
+      Worker.DepFS, CommandLine, CWD, "ScanningByName");
 
   DiagEngineWithCmdAndOpts = std::make_unique<DignosticsEngineWithDiagOpts>(
       CommandLine, OverlayFS, *DiagConsumer);
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 0bc17f9c80605..421a94307a9e5 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -18,7 +18,7 @@ using namespace dependencies;
 
 DependencyScanningWorker::DependencyScanningWorker(
     DependencyScanningService &Service,
-    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
+    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS)
     : Service(Service) {
   PCHContainerOps = std::make_shared<PCHContainerOperations>();
   // We need to read object files from PCH built outside the scanner.
@@ -28,19 +28,11 @@ DependencyScanningWorker::DependencyScanningWorker(
   PCHContainerOps->registerWriter(std::make_unique<RawPCHContainerWriter>());
 
   if (Service.shouldTraceVFS())
-    FS = llvm::makeIntrusiveRefCnt<llvm::vfs::TracingFileSystem>(std::move(FS));
-
-  switch (Service.getMode()) {
-  case ScanningMode::DependencyDirectivesScan:
-    DepFS = llvm::makeIntrusiveRefCnt<DependencyScanningWorkerFilesystem>(
-        Service.getSharedCache(), FS);
-    BaseFS = DepFS;
-    break;
-  case ScanningMode::CanonicalPreprocessing:
-    DepFS = nullptr;
-    BaseFS = FS;
-    break;
-  }
+    BaseFS = llvm::makeIntrusiveRefCnt<llvm::vfs::TracingFileSystem>(
+        std::move(BaseFS));
+
+  DepFS = llvm::makeIntrusiveRefCnt<DependencyScanningWorkerFilesystem>(
+      Service.getSharedCache(), std::move(BaseFS));
 }
 
 DependencyScanningWorker::~DependencyScanningWorker() = default;
@@ -102,7 +94,18 @@ bool DependencyScanningWorker::scanDependencies(
     StringRef WorkingDirectory, const std::vector<std::string> &CommandLine,
     DependencyConsumer &Consumer, DependencyActionController &Controller,
     DiagnosticConsumer &DC,
-    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS) {
+    IntrusiveRefCntPtr<llvm::vfs::FileSystem> OverlayFS) {
+  IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = DepFS;
+  if (OverlayFS) {
+#ifndef NDEBUG
+    bool SawDepFS = false;
+    OverlayFS->visit(
+        [&](llvm::vfs::FileSystem &VFS) { SawDepFS |= &VFS == DepFS.get(); });
+    assert(SawDepFS && "OverlayFS not based on DepFS");
+#endif
+    FS = std::move(OverlayFS);
+  }
+
   DignosticsEngineWithDiagOpts DiagEngineWithCmdAndOpts(CommandLine, FS, DC);
   DependencyScanningAction Action(Service, WorkingDirectory, Consumer,
                                   Controller, DepFS);
@@ -158,13 +161,13 @@ bool DependencyScanningWorker::computeDependencies(
     DiagnosticConsumer &DC, std::optional<llvm::MemoryBufferRef> TUBuffer) {
   if (TUBuffer) {
     auto [FinalFS, FinalCommandLine] = initVFSForTUBufferScanning(
-        BaseFS, CommandLine, WorkingDirectory, *TUBuffer);
+        DepFS, CommandLine, WorkingDirectory, *TUBuffer);
     return scanDependencies(WorkingDirectory, FinalCommandLine, Consumer,
                             Controller, DC, FinalFS);
   } else {
-    BaseFS->setCurrentWorkingDirectory(WorkingDirectory);
+    DepFS->setCurrentWorkingDirectory(WorkingDirectory);
     return scanDependencies(WorkingDirectory, CommandLine, Consumer, Controller,
-                            DC, BaseFS);
+                            DC);
   }
 }
 

[github-actions]

🐧 Linux x64 Test Results

• 111346 tests passed
• 4435 tests skipped

[qiongsiwu]

This change by itself LGTM! Thanks!

I have one question about how we setup the DependencyScanningAction.