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