[clang][deps] Add in-flight query caching to DependencyScanningFilesystemSharedCache#199680
Open
artemcm wants to merge 2 commits into
Open
[clang][deps] Add in-flight query caching to DependencyScanningFilesystemSharedCache#199680artemcm wants to merge 2 commits into
DependencyScanningFilesystemSharedCache#199680artemcm wants to merge 2 commits into
Conversation
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
artemcm
force-pushed
the
artemcm/CacheDepScanFileSystemInProgressQuery
branch
2 times, most recently
from
88f8685 to
8bc818a
Compare
llvmorg-github-actions
Bot
added
the
clang
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-clang Author: Artem Chikin (artemcm) ChangesConcurrent dep-scan workers querying the same filename or UID each issue their own Add per-key in-flight tracking to
Patch is 23.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/199680.diff 3 Files Affected:
diff --git a/clang/include/clang/DependencyScanning/DependencyScanningFilesystem.h b/clang/include/clang/DependencyScanning/DependencyScanningFilesystem.h
index f50332c964ced..81f76e3250362 100644
--- a/clang/include/clang/DependencyScanning/DependencyScanningFilesystem.h
+++ b/clang/include/clang/DependencyScanning/DependencyScanningFilesystem.h
@@ -16,6 +16,8 @@
#include "llvm/Support/Allocator.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/VirtualFileSystem.h"
+#include <condition_variable>
+#include <memory>
#include <mutex>
#include <optional>
#include <variant>
@@ -152,6 +154,28 @@ using CachedRealPath = llvm::ErrorOr<std::string>;
/// the worker threads.
class DependencyScanningFilesystemSharedCache {
public:
+ /// Tracks a cache entry whose value is currently being computed by one
+ /// worker so that other workers arriving at the same key can wait for the
+ /// result rather than producing it in parallel. The producer publishes the
+ /// resolved entry into \c Result, sets \c Done, and notifies waiters via
+ /// \c CondVar. Waiters synchronize through the owning shard's \c CacheLock.
+ struct InProgressEntry {
+ std::condition_variable CondVar;
+ bool Done = false;
+ const CachedFileSystemEntry *Result = nullptr;
+ };
+
+ /// Outcome of attempting to claim a slot for a given key. Exactly one of
+ /// \c Resolved or \c Produce is non-null:
+ /// - \c Resolved: the cache (or another worker) has already produced the
+ /// entry; the caller should use it.
+ /// - \c Produce: the caller has the right to produce the entry and
+ /// must call the matching \c fulfil*Slot once finished.
+ struct SlotAcquisitionResult {
+ const CachedFileSystemEntry *Resolved;
+ std::shared_ptr<InProgressEntry> Produce;
+ };
+
struct CacheShard {
/// The mutex that needs to be locked before mutation of any member.
mutable std::mutex CacheLock;
@@ -166,6 +190,18 @@ class DependencyScanningFilesystemSharedCache {
llvm::DenseMap<llvm::sys::fs::UniqueID, const CachedFileSystemEntry *>
EntriesByUID;
+ /// Filenames whose cache entry is currently being computed. A second
+ /// worker reaching the same filename will wait on the entry's condition
+ /// variable rather than racing the underlying filesystem.
+ llvm::StringMap<std::shared_ptr<InProgressEntry>> InProgressByFilename;
+
+ /// Unique IDs whose cache entry is currently being computed. A worker
+ /// that has stat()-ed a file and gotten a UID already in flight (perhaps
+ /// reached under a different filename) will wait here instead of opening
+ /// the file a second time.
+ llvm::DenseMap<llvm::sys::fs::UniqueID, std::shared_ptr<InProgressEntry>>
+ InProgressByUID;
+
/// The backing storage for cached entries.
llvm::SpecificBumpPtrAllocator<CachedFileSystemEntry> EntryStorage;
@@ -202,6 +238,35 @@ class DependencyScanningFilesystemSharedCache {
getOrInsertEntryForFilename(StringRef Filename,
const CachedFileSystemEntry &Entry);
+ /// Claims a slot for \p Filename. If a resolved entry already exists,
+ /// returns it. If another worker is currently producing a result for this
+ /// filename, blocks on its \c InProgressEntry until done and returns the
+ /// produced entry. Otherwise installs a fresh \c InProgressEntry and
+ /// returns it as a producer slot which the caller must complete via
+ /// \c fulfilFilenameSlot.
+ SlotAcquisitionResult acquireFilenameSlot(StringRef);
+
+ /// Claims a slot for \p UID. Same semantics as \c acquireFilenameSlot but
+ /// keyed by unique ID; used after \c stat() has identified a file so that
+ /// concurrent \c readFile() calls for the same on-disk file (reached via
+ /// different filenames) collapse onto a single open.
+ SlotAcquisitionResult acquireUIDSlot(llvm::sys::fs::UniqueID);
+
+ /// Completes a producer slot acquired via \c acquireFilenameSlot.
+ /// Publishes \p Result under \p Filename in \c CacheByFilename,
+ /// records it in the slot, marks the slot done, removes the
+ /// in-progress entry, and notifies waiters.
+ void fulfilFilenameSlot(StringRef, const std::shared_ptr<InProgressEntry> &,
+ const CachedFileSystemEntry *);
+
+ /// Completes a producer slot acquired via \c acquireUIDSlot.
+ /// Publishes \p Result under \p UID in \c EntriesByUID, records
+ /// it in the slot, marks the slot done, removes the in-progress
+ /// entry, and notifies waiters.
+ void fulfilUIDSlot(llvm::sys::fs::UniqueID,
+ const std::shared_ptr<InProgressEntry> &,
+ const CachedFileSystemEntry *);
+
/// Returns the real path associated with the filename or nullptr if none is
/// found.
const CachedRealPath *findRealPathByFilename(StringRef Filename) const;
@@ -439,22 +504,6 @@ class DependencyScanningWorkerFilesystem
const CachedFileSystemEntry &
getOrEmplaceSharedEntryForUID(TentativeEntry TEntry);
- /// Returns entry associated with the filename or nullptr if none is found.
- ///
- /// Returns entry from local cache if there is some. Otherwise, if the entry
- /// is found in the shared cache, writes it through the local cache and
- /// returns it. Otherwise returns nullptr.
- const CachedFileSystemEntry *
- findEntryByFilenameWithWriteThrough(StringRef Filename);
-
- /// Returns entry associated with the unique ID in the shared cache or nullptr
- /// if none is found.
- const CachedFileSystemEntry *
- findSharedEntryByUID(llvm::vfs::Status Stat) const {
- return SharedCache.getShardForUID(Stat.getUniqueID())
- .findEntryByUID(Stat.getUniqueID());
- }
-
/// Associates the given entry with the filename in the local cache and
/// returns it.
const CachedFileSystemEntry &
diff --git a/clang/lib/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/DependencyScanning/DependencyScanningFilesystem.cpp
index 49dad3758cf57..915b28dd64c07 100644
--- a/clang/lib/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -184,6 +184,101 @@ DependencyScanningFilesystemSharedCache::CacheShard::
return *CachedEntry;
}
+DependencyScanningFilesystemSharedCache::SlotAcquisitionResult
+DependencyScanningFilesystemSharedCache::CacheShard::acquireFilenameSlot(
+ StringRef Filename) {
+ assert(llvm::sys::path::is_absolute_gnu(Filename));
+ std::unique_lock<std::mutex> LockGuard(CacheLock);
+ // Cache hit.
+ if (auto It = CacheByFilename.find(Filename); It != CacheByFilename.end()) {
+ if (const auto *Entry = It->getValue().first)
+ return SlotAcquisitionResult{Entry, nullptr};
+ }
+
+ // Another worker is producing for this filename, wait for it.
+ if (auto It = InProgressByFilename.find(Filename);
+ It != InProgressByFilename.end()) {
+ std::shared_ptr<InProgressEntry> Pending = It->second;
+ Pending->CondVar.wait(LockGuard, [&] { return Pending->Done; });
+ assert(Pending->Result &&
+ "in-progress filename slot fulfilled without an entry");
+ return SlotAcquisitionResult{Pending->Result, nullptr};
+ }
+
+ // Install an in-progress entry and return it.
+ auto Pending = std::make_shared<InProgressEntry>();
+ InProgressByFilename.try_emplace(Filename, Pending);
+ return SlotAcquisitionResult{nullptr, std::move(Pending)};
+}
+
+DependencyScanningFilesystemSharedCache::SlotAcquisitionResult
+DependencyScanningFilesystemSharedCache::CacheShard::acquireUIDSlot(
+ llvm::sys::fs::UniqueID UID) {
+ std::unique_lock<std::mutex> LockGuard(CacheLock);
+ // Cache hit.
+ if (auto It = EntriesByUID.find(UID); It != EntriesByUID.end())
+ return SlotAcquisitionResult{It->getSecond(), nullptr};
+
+ // Another worker is producing for this UID, wait for it.
+ if (auto It = InProgressByUID.find(UID); It != InProgressByUID.end()) {
+ std::shared_ptr<InProgressEntry> Pending = It->second;
+ Pending->CondVar.wait(LockGuard, [&] { return Pending->Done; });
+ assert(Pending->Result &&
+ "in-progress UID slot fulfilled without an entry");
+ return SlotAcquisitionResult{Pending->Result, nullptr};
+ }
+
+ // Install an in-progress entry and return it.
+ auto Pending = std::make_shared<InProgressEntry>();
+ InProgressByUID.try_emplace(UID, Pending);
+ return SlotAcquisitionResult{nullptr, std::move(Pending)};
+}
+
+void DependencyScanningFilesystemSharedCache::CacheShard::fulfilFilenameSlot(
+ StringRef Filename,
+ const std::shared_ptr<
+ DependencyScanningFilesystemSharedCache::InProgressEntry> &IPE,
+ const CachedFileSystemEntry *Result) {
+ {
+ std::lock_guard<std::mutex> LockGuard(CacheLock);
+ if (Result) {
+ // Publish the entry under this filename for future direct lookups,
+ // mirroring the semantics of getOrInsertEntryForFilename.
+ auto [It, Inserted] =
+ CacheByFilename.insert({Filename, {Result, nullptr}});
+ auto &[CachedEntry, CachedRealPath] = It->getValue();
+ if (!Inserted || !CachedEntry)
+ CachedEntry = Result;
+ }
+ IPE->Result = Result;
+ IPE->Done = true;
+ InProgressByFilename.erase(Filename);
+ }
+ // Notify other workers awaiting this result.
+ IPE->CondVar.notify_all();
+}
+
+void DependencyScanningFilesystemSharedCache::CacheShard::fulfilUIDSlot(
+ llvm::sys::fs::UniqueID UID,
+ const std::shared_ptr<
+ DependencyScanningFilesystemSharedCache::InProgressEntry> &IPE,
+ const CachedFileSystemEntry *Result) {
+ {
+ std::lock_guard<std::mutex> LockGuard(CacheLock);
+ if (Result) {
+ // Publish the entry under this UID.
+ auto [It, Inserted] = EntriesByUID.try_emplace(UID, Result);
+ if (!Inserted && !It->getSecond())
+ It->getSecond() = Result;
+ }
+ IPE->Result = Result;
+ IPE->Done = true;
+ InProgressByUID.erase(UID);
+ }
+ // Notify other workers awaiting this result.
+ IPE->CondVar.notify_all();
+}
+
const CachedFileSystemEntry &
DependencyScanningFilesystemSharedCache::CacheShard::getOrEmplaceEntryForUID(
llvm::sys::fs::UniqueID UID, llvm::vfs::Status Stat,
@@ -262,44 +357,73 @@ DependencyScanningWorkerFilesystem::getOrEmplaceSharedEntryForUID(
std::move(TEntry.Contents));
}
-const CachedFileSystemEntry *
-DependencyScanningWorkerFilesystem::findEntryByFilenameWithWriteThrough(
- StringRef Filename) {
- if (const auto *Entry = LocalCache.findEntryByFilename(Filename))
- return Entry;
- auto &Shard = SharedCache.getShardForFilename(Filename);
- if (const auto *Entry = Shard.findEntryByFilename(Filename))
- return &LocalCache.insertEntryForFilename(Filename, *Entry);
- return nullptr;
-}
-
llvm::ErrorOr<const CachedFileSystemEntry &>
DependencyScanningWorkerFilesystem::computeAndStoreResult(
StringRef OriginalFilename, StringRef FilenameForLookup) {
- llvm::ErrorOr<llvm::vfs::Status> Stat =
- getUnderlyingFS().status(OriginalFilename);
- if (!Stat) {
- const auto &Entry =
- getOrEmplaceSharedEntryForFilename(FilenameForLookup, Stat.getError());
- return insertLocalEntryForFilename(FilenameForLookup, Entry);
- }
-
- if (const auto *Entry = findSharedEntryByUID(*Stat))
- return insertLocalEntryForFilename(FilenameForLookup, *Entry);
-
- auto TEntry =
- Stat->isDirectory() ? TentativeEntry(*Stat) : readFile(OriginalFilename);
+ auto &FilenameShard = SharedCache.getShardForFilename(FilenameForLookup);
+
+ // Acquire a per-filename in-progress entry. If another worker has already
+ // produced an entry under this filename, or is currently producing one,
+ // adopt its result instead of duplicating the underlying filesystem.
+ auto FilenameSlot = FilenameShard.acquireFilenameSlot(FilenameForLookup);
+ if (FilenameSlot.Resolved)
+ return insertLocalEntryForFilename(FilenameForLookup,
+ *FilenameSlot.Resolved);
+
+ // Compute the result.
+ std::shared_ptr<DependencyScanningFilesystemSharedCache::InProgressEntry>
+ ProducerSlot = std::move(FilenameSlot.Produce);
+ const CachedFileSystemEntry *ProducedEntry = nullptr;
+ auto ComputeResult = [&]() -> llvm::ErrorOr<const CachedFileSystemEntry &> {
+ llvm::ErrorOr<llvm::vfs::Status> Stat =
+ getUnderlyingFS().status(OriginalFilename);
+ if (!Stat) {
+ const auto &Entry = getOrEmplaceSharedEntryForFilename(FilenameForLookup,
+ Stat.getError());
+ ProducedEntry = &Entry;
+ return insertLocalEntryForFilename(FilenameForLookup, Entry);
+ }
- const CachedFileSystemEntry *SharedEntry = [&]() {
- if (TEntry) {
- const auto &UIDEntry = getOrEmplaceSharedEntryForUID(std::move(*TEntry));
- return &getOrInsertSharedEntryForFilename(FilenameForLookup, UIDEntry);
+ // Acquire a per-UID producer slot to dedup concurrent readFile()
+ // calls across workers that arrived under different filenames
+ // pointing at the same on-disk file.
+ auto &UIDShard = SharedCache.getShardForUID(Stat->getUniqueID());
+ auto UIDSlot = UIDShard.acquireUIDSlot(Stat->getUniqueID());
+ const CachedFileSystemEntry *SharedEntry = nullptr;
+ if (UIDSlot.Resolved) {
+ SharedEntry = UIDSlot.Resolved;
+ } else {
+ auto TEntry = Stat->isDirectory() ? TentativeEntry(*Stat)
+ : readFile(OriginalFilename);
+ if (TEntry) {
+ SharedEntry = &getOrEmplaceSharedEntryForUID(std::move(*TEntry));
+ // Publish the UID-keyed entry to anyone waiting on this UID.
+ UIDShard.fulfilUIDSlot(Stat->getUniqueID(), UIDSlot.Produce,
+ SharedEntry);
+ } else {
+ // `readFile` failed despite `stat` succeeding. Cache
+ // the failure under the filename, and publish that same entry under
+ // the UID so that awaiting workers surface the error rather than racing
+ // to retry the open.
+ SharedEntry = &getOrEmplaceSharedEntryForFilename(FilenameForLookup,
+ TEntry.getError());
+ UIDShard.fulfilUIDSlot(Stat->getUniqueID(), UIDSlot.Produce,
+ SharedEntry);
+ }
}
- return &getOrEmplaceSharedEntryForFilename(FilenameForLookup,
- TEntry.getError());
- }();
- return insertLocalEntryForFilename(FilenameForLookup, *SharedEntry);
+ // Bind the resolved/produced entry to this filename in the shared cache
+ // (idempotent if already there) and the local cache.
+ SharedEntry =
+ &getOrInsertSharedEntryForFilename(FilenameForLookup, *SharedEntry);
+ ProducedEntry = SharedEntry;
+ return insertLocalEntryForFilename(FilenameForLookup, *SharedEntry);
+ };
+
+ auto Result = ComputeResult();
+ FilenameShard.fulfilFilenameSlot(FilenameForLookup, ProducerSlot,
+ ProducedEntry);
+ return Result;
}
llvm::ErrorOr<EntryRef>
@@ -310,9 +434,9 @@ DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
if (!FilenameForLookup)
return FilenameForLookup.getError();
- if (const auto *Entry =
- findEntryByFilenameWithWriteThrough(*FilenameForLookup))
+ if (const auto *Entry = LocalCache.findEntryByFilename(*FilenameForLookup))
return EntryRef(OriginalFilename, *Entry).unwrapError();
+
auto MaybeEntry = computeAndStoreResult(OriginalFilename, *FilenameForLookup);
if (!MaybeEntry)
return MaybeEntry.getError();
diff --git a/clang/unittests/DependencyScanning/DependencyScanningFilesystemTest.cpp b/clang/unittests/DependencyScanning/DependencyScanningFilesystemTest.cpp
index d9489a9bf27ca..5dab80e61371f 100644
--- a/clang/unittests/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ b/clang/unittests/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -10,9 +10,68 @@
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/VirtualFileSystem.h"
#include "gtest/gtest.h"
+#include <atomic>
+#include <condition_variable>
+#include <mutex>
+#include <thread>
using namespace clang::dependencies;
+namespace {
+
+/// VFS proxy that counts operations using atomic counters, suitable for use
+/// from concurrent tests where `llvm::vfs::TracingFileSystem`'s plain
+/// `std::size_t` counters would race.
+class AtomicTracingFileSystem : public llvm::vfs::ProxyFileSystem {
+public:
+ std::atomic<size_t> NumStatusCalls{0};
+ std::atomic<size_t> NumOpenFileForReadCalls{0};
+ std::atomic<size_t> NumGetRealPathCalls{0};
+
+ AtomicTracingFileSystem(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS)
+ : ProxyFileSystem(std::move(FS)) {}
+
+ llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
+ ++NumStatusCalls;
+ return ProxyFileSystem::status(Path);
+ }
+
+ llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
+ openFileForRead(const llvm::Twine &Path) override {
+ ++NumOpenFileForReadCalls;
+ return ProxyFileSystem::openFileForRead(Path);
+ }
+
+ std::error_code getRealPath(const llvm::Twine &Path,
+ llvm::SmallVectorImpl<char> &Output) override {
+ ++NumGetRealPathCalls;
+ return ProxyFileSystem::getRealPath(Path, Output);
+ }
+};
+
+/// Releases all waiting threads simultaneously so that the worker logic can
+/// be observed under maximal concurrency rather than a thread-spawn cascade.
+struct StartBarrier {
+ std::mutex M;
+ std::condition_variable CV;
+ bool Released = false;
+
+ void wait() {
+ std::unique_lock<std::mutex> Lock(M);
+ CV.wait(Lock, [&] { return Released; });
+ }
+
+ void release() {
+ {
+ std::lock_guard<std::mutex> Lock(M);
+ Released = true;
+ }
+ CV.notify_all();
+ }
+};
+
+} // namespace
+
TEST(DependencyScanningFilesystem, OpenFileAndGetBufferRepeatedly) {
auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
InMemoryFS->setCurrentWorkingDirectory("/");
@@ -286,3 +345,138 @@ TEST(DependencyScanningFilesystem, DoNotDiagnoseDirSizeChange) {
auto InvalidEntries = SharedCache.getOutOfDateEntries(*FS);
EXPECT_EQ(InvalidEntries.size(), 0u);
}
+
+TEST(DependencyScanningWorkerFilesystem, ConcurrentSameFilenameDeduplicates) {
+ auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ InMemoryFS->setCurrentWorkingDirectory("/");
+ InMemoryFS->addFile("/foo.c", 0, llvm::MemoryBuffer::getMemBuffer("hello"));
+
+ auto TracingFS =
+ llvm::makeIntrusiveRefCnt<AtomicTracingFileSystem>(InMemoryFS);
+ DependencyScanningFilesystemSharedCache SharedCache;
+
+ constexpr unsigned NumWorkers = 16;
+ std::vector<std::unique_ptr<DependencyScanningWorkerFilesystem>> Workers;
+ Workers.reserve(NumWorkers);
+ for (unsigned I = 0; I < NumWorkers; ++I)
+ Workers.push_back(std::make_unique<DependencyScanningWorkerFilesystem>(
+ SharedCache, TracingFS));
+
+ StartBarrier Barrier;
+ std::vector<std::thread> Threads;
+ std::vector<llvm::ErrorOr<EntryRef>> Results(
+ NumWorkers, llvm::ErrorOr<EntryRef>(std::error_code{}));
+ Threads.reserve(NumWorkers);
+ for (unsigned I = 0; I < NumWorkers; ++I) {
+ Threads.emplace_back([&, I] {
+ Barrier.wait();
+ Results[I] = Workers[I]->getOrCreateFileSystemEntry("/foo.c");
+ });
+ }
+ Barrier.release();
+ for (auto &T : Threads)
+ T.join();
+
+ EXPECT_EQ(TracingFS->NumStatusCalls.load(), 1u);
+ EXPECT_EQ(TracingFS->NumOpenFileForReadCalls.load(), 1u);
+
+ // All workers must have observed the same underlying entry.
+ ASSERT_TRUE(Results[0]);
+ llvm::sys::fs::UniqueID FirstUID = Results[0]->getStatus().getUniqueID();
+ const char *FirstContents = Results[0]->getContents().data();
+ for (unsigned I = 0; I < NumWorkers; ++I) {
+ ASSERT_TRUE(Results[I]) << "worker " << I << " failed";
+ EXPECT_EQ(Results[I]->getStatus().getUniqueID(), FirstUID);
+ EXPECT_EQ(Results[I]->getContents().data(), FirstContents);
+ }
+}
+
+TEST(DependencyScanningWorkerFilesystem,
+ ConcurrentSameUIDDifferentFilenamesDeduplicatesOpen) {
+ auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ InMemoryFS->setCurrentWorkingDirectory("/");
+ InMemoryFS->addFile("/real.c", 0, llvm::Memor...
[truncated]
|
Contributor
jansvoboda11
left a comment
jansvoboda11
left a comment
There was a problem hiding this comment.
This is a great finding and the new approach makes sense conceptually. I think we can improve clarity and performance with some tweaks to the implementation.
| if (!Stat) { | ||
| const auto &Entry = getOrEmplaceSharedEntryForFilename(FilenameForLookup, | ||
| Stat.getError()); | ||
| ProducedEntry = &Entry; |
Contributor
There was a problem hiding this comment.
Can the caller of ComputeResult use the return value instead of setting up the ProducedEntry out-param?
| /// Filenames whose cache entry is currently being computed. A second | ||
| /// worker reaching the same filename will wait on the entry's condition | ||
| /// variable rather than racing the underlying filesystem. | ||
| llvm::StringMap<std::shared_ptr<InProgressEntry>> InProgressByFilename; |
Contributor
There was a problem hiding this comment.
The keys here can get fairly long. How hard would it be to fold these new maps into the existing ones?
Contributor
Author
There was a problem hiding this comment.
Folded the maps into existing ones. Thanks.
| return insertLocalEntryForFilename(FilenameForLookup, *SharedEntry); | ||
| }; | ||
|
|
||
| auto Result = ComputeResult(); |
Contributor
There was a problem hiding this comment.
Since this is the only use of the lambda, I'd find immediately-invoked lambda a bit clearer.
| if (auto It = InProgressByFilename.find(Filename); | ||
| It != InProgressByFilename.end()) { | ||
| std::shared_ptr<InProgressEntry> Pending = It->second; | ||
| Pending->CondVar.wait(LockGuard, [&] { return Pending->Done; }); |
Contributor
There was a problem hiding this comment.
This wait happens while still holding the shared shard lock:
std::unique_lock<std::mutex> LockGuard(CacheLock);Can you refactor this such that you only hold CacheLock to manage the map, and release it before you start waiting on the entry?
Contributor
Author
There was a problem hiding this comment.
std::condition_variable does not actually hold the lock while waiting, so I think this is okay:
https://en.cppreference.com/cpp/thread/condition_variable/wait
wait causes the current thread to block until the condition variable is notified or a spurious wakeup occurs. pred can be optionally provided to detect spurious wakeup.
- Atomically calls lock.unlock() and blocks on *this.
The thread will be unblocked when notify_all() or notify_one() is executed. It may also be unblocked spuriously.
When unblocked, calls lock.lock() (possibly blocking on the lock), then returns.
| // Another worker is producing for this UID, wait for it. | ||
| if (auto It = InProgressByUID.find(UID); It != InProgressByUID.end()) { | ||
| std::shared_ptr<InProgressEntry> Pending = It->second; | ||
| Pending->CondVar.wait(LockGuard, [&] { return Pending->Done; }); |
| } | ||
| IPE->Result = Result; | ||
| IPE->Done = true; | ||
| InProgressByFilename.erase(Filename); |
Contributor
There was a problem hiding this comment.
Ah, you erase from the new map. That alleviates one of my concerns with the map duplication (memory usage due to long strings as keys), but I think we can do better still by avoiding the temporary entry & path allocation in the InProgressByFilename by merging it with CacheByFilename. We'd have a pair of const CachedFileSystemEntry * and std::shared_ptr<InProgressEntry> as the value, and only erase the InProgressEntry part of the entry here.
Contributor
Author
There was a problem hiding this comment.
I folded the new maps into the existing ones, which does make for a much tidier change. Thank you.
| } | ||
| IPE->Result = Result; | ||
| IPE->Done = true; | ||
| InProgressByUID.erase(UID); |
|
|
||
| if (const auto *Entry = | ||
| findEntryByFilenameWithWriteThrough(*FilenameForLookup)) | ||
| if (const auto *Entry = LocalCache.findEntryByFilename(*FilenameForLookup)) |
Contributor
There was a problem hiding this comment.
This seems to have dropped the semantics of writing-through the result from the shared cache to the local cache. But IIUC computeAndStoreResult now takes care of it. I think we should either rename that function or refactor the code such that it's clear what's happening here.
Contributor
Author
There was a problem hiding this comment.
Renamed the method.
| /// VFS proxy that counts operations using atomic counters, suitable for use | ||
| /// from concurrent tests where `llvm::vfs::TracingFileSystem`'s plain | ||
| /// `std::size_t` counters would race. | ||
| class AtomicTracingFileSystem : public llvm::vfs::ProxyFileSystem { |
Contributor
There was a problem hiding this comment.
Could we just make the existing llvm::vfs::TracingFileSystem a template that can be instantiated to either use plain or atomic integers as counters?
Contributor
Author
There was a problem hiding this comment.
Done in a separate commit.
| } | ||
|
|
||
| TEST(DependencyScanningWorkerFilesystem, ConcurrentSameFilenameDeduplicates) { | ||
| auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>(); |
Contributor
There was a problem hiding this comment.
The InMemoryFileSystem is not thread-safe, so if we were to introduce a bug in the caching VFS (or run this test with the old implementation), instead of seeing EXPECT_ failures, we'd see crashes and other weirdness. In this case, I think I'd prefer if we created a temporary on-disk file and the real VFS.
Contributor
Author
There was a problem hiding this comment.
Rewrote to use temporary on-disk files.
Templating `TracingFileSystem` on the counter type lets the same proxy serve single-threaded use (with `std::size_t` counters) and concurrent use (with `std::atomic<std::size_t>`) without duplicating the class. Introduce `TracingFileSystemImpl<CounterT>` as the underlying class template and provide two aliases: using TracingFileSystem = TracingFileSystemImpl<std::size_t>; using AtomicTracingFileSystem = TracingFileSystemImpl<std::atomic<std::size_t>>; Existing callers continue to use `TracingFileSystem` unchanged. A new `TracingFileSystemTest.AtomicCountersUnderConcurrency` test exercises the atomic instantiation with eight threads.
artemcm
force-pushed
the
artemcm/CacheDepScanFileSystemInProgressQuery
branch
from
8bc818a to
04f6767
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
artemcm
force-pushed
the
artemcm/CacheDepScanFileSystemInProgressQuery
branch
from
04f6767 to
f040b20
Compare
…ystemSharedCache` The shared cache deduplicates results only after the underlying filesystem trip completes, so concurrent workers querying the same filename or UID each pay their own `stat` and `open`. Track in-flight queries per key: the first arrival produces the result, late arrivals wait on a condition variable and adopt it. `CacheByFilename`'s value gains a `std::shared_ptr<InProgressEntry>` field alongside the resolved entry and real path; `EntriesByUID` does the same. The in-progress entry is populated for the duration of a producer's filesystem trip and reset on publish. Four new shard methods manage the slot lifecycle: - `acquireFilenameSlot` / `acquireUIDSlot` collapse three outcomes (resolved hit, in-progress wait, fresh producer) into one critical section. Waiters block on the slot's condition variable, which atomically releases the shard lock for the duration of the wait. - `fulfilFilenameSlot` / `fulfilUIDSlot` publish the produced entry, set `Done`, clear the slot, and `notify_all` waiters outside the lock.
artemcm
force-pushed
the
artemcm/CacheDepScanFileSystemInProgressQuery
branch
from
f040b20 to
1ef36f6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Concurrent dep-scan workers querying the same filename or UID each issue their own
statandopenagainst the underlying filesystem; only the first to finish wins the cache insert, the others' work is wasted.Add per-key in-flight tracking to
CacheShard.InProgressByFilenameandInProgressByUIDmap each active key to astd::shared_ptr<InProgressEntry>carrying astd::condition_variable, aDonepredicate, and the produced entry pointer.acquireFilenameSlot/acquireUIDSlotcollapse three outcomes (resolved hit, in-progress wait, fresh producer slot) into one critical section against the shard lock.fulfilFilenameSlot,fulfilUIDSlotpublish the produced entry, setDone, erase the slot, andnotify_allwaiters outside the lock.computeAndStoreResultnow claims the filename slot beforestatand the UID slot beforereadFile, so both stat and open redundancy collapse.