[clang] [clang-scan-deps] Fix contention when updating `TrackingStatistic`s in hot code paths in `FileManager`. (PR #88427)

via cfe-commits cfe-commits at lists.llvm.org
Thu Apr 11 12:31:32 PDT 2024


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Alexandre Ganea (aganea)

<details>
<summary>Changes</summary>

`FileManager::getDirectoryRef` and `FileManager::getFileRef` are hot code paths in `clang-scan-deps`. In these functions, we update a couple of atomic variables related to printing statistics, which causes contention on high core count machines.

![Screenshot 2024-04-10 214123](https://github.com/llvm/llvm-project/assets/37383324/5756b1bc-cab5-4612-8769-ee7e03a66479)

![Screenshot 2024-04-10 214246](https://github.com/llvm/llvm-project/assets/37383324/3d560e89-61c7-4fb9-9330-f9e660e8fc8b)

![Screenshot 2024-04-10 214315](https://github.com/llvm/llvm-project/assets/37383324/006341fc-49d4-4720-a348-7af435c21b17)


After this PR, we update these variables iff the user wants to print statistics.

On my use case, this saves about 49 sec over 1 min 47 sec of `clang-scan-deps` run time (1 min 47 sec before, 58 sec after). These figures are after applying my suggestion in https://github.com/llvm/llvm-project/pull/88152#issuecomment-2049803229, that is:
```
static bool shouldCacheStatFailures(StringRef Filename) {
  return true;
}
```
Without that, there's just too much OS noise from the high volume of `status()` calls with regular non-module C++ code. Tested on Windows with clang-cl.

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


8 Files Affected:

- (modified) clang/include/clang/Basic/FileManager.h (+5-1) 
- (modified) clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h (+5-1) 
- (modified) clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h (+2) 
- (modified) clang/lib/Basic/FileManager.cpp (+15-8) 
- (modified) clang/lib/Frontend/CompilerInstance.cpp (+2-1) 
- (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp (+2-2) 
- (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp (+4-2) 
- (modified) clang/tools/clang-scan-deps/ClangScanDeps.cpp (+1-1) 


``````````diff
diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index 2245fd78bfc9f0..24256a7368ccc8 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -114,6 +114,9 @@ class FileManager : public RefCountedBase<FileManager> {
   ///
   unsigned NextFileUID;
 
+  /// Whether we want to print statistics. This impacts the collection of data.
+  bool EnablePrintStats;
+
   // Caching.
   std::unique_ptr<FileSystemStatCache> StatCache;
 
@@ -134,7 +137,8 @@ class FileManager : public RefCountedBase<FileManager> {
   /// \param FS if non-null, the VFS to use.  Otherwise uses
   /// llvm::vfs::getRealFileSystem().
   FileManager(const FileSystemOptions &FileSystemOpts,
-              IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr);
+              IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr,
+              bool PrintStats = false);
   ~FileManager();
 
   /// Installs the provided FileSystemStatCache object within
diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
index 557f0e547ab4a8..7b869bb7976f2a 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
@@ -76,7 +76,7 @@ class DependencyScanningService {
   DependencyScanningService(
       ScanningMode Mode, ScanningOutputFormat Format,
       ScanningOptimizations OptimizeArgs = ScanningOptimizations::Default,
-      bool EagerLoadModules = false);
+      bool EagerLoadModules = false, bool PrintStats = false);
 
   ScanningMode getMode() const { return Mode; }
 
@@ -90,6 +90,8 @@ class DependencyScanningService {
     return SharedCache;
   }
 
+  bool getPrintStats() const { return PrintStats; }
+
 private:
   const ScanningMode Mode;
   const ScanningOutputFormat Format;
@@ -97,6 +99,8 @@ class DependencyScanningService {
   const ScanningOptimizations OptimizeArgs;
   /// Whether to set up command-lines to load PCM files eagerly.
   const bool EagerLoadModules;
+  /// Whether we should collect statistics during execution.
+  const bool PrintStats;
   /// The global file system cache.
   DependencyScanningFilesystemSharedCache SharedCache;
 };
diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
index 0f607862194b31..27b96c964ce83d 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
@@ -119,6 +119,8 @@ class DependencyScanningWorker {
   ScanningOptimizations OptimizeArgs;
   /// Whether to set up command-lines to load PCM files eagerly.
   bool EagerLoadModules;
+  /// Whether we should collect statistics during execution.
+  bool PrintStats;
 };
 
 } // end namespace dependencies
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index cd520a6375e07e..1071f6ae53dd78 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -50,9 +50,10 @@ ALWAYS_ENABLED_STATISTIC(NumFileCacheMisses, "Number of file cache misses.");
 //===----------------------------------------------------------------------===//
 
 FileManager::FileManager(const FileSystemOptions &FSO,
-                         IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
+                         IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
+                         bool PrintStats)
     : FS(std::move(FS)), FileSystemOpts(FSO), SeenDirEntries(64),
-      SeenFileEntries(64), NextFileUID(0) {
+      SeenFileEntries(64), NextFileUID(0), EnablePrintStats(PrintStats) {
   // If the caller doesn't provide a virtual file system, just grab the real
   // file system.
   if (!this->FS)
@@ -134,7 +135,8 @@ FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
     }
   }
 
-  ++NumDirLookups;
+  if (EnablePrintStats)
+    ++NumDirLookups;
 
   // See if there was already an entry in the map.  Note that the map
   // contains both virtual and real directories.
@@ -147,7 +149,8 @@ FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
   }
 
   // We've not seen this before. Fill it in.
-  ++NumDirCacheMisses;
+  if (EnablePrintStats)
+    ++NumDirCacheMisses;
   auto &NamedDirEnt = *SeenDirInsertResult.first;
   assert(!NamedDirEnt.second && "should be newly-created");
 
@@ -202,7 +205,8 @@ FileManager::getFile(StringRef Filename, bool openFile, bool CacheFailure) {
 
 llvm::Expected<FileEntryRef>
 FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
-  ++NumFileLookups;
+  if (EnablePrintStats)
+    ++NumFileLookups;
 
   // See if there is already an entry in the map.
   auto SeenFileInsertResult =
@@ -215,7 +219,8 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
   }
 
   // We've not seen this before. Fill it in.
-  ++NumFileCacheMisses;
+  if (EnablePrintStats)
+    ++NumFileCacheMisses;
   auto *NamedFileEnt = &*SeenFileInsertResult.first;
   assert(!NamedFileEnt->second && "should be newly-created");
 
@@ -377,7 +382,8 @@ const FileEntry *FileManager::getVirtualFile(StringRef Filename, off_t Size,
 
 FileEntryRef FileManager::getVirtualFileRef(StringRef Filename, off_t Size,
                                             time_t ModificationTime) {
-  ++NumFileLookups;
+  if (EnablePrintStats)
+    ++NumFileLookups;
 
   // See if there is already an entry in the map for an existing file.
   auto &NamedFileEnt = *SeenFileEntries.insert(
@@ -390,7 +396,8 @@ FileEntryRef FileManager::getVirtualFileRef(StringRef Filename, off_t Size,
   }
 
   // We've not seen this before, or the file is cached as non-existent.
-  ++NumFileCacheMisses;
+  if (EnablePrintStats)
+    ++NumFileCacheMisses;
   addAncestorsAsVirtualDirs(Filename);
   FileEntry *UFE = nullptr;
 
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 6e3baf83864415..d29bc92a953d66 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -381,7 +381,8 @@ FileManager *CompilerInstance::createFileManager(
                   : createVFSFromCompilerInvocation(getInvocation(),
                                                     getDiagnostics());
   assert(VFS && "FileManager has no VFS?");
-  FileMgr = new FileManager(getFileSystemOpts(), std::move(VFS));
+  FileMgr = new FileManager(getFileSystemOpts(), std::move(VFS),
+                            getFrontendOpts().ShowStats);
   return FileMgr.get();
 }
 
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
index 7458ef484b16c4..584066df85aa66 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
@@ -15,9 +15,9 @@ using namespace dependencies;
 
 DependencyScanningService::DependencyScanningService(
     ScanningMode Mode, ScanningOutputFormat Format,
-    ScanningOptimizations OptimizeArgs, bool EagerLoadModules)
+    ScanningOptimizations OptimizeArgs, bool EagerLoadModules, bool PrintStats)
     : Mode(Mode), Format(Format), OptimizeArgs(OptimizeArgs),
-      EagerLoadModules(EagerLoadModules) {
+      EagerLoadModules(EagerLoadModules), PrintStats(PrintStats) {
   // Initialize targets for object file support.
   llvm::InitializeAllTargets();
   llvm::InitializeAllTargetMCs();
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 32850f5eea92a9..a684ba9cf5ca0e 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -501,6 +501,8 @@ DependencyScanningWorker::DependencyScanningWorker(
     BaseFS = FS;
     break;
   }
+
+  PrintStats = Service.getPrintStats();
 }
 
 llvm::Error DependencyScanningWorker::computeDependencies(
@@ -623,8 +625,8 @@ bool DependencyScanningWorker::computeDependencies(
       ModifiedCommandLine ? *ModifiedCommandLine : CommandLine;
   auto &FinalFS = ModifiedFS ? ModifiedFS : BaseFS;
 
-  auto FileMgr =
-      llvm::makeIntrusiveRefCnt<FileManager>(FileSystemOptions{}, FinalFS);
+  auto FileMgr = llvm::makeIntrusiveRefCnt<FileManager>(FileSystemOptions{},
+                                                        FinalFS, PrintStats);
 
   std::vector<const char *> FinalCCommandLine(FinalCommandLine.size(), nullptr);
   llvm::transform(FinalCommandLine, FinalCCommandLine.begin(),
diff --git a/clang/tools/clang-scan-deps/ClangScanDeps.cpp b/clang/tools/clang-scan-deps/ClangScanDeps.cpp
index eaa76dd43e41dd..bef96213cd6e54 100644
--- a/clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ b/clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -973,7 +973,7 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) {
   };
 
   DependencyScanningService Service(ScanMode, Format, OptimizeArgs,
-                                    EagerLoadModules);
+                                    EagerLoadModules, PrintTiming);
 
   llvm::Timer T;
   T.startTimer();

``````````

</details>


https://github.com/llvm/llvm-project/pull/88427


More information about the cfe-commits mailing list