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

aganea · 2024-04-11T19:30:54Z

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.

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 #88152 (comment), that is:

static bool shouldCacheStatFailures(StringRef Filename) {
  return true;
}

Without the above, there's just too much OS noise from the high volume of status() calls with regular non-modules C++ code. Tested on Windows with clang-cl.

…tic`s in hot code paths in `FileManager`.

llvmbot · 2024-04-11T19:31:25Z

@llvm/pr-subscribers-clang

Author: Alexandre Ganea (aganea)

Changes

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.

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 #88152 (comment), 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 --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();

jansvoboda11 · 2024-04-12T18:17:24Z

I'd like for @vsapsai to chime in.

As an alternative approach: could we turn these into member variables, make them non-atomic and take care to update the stats of the superior FileManager whenever an inferior FileManager goes out of scope? (e.g. after implicitly building a module)

vsapsai · 2024-04-13T01:46:31Z

The bigger idea is that not enabled stats should be negligibly cheap. As for me, the problem is in that not being true. Let me check how we have expensive stats all the time.

Extra fixes are good but I'm concerned they help only with these specific stats while others might still cause problems in different contexts.

aganea · 2024-04-13T17:36:43Z

I think in the short term @jansvoboda11's suggestion should be good enough.

But if we want Statistics to be always cheap, we should make them thread_local instead, not atomic. getValue() could do the "collection" of data over all active, or past threads. It might also need a mechanism for collecting data when a thread ends through pthread_key_create/FlsCallbacks. It would be a bit more involved than what's there currently, but that should fix the issue I'm seeing (and maybe others).

aganea · 2024-04-17T14:23:55Z

As an alternative approach: could we turn these into member variables, make them non-atomic and take care to update the stats of the superior FileManager whenever an inferior FileManager goes out of scope? (e.g. after implicitly building a module)

As suggested.

clang/lib/Frontend/CompilerInstance.cpp

jansvoboda11

LGTM, thanks!

jansvoboda11 · 2024-04-24T16:38:45Z

(Out of interest, what machine are you seeing the contention with?)

aganea · 2024-04-24T16:40:32Z

LGTM, thanks!

Thanks for the review!

(Out of interest, what machine are you seeing the contention with?)

It's a ThreadRipper Pro 3975WX 32c/64t running on Windows 11.

MaskRay · 2024-04-26T00:54:07Z

(BTW: https://discourse.llvm.org/t/hidden-emails-on-github-should-we-do-something-about-it/74223 The recommendation is to avoid the github "hidden email" feature.)

aganea · 2024-04-26T11:35:49Z

Thanks for pointing that out @MaskRay !

[clang-scan-deps] Fix atomic contention when updating `TrackingStatis…

1b11d52

…tic`s in hot code paths in `FileManager`.

aganea requested review from Bigcheese, benlangmuir, jansvoboda11 and ChuanqiXu9 April 11, 2024 19:30

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 11, 2024

jansvoboda11 requested a review from vsapsai April 11, 2024 23:34

Revert some changes and make statistics members of the FileManager

f9c5316

jansvoboda11 reviewed Apr 18, 2024

View reviewed changes

clang/lib/Frontend/CompilerInstance.cpp Outdated Show resolved Hide resolved

Fix condition.

183e149

jansvoboda11 approved these changes Apr 24, 2024

View reviewed changes

aganea merged commit 39ed3c6 into llvm:main Apr 25, 2024

aganea deleted the improve_perf_scan_deps branch April 25, 2024 14:31

aganea restored the improve_perf_scan_deps branch June 12, 2024 16:36

jansvoboda11 mentioned this pull request Aug 8, 2024

[llvm][support] Implement tracing virtual file system #88326

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

aganea commented Apr 11, 2024 •

edited

Loading

Uh oh!

llvmbot commented Apr 11, 2024

Uh oh!

jansvoboda11 commented Apr 12, 2024

Uh oh!

vsapsai commented Apr 13, 2024

Uh oh!

aganea commented Apr 13, 2024 •

edited

Loading

Uh oh!

aganea commented Apr 17, 2024

Uh oh!

Uh oh!

jansvoboda11 left a comment

Uh oh!

jansvoboda11 commented Apr 24, 2024

Uh oh!

aganea commented Apr 24, 2024 •

edited

Loading

Uh oh!

MaskRay commented Apr 26, 2024

Uh oh!

aganea commented Apr 26, 2024

Uh oh!

Uh oh!

[clang-scan-deps] Fix contention when updating TrackingStatistics in hot code paths in FileManager. #88427

[clang-scan-deps] Fix contention when updating TrackingStatistics in hot code paths in FileManager. #88427

Uh oh!

Conversation

aganea commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 11, 2024

Uh oh!

jansvoboda11 commented Apr 12, 2024

Uh oh!

vsapsai commented Apr 13, 2024

Uh oh!

aganea commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aganea commented Apr 17, 2024

Uh oh!

Uh oh!

jansvoboda11 left a comment

Choose a reason for hiding this comment

Uh oh!

jansvoboda11 commented Apr 24, 2024

Uh oh!

aganea commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaskRay commented Apr 26, 2024

Uh oh!

aganea commented Apr 26, 2024

Uh oh!

Uh oh!

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

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

aganea commented Apr 11, 2024 •

edited

Loading

aganea commented Apr 13, 2024 •

edited

Loading

aganea commented Apr 24, 2024 •

edited

Loading