[clang][modules][deps] Add mutex as an alternative to file lock #129751
+247
−25
Conversation
This file contains 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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) ChangesThe dependency scanner uses implicitly-built Clang modules under the hood. This system was originally designed to handle multiple concurrent processes working on the same module cache, and mutual exclusion was implemented using file locks. The scanner, however, runs within single process, making file locks unnecessary. This patch virtualizes the interface for module cache locking and provides an implementation based on Note that even when multiple processes run a scan on the same module cache (and therefore don't coordinate efficiently), this should still be correct due to the strict context hash, the write-through Full diff: https://github.com/llvm/llvm-project/pull/129751.diff 10 Files Affected:
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index 8b539dfc92960..bede160991443 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -53,6 +53,7 @@ class FileManager;
class FrontendAction;
class InMemoryModuleCache;
class Module;
+class ModuleCacheLock;
class Preprocessor;
class Sema;
class SourceManager;
@@ -96,9 +97,12 @@ class CompilerInstance : public ModuleLoader {
/// The source manager.
IntrusiveRefCntPtr<SourceManager> SourceMgr;
- /// The cache of PCM files.
+ /// The local in-memory cache of PCM files.
IntrusiveRefCntPtr<InMemoryModuleCache> ModuleCache;
+ /// The lock managing the global cache of PCM files.
+ std::shared_ptr<ModuleCacheLock> ModuleCacheLck;
+
/// The preprocessor.
std::shared_ptr<Preprocessor> PP;
@@ -209,7 +213,8 @@ class CompilerInstance : public ModuleLoader {
explicit CompilerInstance(
std::shared_ptr<PCHContainerOperations> PCHContainerOps =
std::make_shared<PCHContainerOperations>(),
- InMemoryModuleCache *SharedModuleCache = nullptr);
+ InMemoryModuleCache *SharedModuleCache = nullptr,
+ std::shared_ptr<ModuleCacheLock> ModuleCacheLck = nullptr);
~CompilerInstance() override;
/// @name High-Level Operations
@@ -897,6 +902,10 @@ class CompilerInstance : public ModuleLoader {
void setExternalSemaSource(IntrusiveRefCntPtr<ExternalSemaSource> ESS);
InMemoryModuleCache &getModuleCache() const { return *ModuleCache; }
+
+ std::shared_ptr<ModuleCacheLock> getModuleCacheLockPtr() const {
+ return ModuleCacheLck;
+ }
};
} // end namespace clang
diff --git a/clang/include/clang/Serialization/ModuleCacheLock.h b/clang/include/clang/Serialization/ModuleCacheLock.h
new file mode 100644
index 0000000000000..9435735b1a1bd
--- /dev/null
+++ b/clang/include/clang/Serialization/ModuleCacheLock.h
@@ -0,0 +1,31 @@
+#ifndef LLVM_CLANG_SERIALIZATION_MODULECACHELOCK_H
+#define LLVM_CLANG_SERIALIZATION_MODULECACHELOCK_H
+
+#include "clang/Basic/LLVM.h"
+#include "llvm/Support/LockFileManager.h"
+
+namespace clang {
+enum class LockResult { Owned, Shared, Error };
+enum class WaitForUnlockResult { Success, OwnerDied, Timeout };
+
+class ModuleCacheLockManager {
+public:
+ virtual operator LockResult() const = 0;
+ virtual WaitForUnlockResult waitForUnlock() = 0;
+ virtual void unsafeRemoveLock() = 0;
+ virtual std::string getErrorMessage() const = 0;
+ virtual ~ModuleCacheLockManager() = default;
+};
+
+class ModuleCacheLock {
+public:
+ virtual void prepareLock(StringRef ModuleFilename) = 0;
+ virtual std::unique_ptr<ModuleCacheLockManager>
+ tryLock(StringRef ModuleFilename) = 0;
+ virtual ~ModuleCacheLock() = default;
+};
+
+std::shared_ptr<ModuleCacheLock> getModuleCacheFileLock();
+} // namespace clang
+
+#endif
diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
index f002f8645d3f6..f6914b90ac67e 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
@@ -9,6 +9,7 @@
#ifndef LLVM_CLANG_TOOLING_DEPENDENCYSCANNING_DEPENDENCYSCANNINGSERVICE_H
#define LLVM_CLANG_TOOLING_DEPENDENCYSCANNING_DEPENDENCYSCANNINGSERVICE_H
+#include "clang/Tooling/DependencyScanning/ModuleCacheMutexLock.h"
#include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
#include "llvm/ADT/BitmaskEnum.h"
@@ -95,6 +96,10 @@ class DependencyScanningService {
return SharedCache;
}
+ ModuleCacheMutexes &getSharedModuleCacheMutexes() {
+ return ModuleCacheMutexes;
+ }
+
private:
const ScanningMode Mode;
const ScanningOutputFormat Format;
@@ -106,6 +111,8 @@ class DependencyScanningService {
const bool TraceVFS;
/// The global file system cache.
DependencyScanningFilesystemSharedCache SharedCache;
+ /// The global module cache mutexes.
+ ModuleCacheMutexes ModuleCacheMutexes;
};
} // end namespace dependencies
diff --git a/clang/include/clang/Tooling/DependencyScanning/ModuleCacheMutexLock.h b/clang/include/clang/Tooling/DependencyScanning/ModuleCacheMutexLock.h
new file mode 100644
index 0000000000000..312972b26702c
--- /dev/null
+++ b/clang/include/clang/Tooling/DependencyScanning/ModuleCacheMutexLock.h
@@ -0,0 +1,31 @@
+#ifndef LLVM_CLANG_TOOLING_DEPENDENCYSCANNING_MODULECACHEMUTEXLOCK_H
+#define LLVM_CLANG_TOOLING_DEPENDENCYSCANNING_MODULECACHEMUTEXLOCK_H
+
+#include "clang/Serialization/ModuleCacheLock.h"
+#include "llvm/ADT/StringMap.h"
+
+#include <condition_variable>
+
+namespace clang {
+namespace tooling {
+namespace dependencies {
+struct ModuleCacheMutexWrapper {
+ std::mutex Mutex;
+ std::condition_variable CondVar;
+ bool Done = false;
+
+ ModuleCacheMutexWrapper() = default;
+};
+
+struct ModuleCacheMutexes {
+ std::mutex Mutex;
+ llvm::StringMap<std::shared_ptr<ModuleCacheMutexWrapper>> Map;
+};
+
+std::shared_ptr<ModuleCacheLock>
+getModuleCacheMutexLock(ModuleCacheMutexes &Mutexes);
+} // namespace dependencies
+} // namespace tooling
+} // namespace clang
+
+#endif
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index c11c857ea0606..3d02d42032e21 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -39,6 +39,7 @@
#include "clang/Serialization/ASTReader.h"
#include "clang/Serialization/GlobalModuleIndex.h"
#include "clang/Serialization/InMemoryModuleCache.h"
+#include "clang/Serialization/ModuleCacheLock.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
@@ -66,11 +67,14 @@ using namespace clang;
CompilerInstance::CompilerInstance(
std::shared_ptr<PCHContainerOperations> PCHContainerOps,
- InMemoryModuleCache *SharedModuleCache)
+ InMemoryModuleCache *SharedModuleCache,
+ std::shared_ptr<ModuleCacheLock> ModuleCacheLck)
: ModuleLoader(/* BuildingModule = */ SharedModuleCache),
Invocation(new CompilerInvocation()),
ModuleCache(SharedModuleCache ? SharedModuleCache
: new InMemoryModuleCache),
+ ModuleCacheLck(ModuleCacheLck ? ModuleCacheLck
+ : getModuleCacheFileLock()),
ThePCHContainerOperations(std::move(PCHContainerOps)) {}
CompilerInstance::~CompilerInstance() {
@@ -1227,7 +1231,8 @@ compileModuleImpl(CompilerInstance &ImportingInstance, SourceLocation ImportLoc,
// CompilerInstance::CompilerInstance is responsible for finalizing the
// buffers to prevent use-after-frees.
CompilerInstance Instance(ImportingInstance.getPCHContainerOperations(),
- &ImportingInstance.getModuleCache());
+ &ImportingInstance.getModuleCache(),
+ ImportingInstance.getModuleCacheLockPtr());
auto &Inv = *Invocation;
Instance.setInvocation(std::move(Invocation));
@@ -1471,47 +1476,45 @@ static bool compileModuleAndReadASTBehindLock(
Diags.Report(ModuleNameLoc, diag::remark_module_lock)
<< ModuleFileName << Module->Name;
- // FIXME: have LockFileManager return an error_code so that we can
- // avoid the mkdir when the directory already exists.
- StringRef Dir = llvm::sys::path::parent_path(ModuleFileName);
- llvm::sys::fs::create_directories(Dir);
+ auto Lock = ImportingInstance.getModuleCacheLockPtr();
+ Lock->prepareLock(ModuleFileName);
while (true) {
- llvm::LockFileManager Locked(ModuleFileName);
- switch (Locked) {
- case llvm::LockFileManager::LFS_Error:
+ auto Locked = Lock->tryLock(ModuleFileName);
+ switch (*Locked) {
+ case LockResult::Error:
// ModuleCache takes care of correctness and locks are only necessary for
// performance. Fallback to building the module in case of any lock
// related errors.
Diags.Report(ModuleNameLoc, diag::remark_module_lock_failure)
- << Module->Name << Locked.getErrorMessage();
+ << Module->Name << Locked->getErrorMessage();
// Clear out any potential leftover.
- Locked.unsafeRemoveLockFile();
+ Locked->unsafeRemoveLock();
[[fallthrough]];
- case llvm::LockFileManager::LFS_Owned:
+ case LockResult::Owned:
// We're responsible for building the module ourselves.
return compileModuleAndReadASTImpl(ImportingInstance, ImportLoc,
ModuleNameLoc, Module, ModuleFileName);
- case llvm::LockFileManager::LFS_Shared:
+ case LockResult::Shared:
break; // The interesting case.
}
// Someone else is responsible for building the module. Wait for them to
// finish.
- switch (Locked.waitForUnlock()) {
- case llvm::LockFileManager::Res_Success:
+ switch (Locked->waitForUnlock()) {
+ case WaitForUnlockResult::Success:
break; // The interesting case.
- case llvm::LockFileManager::Res_OwnerDied:
+ case WaitForUnlockResult::OwnerDied:
continue; // try again to get the lock.
- case llvm::LockFileManager::Res_Timeout:
- // Since ModuleCache takes care of correctness, we try waiting for
- // another process to complete the build so clang does not do it done
- // twice. If case of timeout, build it ourselves.
+ case WaitForUnlockResult::Timeout:
+ // Since the InMemoryModuleCache takes care of correctness, we try waiting
+ // for someone else to complete the build so that it does not happen
+ // twice. In case of timeout, build it ourselves.
Diags.Report(ModuleNameLoc, diag::remark_module_lock_timeout)
<< Module->Name;
- // Clear the lock file so that future invocations can make progress.
- Locked.unsafeRemoveLockFile();
+ // Remove the lock so that future invocations can make progress.
+ Locked->unsafeRemoveLock();
continue;
}
diff --git a/clang/lib/Serialization/CMakeLists.txt b/clang/lib/Serialization/CMakeLists.txt
index b1fc0345047f2..acbd8669caaed 100644
--- a/clang/lib/Serialization/CMakeLists.txt
+++ b/clang/lib/Serialization/CMakeLists.txt
@@ -18,6 +18,7 @@ add_clang_library(clangSerialization
GeneratePCH.cpp
GlobalModuleIndex.cpp
InMemoryModuleCache.cpp
+ ModuleCacheLock.cpp
ModuleFile.cpp
ModuleFileExtension.cpp
ModuleManager.cpp
diff --git a/clang/lib/Serialization/ModuleCacheLock.cpp b/clang/lib/Serialization/ModuleCacheLock.cpp
new file mode 100644
index 0000000000000..0c59ed7f28b98
--- /dev/null
+++ b/clang/lib/Serialization/ModuleCacheLock.cpp
@@ -0,0 +1,60 @@
+#include "clang/Serialization/ModuleCacheLock.h"
+
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+using namespace clang;
+
+namespace {
+struct ModuleCacheFileLockManager : ModuleCacheLockManager {
+ llvm::LockFileManager Lock;
+
+ ModuleCacheFileLockManager(StringRef ModuleFilename) : Lock(ModuleFilename) {}
+
+ operator LockResult() const override {
+ switch (Lock) {
+ case llvm::LockFileManager::LFS_Owned:
+ return LockResult::Owned;
+ case llvm::LockFileManager::LFS_Shared:
+ return LockResult::Shared;
+ case llvm::LockFileManager::LFS_Error:
+ return LockResult::Error;
+ }
+ }
+
+ WaitForUnlockResult waitForUnlock() override {
+ switch (Lock.waitForUnlock()) {
+ case llvm::LockFileManager::Res_Success:
+ return WaitForUnlockResult::Success;
+ case llvm::LockFileManager::Res_OwnerDied:
+ return WaitForUnlockResult::OwnerDied;
+ case llvm::LockFileManager::Res_Timeout:
+ return WaitForUnlockResult::Timeout;
+ }
+ }
+
+ void unsafeRemoveLock() override { Lock.unsafeRemoveLockFile(); }
+
+ std::string getErrorMessage() const override {
+ return Lock.getErrorMessage();
+ }
+};
+
+struct ModuleCacheFileLock : ModuleCacheLock {
+ void prepareLock(StringRef ModuleFilename) override {
+ // FIXME: have LockFileManager return an error_code so that we can
+ // avoid the mkdir when the directory already exists.
+ StringRef Dir = llvm::sys::path::parent_path(ModuleFilename);
+ llvm::sys::fs::create_directories(Dir);
+ }
+
+ std::unique_ptr<ModuleCacheLockManager>
+ tryLock(StringRef ModuleFilename) override {
+ return std::make_unique<ModuleCacheFileLockManager>(ModuleFilename);
+ }
+};
+} // namespace
+
+std::shared_ptr<ModuleCacheLock> clang::getModuleCacheFileLock() {
+ return std::make_unique<ModuleCacheFileLock>();
+}
diff --git a/clang/lib/Tooling/DependencyScanning/CMakeLists.txt b/clang/lib/Tooling/DependencyScanning/CMakeLists.txt
index 66795b0be0baa..7ae35e69d7000 100644
--- a/clang/lib/Tooling/DependencyScanning/CMakeLists.txt
+++ b/clang/lib/Tooling/DependencyScanning/CMakeLists.txt
@@ -11,6 +11,7 @@ add_clang_library(clangDependencyScanning
DependencyScanningService.cpp
DependencyScanningWorker.cpp
DependencyScanningTool.cpp
+ ModuleCacheMutexLock.cpp
ModuleDepCollector.cpp
DEPENDS
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 697f26ee5d12f..fdc86d1764eea 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -22,6 +22,7 @@
#include "clang/Lex/PreprocessorOptions.h"
#include "clang/Serialization/ObjectFilePCHContainerReader.h"
#include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
+#include "clang/Tooling/DependencyScanning/ModuleCacheMutexLock.h"
#include "clang/Tooling/DependencyScanning/ModuleDepCollector.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
@@ -315,7 +316,10 @@ class DependencyScanningAction : public tooling::ToolAction {
Scanned = true;
// Create a compiler instance to handle the actual work.
- ScanInstanceStorage.emplace(std::move(PCHContainerOps));
+ ModuleCacheLck =
+ getModuleCacheMutexLock(Service.getSharedModuleCacheMutexes());
+ ScanInstanceStorage.emplace(std::move(PCHContainerOps), nullptr,
+ ModuleCacheLck);
CompilerInstance &ScanInstance = *ScanInstanceStorage;
ScanInstance.setInvocation(std::move(Invocation));
@@ -479,6 +483,7 @@ class DependencyScanningAction : public tooling::ToolAction {
llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS;
bool DisableFree;
std::optional<StringRef> ModuleName;
+ std::shared_ptr<ModuleCacheLock> ModuleCacheLck;
std::optional<CompilerInstance> ScanInstanceStorage;
std::shared_ptr<ModuleDepCollector> MDC;
std::vector<std::string> LastCC1Arguments;
diff --git a/clang/lib/Tooling/DependencyScanning/ModuleCacheMutexLock.cpp b/clang/lib/Tooling/DependencyScanning/ModuleCacheMutexLock.cpp
new file mode 100644
index 0000000000000..162b5a4b213f5
--- /dev/null
+++ b/clang/lib/Tooling/DependencyScanning/ModuleCacheMutexLock.cpp
@@ -0,0 +1,81 @@
+#include "clang/Tooling/DependencyScanning/ModuleCacheMutexLock.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace dependencies;
+
+namespace {
+struct ModuleCacheMutexLockManager : ModuleCacheLockManager {
+ ModuleCacheMutexes &Mutexes;
+ StringRef Filename;
+
+ std::shared_ptr<ModuleCacheMutexWrapper> MutexWrapper;
+ bool Owning;
+
+ ModuleCacheMutexLockManager(ModuleCacheMutexes &Mutexes, StringRef Filename)
+ : Mutexes(Mutexes), Filename(Filename) {
+ Owning = false;
+ {
+ std::lock_guard Lock(Mutexes.Mutex);
+ auto &MutexWrapperInMap = Mutexes.Map[Filename];
+ if (!MutexWrapperInMap) {
+ MutexWrapperInMap = std::make_shared<ModuleCacheMutexWrapper>();
+ Owning = true;
+ }
+ // Increment the reference count of the mutex here in the critical section
+ // to guarantee that it's kept alive even when another thread removes it
+ // via \c unsafeRemoveLock().
+ MutexWrapper = MutexWrapperInMap;
+ }
+
+ if (Owning)
+ MutexWrapper->Mutex.lock();
+ }
+
+ ~ModuleCacheMutexLockManager() override {
+ if (Owning) {
+ MutexWrapper->Done = true;
+ MutexWrapper->CondVar.notify_all();
+ MutexWrapper->Mutex.unlock();
+ }
+ }
+
+ operator LockResult() const override {
+ return Owning ? LockResult::Owned : LockResult::Shared;
+ }
+
+ WaitForUnlockResult waitForUnlock() override {
+ assert(!Owning);
+ std::unique_lock Lock(MutexWrapper->Mutex);
+ bool Done = MutexWrapper->CondVar.wait_for(
+ Lock, std::chrono::seconds(90), [&] { return MutexWrapper->Done; });
+ return Done ? WaitForUnlockResult::Success : WaitForUnlockResult::Timeout;
+ }
+
+ void unsafeRemoveLock() override {
+ std::lock_guard Lock(Mutexes.Mutex);
+ Mutexes.Map[Filename].reset();
+ }
+
+ std::string getErrorMessage() const override {
+ llvm_unreachable("ModuleCacheMutexLockManager cannot fail");
+ }
+};
+
+struct ModuleCacheMutexLock : ModuleCacheLock {
+ ModuleCacheMutexes &Mutexes;
+
+ ModuleCacheMutexLock(ModuleCacheMutexes &Mutexes) : Mutexes(Mutexes) {}
+
+ void prepareLock(StringRef Filename) override {}
+
+ std::unique_ptr<ModuleCacheLockManager> tryLock(StringRef Filename) override {
+ return std::make_unique<ModuleCacheMutexLockManager>(Mutexes, Filename);
+ }
+};
+} // namespace
+
+std::shared_ptr<ModuleCacheLock>
+dependencies::getModuleCacheMutexLock(ModuleCacheMutexes &Mutexes) {
+ return std::make_shared<ModuleCacheMutexLock>(Mutexes);
+}
|
jansvoboda11
commented
Mar 4, 2025
Comment on lines
+8
to
+9
| enum class LockResult { Owned, Shared, Error }; | ||
| enum class WaitForUnlockResult { Success, OwnerDied, Timeout }; |
There was a problem hiding this comment.
Not sure if this is better than just using the LockFileManager enums... Any thoughts?
jansvoboda11
commented
Mar 4, 2025
Comment on lines
+11
to
+26
| class ModuleCacheLockManager { | ||
| public: | ||
| virtual operator LockResult() const = 0; | ||
| virtual WaitForUnlockResult waitForUnlock() = 0; | ||
| virtual void unsafeRemoveLock() = 0; | ||
| virtual std::string getErrorMessage() const = 0; | ||
| virtual ~ModuleCacheLockManager() = default; | ||
| }; | ||
|
|
||
| class ModuleCacheLock { | ||
| public: | ||
| virtual void prepareLock(StringRef ModuleFilename) = 0; | ||
| virtual std::unique_ptr<ModuleCacheLockManager> | ||
| tryLock(StringRef ModuleFilename) = 0; | ||
| virtual ~ModuleCacheLock() = default; | ||
| }; |
There was a problem hiding this comment.
Not sure if it'd be neater to add these interfaces into LLVMSupport and have LockFileManager implement them right then and there. Thoughts?
There was a problem hiding this comment.
Hmm, ModuleCacheLockManager is basically the same interface, and the enums are the same. I do think it makes sense to have that part in llvm/Support. I think the ModuleCacheLock should stay in Clang though.
It would also be nice to modernize the interface (particularly std::string getErrorMessage()), but I think that can happen separately.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jansvoboda11
commented
Mar 4, 2025
| @@ -95,6 +96,10 @@ class DependencyScanningService { | |||
| return SharedCache; | |||
| } | |||
|
|
|||
| ModuleCacheMutexes &getSharedModuleCacheMutexes() { | |||
There was a problem hiding this comment.
Not sure whether the service should hand out ModuleCacheMutexes & (the same thing we do for the shared caching filesystem) or whether it'd be simpler to just hand out ready thread-safe std::shared_ptr<ModuleCacheLock>. Thoughts?
There was a problem hiding this comment.
I don't think we need shared_ptr here, the lifetime of workers is already bound to the service.
There was a problem hiding this comment.
We don't need it, but we currently call std::make_shared<ModuleCacheMutexLock> for each TU, which is unnecessary. We could just do that on service construction and avoid future allocations.
Bigcheese
reviewed
Mar 6, 2025
Comment on lines
+19
to
+34
| class ModuleCacheLockManager { | ||
| public: | ||
| virtual operator LockResult() const = 0; | ||
| virtual WaitForUnlockResult waitForUnlock() = 0; | ||
| virtual void unsafeRemoveLock() = 0; | ||
| virtual std::string getErrorMessage() const = 0; | ||
| virtual ~ModuleCacheLockManager() = default; | ||
| }; | ||
|
|
||
| class ModuleCacheLock { | ||
| public: | ||
| virtual void prepareLock(StringRef ModuleFilename) = 0; | ||
| virtual std::unique_ptr<ModuleCacheLockManager> | ||
| tryLock(StringRef ModuleFilename) = 0; | ||
| virtual ~ModuleCacheLock() = default; | ||
| }; |
There was a problem hiding this comment.
These two names are a bit confusing, and seem reversed. I would expect a LockManager to handle multiple locks, and just Lock to be the actual lock.
Comment on lines
+11
to
+26
| class ModuleCacheLockManager { | ||
| public: | ||
| virtual operator LockResult() const = 0; | ||
| virtual WaitForUnlockResult waitForUnlock() = 0; | ||
| virtual void unsafeRemoveLock() = 0; | ||
| virtual std::string getErrorMessage() const = 0; | ||
| virtual ~ModuleCacheLockManager() = default; | ||
| }; | ||
|
|
||
| class ModuleCacheLock { | ||
| public: | ||
| virtual void prepareLock(StringRef ModuleFilename) = 0; | ||
| virtual std::unique_ptr<ModuleCacheLockManager> | ||
| tryLock(StringRef ModuleFilename) = 0; | ||
| virtual ~ModuleCacheLock() = default; | ||
| }; |
There was a problem hiding this comment.
Hmm, ModuleCacheLockManager is basically the same interface, and the enums are the same. I do think it makes sense to have that part in llvm/Support. I think the ModuleCacheLock should stay in Clang though.
It would also be nice to modernize the interface (particularly std::string getErrorMessage()), but I think that can happen separately.
| class ModuleCacheLockManager { | ||
| public: | ||
| virtual operator LockResult() const = 0; | ||
| virtual WaitForUnlockResult waitForUnlock() = 0; |
There was a problem hiding this comment.
Looking more at how this is structured I think this should still have a timeout because of the LockFile implementation. And then it doesn't really hurt to actually do the timeout for the mutex impl, even though actually hitting that has other issues.
| @@ -95,6 +96,10 @@ class DependencyScanningService { | |||
| return SharedCache; | |||
| } | |||
|
|
|||
| ModuleCacheMutexes &getSharedModuleCacheMutexes() { | |||
There was a problem hiding this comment.
I don't think we need shared_ptr here, the lifetime of workers is already bound to the service.
Comment on lines
+51
to
+57
| auto &Mutex = [&]() -> std::shared_mutex & { | ||
| std::lock_guard Lock(Mutexes.Mutex); | ||
| auto &Mutex = Mutexes.Map[Filename]; | ||
| if (!Mutex) | ||
| Mutex = std::make_unique<std::shared_mutex>(); | ||
| return *Mutex; | ||
| }(); |
There was a problem hiding this comment.
I don't think you need a lambda here, the only difference in lifetime without it is that the return value will be constructed before unlock.
There was a problem hiding this comment.
Yeah, that's the reason I did this - I wanted to avoid an unnecessary allocation in the critical section.
|
|
||
| namespace { | ||
| struct ModuleCacheMutexLockManager : ModuleCacheLockManager { | ||
| std::unique_lock<std::shared_mutex> OwningLock; |
There was a problem hiding this comment.
Can you add a comment here saying something like:
TODO: Consider using std::atomic::{wait, notify_all} when LLVM moves to C++20.
This is the actual operation we want to use, which lowers to futex/ulock.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The dependency scanner uses implicitly-built Clang modules under the hood. This system was originally designed to handle multiple concurrent processes working on the same module cache, and mutual exclusion was implemented using file locks. The scanner, however, runs within single process, making file locks unnecessary. This patch virtualizes the interface for module cache locking and provides an implementation based on
std::shared_mutex. This reducesclang-scan-depsruntime by ~17% on my benchmark.Note that even when multiple processes run a scan on the same module cache (and therefore don't coordinate efficiently), this should still be correct due to the strict context hash, the write-through
InMemoryModuleCacheand the logic for rebuilding out-of-date or incompatible modules.