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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thanks for the update Alex! Just a few more comments and we should be good to go:



================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:117
+    std::mutex CacheLock;
+    llvm::StringMap<std::unique_ptr<SharedFileSystemEntry>> Cache;
+  };
----------------
Why not use a bump allocator here? (it would be per-thread) On Windows the heap is always thread-safe, which induce a lock in `malloc` for each new entry. You could also avoid the usage of `unique_ptr` by the same occasion:

`llvm::StringMap<SharedFileSystemEntry, SpecificBumpPtrAllocator<SharedFileSystemEntry>> Cache;`

//(unless you're planning on removing entries from the cache later on?)//


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:76
+CachedFileSystemEntry
+CachedFileSystemEntry::createDirectoryEntry(llvm::vfs::Status Stat) {
+  assert(Stat.isDirectory() && "not a directory!");
----------------
`llvm::vfs::Status &&Stat` to avoid a copy?


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:103
+  auto It =
+      Shard.Cache.try_emplace(Key, std::unique_ptr<SharedFileSystemEntry>());
+  auto &Ptr = It.first->getValue();
----------------
`Shard.Cache.try_emplace(Key)` will provide a default constructed value if it's not there.


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:117
+  // Check the local cache first.
+  if (const auto *Entry = getCachedEntry(Filename))
+    return Entry->getStatus();
----------------
`CachedFileSystemEntry *Entry` ?


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:198
+  // Check the local cache first.
+  if (const auto *Entry = getCachedEntry(Filename))
+    return createFile(Entry);
----------------
CachedFileSystemEntry *Entry ?


================
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);
----------------
arphaman wrote:
> aganea wrote:
> > arphaman wrote:
> > > aganea wrote:
> > > > Can we not use caching all the time?
> > > We want to have a mode where it's as close to the regular `clang -E` invocation as possible for correctness CI and testing. We also haven't evaluated if the cost of keeping non-minimized sources in memory, so it might be too expensive for practical use? I can add a third option that caches but doesn't minimize though as a follow-up patch though 
> > > 
> > Yes that would be nice. As for the size taken in RAM, it would be only .H files, right? For Clang+LLVM+LLD I'm counting about 40 MB. But indeed with a large project, that would be about 1.5 GB of .H. Although I doubt all these files will be loaded at once in memory (I'll check)
> > 
> > As for the usage: Fastbuild works like distcc (plain mode, not pump) so we were also planning on extracting the fully preprocessed output, not only the dependencies. There's one use-case where we need to preprocess locally, then send the preprocessed output remotely for compilation. And another use-case where we only want to extract the dependency list, compute a digest, to retrieve the OBJ from a network cache.
> Right now it doesn't differentiate between .H and other files, but we could teach it do have a header only filter for the cache.
No worries, I was simply wondering about the size in memory.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63907/new/

https://reviews.llvm.org/D63907