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

2024-04-26 Thread Alexandre Ganea via cfe-commits 

aganea wrote:

Thanks for pointing that out @MaskRay !

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-25 Thread Fangrui Song via cfe-commits 

MaskRay wrote:

(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.)

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-25 Thread Alexandre Ganea via cfe-commits 

https://github.com/aganea closed https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-24 Thread Alexandre Ganea via cfe-commits 

aganea wrote:

> 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 with Windows 11.

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-24 Thread Jan Svoboda via cfe-commits 

jansvoboda11 wrote:

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

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-24 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 approved this pull request.

LGTM, thanks!

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-24 Thread Alexandre Ganea via cfe-commits 


@@ -1293,6 +1293,10 @@ compileModuleImpl(CompilerInstance , 
SourceLocation ImportLoc,
 diag::remark_module_build_done)
 << ModuleName;
 
+  // Propagate the statistics to the parent FileManager.
+  if (FrontendOpts.ModulesShareFileManager)

aganea wrote:

Yes, you're right, fixed!

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-24 Thread Alexandre Ganea via cfe-commits 

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

>From 1b11d526e2cde1df64c7c4e05b2698b6d40926c3 Mon Sep 17 00:00:00 2001
From: Alexandre Ganea 
Date: Thu, 11 Apr 2024 13:02:30 -0400
Subject: [PATCH 1/3] [clang-scan-deps] Fix atomic contention when updating
 `TrackingStatistic`s in hot code paths in `FileManager`.

---
 clang/include/clang/Basic/FileManager.h   |  6 -
 .../DependencyScanningService.h   |  6 -
 .../DependencyScanningWorker.h|  2 ++
 clang/lib/Basic/FileManager.cpp   | 23 ---
 clang/lib/Frontend/CompilerInstance.cpp   |  3 ++-
 .../DependencyScanningService.cpp |  4 ++--
 .../DependencyScanningWorker.cpp  |  6 +++--
 clang/tools/clang-scan-deps/ClangScanDeps.cpp |  2 +-
 8 files changed, 36 insertions(+), 16 deletions(-)

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 {
   ///
   unsigned NextFileUID;
 
+  /// Whether we want to print statistics. This impacts the collection of data.
+  bool EnablePrintStats;
+
   // Caching.
   std::unique_ptr StatCache;
 
@@ -134,7 +137,8 @@ class FileManager : public RefCountedBase {
   /// \param FS if non-null, the VFS to use.  Otherwise uses
   /// llvm::vfs::getRealFileSystem().
   FileManager(const FileSystemOptions ,
-  IntrusiveRefCntPtr FS = nullptr);
+  IntrusiveRefCntPtr 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 ,
- IntrusiveRefCntPtr FS)
+ IntrusiveRefCntPtr 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 @@

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

2024-04-18 Thread Jan Svoboda via cfe-commits 


@@ -1293,6 +1293,10 @@ compileModuleImpl(CompilerInstance , 
SourceLocation ImportLoc,
 diag::remark_module_build_done)
 << ModuleName;
 
+  // Propagate the statistics to the parent FileManager.
+  if (FrontendOpts.ModulesShareFileManager)

jansvoboda11 wrote:

Shouldn't this condition be inverted? When 
`FrontendOpts.ModulesShareFileManager == true`, I'd expect 
`() == ()`. Maybe we 
can have an assertion in `FileManager::AddStats()` that would catch this.

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-17 Thread Alexandre Ganea via cfe-commits 

aganea wrote:

> 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.

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-17 Thread Alexandre Ganea via cfe-commits 

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

>From 1b11d526e2cde1df64c7c4e05b2698b6d40926c3 Mon Sep 17 00:00:00 2001
From: Alexandre Ganea 
Date: Thu, 11 Apr 2024 13:02:30 -0400
Subject: [PATCH 1/2] [clang-scan-deps] Fix atomic contention when updating
 `TrackingStatistic`s in hot code paths in `FileManager`.

---
 clang/include/clang/Basic/FileManager.h   |  6 -
 .../DependencyScanningService.h   |  6 -
 .../DependencyScanningWorker.h|  2 ++
 clang/lib/Basic/FileManager.cpp   | 23 ---
 clang/lib/Frontend/CompilerInstance.cpp   |  3 ++-
 .../DependencyScanningService.cpp |  4 ++--
 .../DependencyScanningWorker.cpp  |  6 +++--
 clang/tools/clang-scan-deps/ClangScanDeps.cpp |  2 +-
 8 files changed, 36 insertions(+), 16 deletions(-)

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 {
   ///
   unsigned NextFileUID;
 
+  /// Whether we want to print statistics. This impacts the collection of data.
+  bool EnablePrintStats;
+
   // Caching.
   std::unique_ptr StatCache;
 
@@ -134,7 +137,8 @@ class FileManager : public RefCountedBase {
   /// \param FS if non-null, the VFS to use.  Otherwise uses
   /// llvm::vfs::getRealFileSystem().
   FileManager(const FileSystemOptions ,
-  IntrusiveRefCntPtr FS = nullptr);
+  IntrusiveRefCntPtr 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 ,
- IntrusiveRefCntPtr FS)
+ IntrusiveRefCntPtr 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 @@

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

2024-04-13 Thread Alexandre Ganea via cfe-commits 

aganea wrote:

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

Bit 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 would also need a mechanism for 
collecting data when a thread ends through `pthread_key_create/FlsCallback`s.  
It would be a bit more involved than what's there currently, but that should 
fix the issue I'm seeing (and maybe others).

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-12 Thread Volodymyr Sapsai via cfe-commits 

vsapsai wrote:

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.

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-12 Thread Jan Svoboda via cfe-commits 

jansvoboda11 wrote:

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)

https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-11 Thread Alexandre Ganea via cfe-commits 

https://github.com/aganea edited https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-11 Thread Alexandre Ganea via cfe-commits 

https://github.com/aganea edited https://github.com/llvm/llvm-project/pull/88427
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-04-11 Thread via cfe-commits 

llvmbot wrote:




@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.

![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 {
   ///
   unsigned NextFileUID;
 
+  /// Whether we want to print statistics. This impacts the collection of data.
+  bool EnablePrintStats;
+
   // Caching.
   std::unique_ptr StatCache;
 
@@ -134,7 +137,8 @@ class FileManager : public RefCountedBase {
   /// \param FS if non-null, the VFS to use.  Otherwise uses
   /// llvm::vfs::getRealFileSystem().
   FileManager(const FileSystemOptions ,
-  IntrusiveRefCntPtr FS = nullptr);
+  IntrusiveRefCntPtr 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

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

2024-04-11 Thread Alexandre Ganea via cfe-commits 

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

`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.

>From 1b11d526e2cde1df64c7c4e05b2698b6d40926c3 Mon Sep 17 00:00:00 2001
From: Alexandre Ganea 
Date: Thu, 11 Apr 2024 13:02:30 -0400
Subject: [PATCH] [clang-scan-deps] Fix atomic contention when updating
 `TrackingStatistic`s in hot code paths in `FileManager`.

---
 clang/include/clang/Basic/FileManager.h   |  6 -
 .../DependencyScanningService.h   |  6 -
 .../DependencyScanningWorker.h|  2 ++
 clang/lib/Basic/FileManager.cpp   | 23 ---
 clang/lib/Frontend/CompilerInstance.cpp   |  3 ++-
 .../DependencyScanningService.cpp |  4 ++--
 .../DependencyScanningWorker.cpp  |  6 +++--
 clang/tools/clang-scan-deps/ClangScanDeps.cpp |  2 +-
 8 files changed, 36 insertions(+), 16 deletions(-)

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 {
   ///
   unsigned NextFileUID;
 
+  /// Whether we want to print statistics. This impacts the collection of data.
+  bool EnablePrintStats;
+
   // Caching.
   std::unique_ptr StatCache;
 
@@ -134,7 +137,8 @@ class FileManager : public RefCountedBase {
   /// \param FS if non-null, the VFS to use.  Otherwise uses
   /// llvm::vfs::getRealFileSystem().
   FileManager(const FileSystemOptions ,
-  IntrusiveRefCntPtr FS = nullptr);
+  IntrusiveRefCntPtr 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