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 <aga...@havenstudios.com> 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<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(); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits