[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

[Alexandre Ganea via Phabricator via cfe-commits]

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