aganea added subscribers: sammccall, JDevlieghere.
aganea added inline comments.
================
Comment at:
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:1
+//===- DependencyScanningFilesystem.h - clang-scan-deps fs ===---*- C++
-*-===//
+//
----------------
General comment for this file and the implementation: it seems there's nothing
specific to the dependency scanning, except for replacing the content with
minimized content? This cached FS could be very well used to compile a project
with parallel workers. Could this be part of the `VirtualFileSystem`
infrastructure? ( `llvm/include/llvm/Support/VirtualFileSystem.h`) If yes, can
you possibly create a separate patch for this? @JDevlieghere @sammccall
================
Comment at:
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:101
+public:
+ /// A \c CachedFileSystemEntry with a lock.
+ struct SharedFileSystemEntry {
----------------
The struct is self-explanatory, not sure the comment is needed here?
================
Comment at:
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:103
+ struct SharedFileSystemEntry {
+ std::mutex Lock;
+ CachedFileSystemEntry Value;
----------------
Would you mind renaming this to `ValueLock` so it is easier to search for? (and
to remain consistent with `CacheLock` below)
================
Comment at:
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:129
+/// computation.
+class DependencyScanningFilesystem : public llvm::vfs::ProxyFileSystem {
+public:
----------------
Maybe worth mentioning this is NOT a global, thread-safe, class like
`DependencyScanningFilesystemSharedCache`, but rather meant to be used as
per-thread instances?
I am also wondering if there could be a better name to signify at first sight
that this is a per-thread class. `DependencyScanningLocalFilesystem`?
`DependencyScanningWorkerFilesystem`?
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:84
+ DependencyScanningFilesystemSharedCache() {
+ NumShards = 8;
+ CacheShards = llvm::make_unique<CacheShard[]>(NumShards);
----------------
This line needs a comment. Is this value based on empirical results across
different hardwares? (I can't recall your answer at the LLVM conf) I am
wondering what would be the good strategy here? The more cores/HT in your PC,
the more chances that you'll hit the same shard, thus locking. But the bigger
we make this value, the more `StringMaps` we will have, and more cache misses
possibly.
Should it be something like `llvm::hardware_concurrency() / some_fudge`? It'd
be interesting to subsequently profile on high core count machines (or maybe
you have done that).
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:210
+ bool KeepOriginalSource = IgnoredFiles.count(Filename);
+ auto &SharedCacheEntry = SharedCache.get(Filename);
+ const CachedFileSystemEntry *Result;
----------------
Don't use `auto` when the type is not obvious.
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:148
+ RealFS = new ProxyFileSystemWithoutChdir(llvm::vfs::getRealFileSystem());
+ if (Service.getMode() == ScanningMode::MinimizedSourcePreprocessing)
+ DepFS = new DependencyScanningFilesystem(Service.getSharedCache(), RealFS);
----------------
Can we not use caching all the time?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63907/new/
https://reviews.llvm.org/D63907
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
