[clangd] [C++20] [Modules] Add scanning cache #125988
Merged
Conversation
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
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Chuanqi Xu (ChuanqiXu9) ChangesPreviously, everytime we want to get a source file declaring a specific module, we need to scan the whole projects again and again. The performance is super bad. This patch tries to improve this by introducing a simple cache. Full diff: https://github.com/llvm/llvm-project/pull/125988.diff 6 Files Affected:
diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt
index 6f10afe4a5625f5..a0ff8483991bb09 100644
--- a/clang-tools-extra/clangd/CMakeLists.txt
+++ b/clang-tools-extra/clangd/CMakeLists.txt
@@ -99,6 +99,7 @@ add_clang_library(clangDaemon STATIC
ModulesBuilder.cpp
PathMapping.cpp
Protocol.cpp
+ ProjectModulesCache.cpp
Quality.cpp
ParsedAST.cpp
Preamble.cpp
diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index bee31fe51555e02..9e8e66100aeca94 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -361,7 +361,8 @@ void ModuleFileCache::remove(StringRef ModuleName) {
/// ModuleName in topological order. The \param ModuleName is guaranteed to
/// be the last element in \param ModuleNames.
llvm::SmallVector<StringRef> getAllRequiredModules(ProjectModules &MDB,
- StringRef ModuleName) {
+ StringRef ModuleName,
+ ProjectModulesCache &Cache) {
llvm::SmallVector<llvm::StringRef> ModuleNames;
llvm::StringSet<> ModuleNamesSet;
@@ -369,7 +370,7 @@ llvm::SmallVector<StringRef> getAllRequiredModules(ProjectModules &MDB,
ModuleNamesSet.insert(ModuleName);
for (StringRef RequiredModuleName :
- MDB.getRequiredModules(MDB.getSourceForModuleName(ModuleName)))
+ MDB.getRequiredModules(MDB.getSourceForModuleName(Cache, ModuleName)))
if (ModuleNamesSet.insert(RequiredModuleName).second)
Visitor(RequiredModuleName, Visitor);
@@ -384,7 +385,9 @@ llvm::SmallVector<StringRef> getAllRequiredModules(ProjectModules &MDB,
class ModulesBuilder::ModulesBuilderImpl {
public:
- ModulesBuilderImpl(const GlobalCompilationDatabase &CDB) : Cache(CDB) {}
+ ModulesBuilderImpl(const GlobalCompilationDatabase &CDB) : Cache(CDB) {
+ MDBCache = createProjectModulesCache();
+ }
const GlobalCompilationDatabase &getCDB() const { return Cache.getCDB(); }
@@ -395,6 +398,7 @@ class ModulesBuilder::ModulesBuilderImpl {
private:
ModuleFileCache Cache;
+ std::unique_ptr<ProjectModulesCache> MDBCache;
};
llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
@@ -403,7 +407,8 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
if (BuiltModuleFiles.isModuleUnitBuilt(ModuleName))
return llvm::Error::success();
- PathRef ModuleUnitFileName = MDB.getSourceForModuleName(ModuleName);
+ PathRef ModuleUnitFileName =
+ MDB.getSourceForModuleName(*MDBCache, ModuleName);
/// It is possible that we're meeting third party modules (modules whose
/// source are not in the project. e.g, the std module may be a third-party
/// module for most project) or something wrong with the implementation of
@@ -416,7 +421,7 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
llvm::formatv("Don't get the module unit for module {0}", ModuleName));
// Get Required modules in topological order.
- auto ReqModuleNames = getAllRequiredModules(MDB, ModuleName);
+ auto ReqModuleNames = getAllRequiredModules(MDB, ModuleName, *MDBCache);
for (llvm::StringRef ReqModuleName : ReqModuleNames) {
if (BuiltModuleFiles.isModuleUnitBuilt(ModuleName))
continue;
diff --git a/clang-tools-extra/clangd/ProjectModules.h b/clang-tools-extra/clangd/ProjectModules.h
index 48d52ac9deb893f..cdf54067cf0b9a7 100644
--- a/clang-tools-extra/clangd/ProjectModules.h
+++ b/clang-tools-extra/clangd/ProjectModules.h
@@ -9,6 +9,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROJECTMODULES_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROJECTMODULES_H
+#include "ProjectModulesCache.h"
#include "support/Function.h"
#include "support/Path.h"
#include "support/ThreadsafeFS.h"
@@ -43,7 +44,7 @@ class ProjectModules {
virtual std::vector<std::string> getRequiredModules(PathRef File) = 0;
virtual PathRef
- getSourceForModuleName(llvm::StringRef ModuleName,
+ getSourceForModuleName(ProjectModulesCache &Cache, llvm::StringRef ModuleName,
PathRef RequiredSrcFile = PathRef()) = 0;
virtual void setCommandMangler(CommandMangler Mangler) {}
diff --git a/clang-tools-extra/clangd/ProjectModulesCache.cpp b/clang-tools-extra/clangd/ProjectModulesCache.cpp
new file mode 100644
index 000000000000000..9eb0723a7a13ede
--- /dev/null
+++ b/clang-tools-extra/clangd/ProjectModulesCache.cpp
@@ -0,0 +1,56 @@
+//===------------------ ProjectModulesCache.cpp ------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ProjectModulesCache.h"
+#include "llvm/ADT/StringMap.h"
+#include <mutex>
+
+namespace clang::clangd {
+namespace {
+class SharedProjectModulesCache : public ProjectModulesCache {
+public:
+ std::optional<std::string>
+ getSourceForModuleName(llvm::StringRef ModuleName,
+ PathRef RequiredSrcFile = PathRef()) override {
+ std::lock_guard<std::mutex> Lock(Mutex);
+
+ auto Iter = ModuleNameToSource.find(ModuleName);
+ if (Iter == ModuleNameToSource.end())
+ return std::nullopt;
+
+ return Iter->second;
+ }
+
+ void clearEntry(llvm::StringRef ModuleName,
+ PathRef RequiredSrcFile = PathRef()) override {
+ std::lock_guard<std::mutex> Lock(Mutex);
+
+ auto Iter = ModuleNameToSource.find(ModuleName);
+ if (Iter == ModuleNameToSource.end())
+ return;
+
+ ModuleNameToSource.erase(Iter);
+ }
+
+ void setEntry(PathRef FilePath, llvm::StringRef ModuleName) override {
+ std::lock_guard<std::mutex> Lock(Mutex);
+
+ ModuleNameToSource[ModuleName] = FilePath;
+ }
+
+private:
+ std::mutex Mutex;
+ llvm::StringMap<std::string> ModuleNameToSource;
+};
+} // namespace
+
+std::unique_ptr<ProjectModulesCache> createProjectModulesCache() {
+ return std::make_unique<SharedProjectModulesCache>();
+}
+
+} // namespace clang::clangd
diff --git a/clang-tools-extra/clangd/ProjectModulesCache.h b/clang-tools-extra/clangd/ProjectModulesCache.h
new file mode 100644
index 000000000000000..8aee77119a80cbe
--- /dev/null
+++ b/clang-tools-extra/clangd/ProjectModulesCache.h
@@ -0,0 +1,46 @@
+//===------------------ ProjectModulesCache.h --------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROJECTMODULESCACHE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROJECTMODULESCACHE_H
+
+#include "support/Path.h"
+#include <memory>
+
+namespace clang {
+namespace clangd {
+
+/// A cache for the module name to file name map in the project.
+/// The rationale is:
+/// (1) It is fast to get the module name to a file.
+/// (2) It may be slow to get a file with a specified module name.
+/// (3) The module name of files may not change drastically and frequently.
+///
+/// The cache itself is not responsible for the validness of cached result.
+/// Users of the cache should check it after getting the result and updating
+/// the cache if the result is invalid.
+class ProjectModulesCache {
+public:
+ virtual ~ProjectModulesCache() = default;
+
+ virtual std::optional<std::string>
+ getSourceForModuleName(llvm::StringRef ModuleName,
+ PathRef RequiredSrcFile = PathRef()) = 0;
+
+ virtual void clearEntry(llvm::StringRef ModuleName,
+ PathRef RequiredSrcFile = PathRef()) = 0;
+
+ virtual void setEntry(PathRef FilePath, llvm::StringRef ModuleName) = 0;
+};
+
+std::unique_ptr<ProjectModulesCache> createProjectModulesCache();
+
+} // namespace clangd
+} // namespace clang
+
+#endif
diff --git a/clang-tools-extra/clangd/ScanningProjectModules.cpp b/clang-tools-extra/clangd/ScanningProjectModules.cpp
index e4dc11c1c28958b..3b0cd96e01c7fff 100644
--- a/clang-tools-extra/clangd/ScanningProjectModules.cpp
+++ b/clang-tools-extra/clangd/ScanningProjectModules.cpp
@@ -49,7 +49,8 @@ class ModuleDependencyScanner {
/// Scanning the single file specified by \param FilePath.
std::optional<ModuleDependencyInfo>
- scan(PathRef FilePath, const ProjectModules::CommandMangler &Mangler);
+ scan(PathRef FilePath, const ProjectModules::CommandMangler &Mangler,
+ ProjectModulesCache *Cache);
/// Scanning every source file in the current project to get the
/// <module-name> to <module-unit-source> map.
@@ -58,7 +59,8 @@ class ModuleDependencyScanner {
/// a global module dependency scanner to monitor every file. Or we
/// can simply require the build systems (or even the end users)
/// to provide the map.
- void globalScan(const ProjectModules::CommandMangler &Mangler);
+ void globalScan(const ProjectModules::CommandMangler &Mangler,
+ ProjectModulesCache &Cache);
/// Get the source file from the module name. Note that the language
/// guarantees all the module names are unique in a valid program.
@@ -68,6 +70,12 @@ class ModuleDependencyScanner {
/// declaring the same module.
PathRef getSourceForModuleName(llvm::StringRef ModuleName) const;
+ /// Validate if source file \c Source declare a module with \c ModuleName.
+ /// If yes, return the source file path. Otherwise, return std::nullopt.
+ std::optional<PathRef>
+ validateSourceForModuleName(PathRef Source, llvm::StringRef ModuleName,
+ const ProjectModules::CommandMangler &Mangler);
+
/// Return the direct required modules. Indirect required modules are not
/// included.
std::vector<std::string>
@@ -83,15 +91,14 @@ class ModuleDependencyScanner {
clang::tooling::dependencies::DependencyScanningService Service;
- // TODO: Add a scanning cache.
-
// Map module name to source file path.
llvm::StringMap<std::string> ModuleNameToSource;
};
std::optional<ModuleDependencyScanner::ModuleDependencyInfo>
ModuleDependencyScanner::scan(PathRef FilePath,
- const ProjectModules::CommandMangler &Mangler) {
+ const ProjectModules::CommandMangler &Mangler,
+ ProjectModulesCache *Cache) {
auto Candidates = CDB->getCompileCommands(FilePath);
if (Candidates.empty())
return std::nullopt;
@@ -124,6 +131,9 @@ ModuleDependencyScanner::scan(PathRef FilePath,
if (ScanningResult->Provides) {
ModuleNameToSource[ScanningResult->Provides->ModuleName] = FilePath;
Result.ModuleName = ScanningResult->Provides->ModuleName;
+
+ if (Cache)
+ Cache->setEntry(FilePath, ScanningResult->Provides->ModuleName);
}
for (auto &Required : ScanningResult->Requires)
@@ -133,9 +143,9 @@ ModuleDependencyScanner::scan(PathRef FilePath,
}
void ModuleDependencyScanner::globalScan(
- const ProjectModules::CommandMangler &Mangler) {
+ const ProjectModules::CommandMangler &Mangler, ProjectModulesCache &Cache) {
for (auto &File : CDB->getAllFiles())
- scan(File, Mangler);
+ scan(File, Mangler, &Cache);
GlobalScanned = true;
}
@@ -155,12 +165,28 @@ PathRef ModuleDependencyScanner::getSourceForModuleName(
std::vector<std::string> ModuleDependencyScanner::getRequiredModules(
PathRef File, const ProjectModules::CommandMangler &Mangler) {
- auto ScanningResult = scan(File, Mangler);
+ auto ScanningResult = scan(File, Mangler, /*Cache=*/nullptr);
if (!ScanningResult)
return {};
return ScanningResult->RequiredModules;
}
+
+std::optional<PathRef> ModuleDependencyScanner::validateSourceForModuleName(
+ PathRef Source, llvm::StringRef ModuleName,
+ const ProjectModules::CommandMangler &Mangler) {
+ auto ScanningResult = scan(Source, Mangler, /*Cache=*/nullptr);
+ if (!ScanningResult)
+ return std::nullopt;
+
+ // If the name matches, return the source file path from ModuleNameToSource
+ // cache instead of the input. Since the lifetime of the input may not be long
+ // enough.
+ if (ScanningResult->ModuleName == ModuleName)
+ return ModuleNameToSource[ModuleName];
+
+ return std::nullopt;
+}
} // namespace
/// TODO: The existing `ScanningAllProjectModules` is not efficient. See the
@@ -190,9 +216,18 @@ class ScanningAllProjectModules : public ProjectModules {
/// RequiredSourceFile is not used intentionally. See the comments of
/// ModuleDependencyScanner for detail.
PathRef
- getSourceForModuleName(llvm::StringRef ModuleName,
+ getSourceForModuleName(ProjectModulesCache &Cache, llvm::StringRef ModuleName,
PathRef RequiredSourceFile = PathRef()) override {
- Scanner.globalScan(Mangler);
+ if (auto Source =
+ Cache.getSourceForModuleName(ModuleName, RequiredSourceFile)) {
+ if (std::optional<PathRef> Path =
+ Scanner.validateSourceForModuleName(*Source, ModuleName, Mangler))
+ return *Path;
+
+ Cache.clearEntry(ModuleName, RequiredSourceFile);
+ }
+
+ Scanner.globalScan(Mangler, Cache);
return Scanner.getSourceForModuleName(ModuleName);
}
|
kadircet
requested changes
Feb 6, 2025
There was a problem hiding this comment.
instead of introducing a new concept of ProjectModulesCache and requiring each ProjectModules implementation to know about it and work with it; what about introducing a:
class CachingProjectModules : public ProjectModules {
public:
CachingProjectModules(GlobalCompilationDatabase &CDB);
// Implements rest of the operations with a cache overlay.
...;
// Whenever cache is invalid/empty, queries `CDB` for underlying `ProjectModules`
// and updates/builds a cache using it.
};into ModulesBuilder.cpp and have an instance of this in ModulesBuilderImpl? That way we can centralize how the caching will work and won't need to maintain multiple implementations that play with it.
As for cache validation, i think it's fine to invalidate based on file contents and whenever there's a mismatch we can query CDB again.
WDYT?
It sounds good but it has a small problem that:
|
Not sure I understand the argument here. As for thread-safety concerns in Does that make sense? If not could you elaborate on your concern? |
ChuanqiXu9
force-pushed
the
ModulesScanningCache
branch
from
b452efa to
42eb382
Compare
Yeah, I though you were saying to store the underlying ProjectModules. Now it is fine. |
kadircet
requested changes
Feb 13, 2025
|
Update: I asked the Scanner to not run again after global scanned. I felt this was an simple over sight. It doesn't make sense to ask a scanner to scan globally multiple times. |
kadircet
approved these changes
Feb 26, 2025
|
I hope to backport this to 20.x. It shouldn't be riskful given the modules support in clangd is controlled by an "experiment" option. |
|
/cherry-pick ae839b0 |
|
/pull-request #128841 |
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.
Previously, everytime we want to get a source file declaring a specific module, we need to scan the whole projects again and again. The performance is super bad. This patch tries to improve this by introducing a simple cache.