[clang-tools-extra] [clangd] Introduce reusable modules builder (PR #73483)
https://github.com/ChuanqiXu9 edited https://github.com/llvm/llvm-project/pull/73483 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Introduce reusable modules builder (PR #73483)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff e89324219acf3d799a86fed5651e492bbab4867c
052e2da0ede8cc72e22ad9ba75ddf2868e5fffe1 --
clang-tools-extra/clangd/ModuleDependencyScanner.cpp
clang-tools-extra/clangd/ModuleDependencyScanner.h
clang-tools-extra/clangd/ModulesBuilder.cpp
clang-tools-extra/clangd/ModulesBuilder.h
clang-tools-extra/clangd/PrerequisiteModules.h
clang-tools-extra/clangd/ProjectModules.cpp
clang-tools-extra/clangd/ProjectModules.h
clang-tools-extra/clangd/unittests/ModuleDependencyScannerTest.cpp
clang-tools-extra/clangd/unittests/ModulesTestSetup.h
clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
clang-tools-extra/clangd/GlobalCompilationDatabase.h
clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h clang-tools-extra/clangd/tool/Check.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang-tools-extra/clangd/unittests/FileIndexTests.cpp
clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp
clang-tools-extra/clangd/unittests/TestTU.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp
b/clang-tools-extra/clangd/ModulesBuilder.cpp
index c3a2ba89e9..b7c71bb31e 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -100,9 +100,9 @@ struct ModuleFile {
ModuleFile(ModuleFile &&Other)
: ModuleName(std::move(Other.ModuleName)),
ModuleFilePath(std::move(Other.ModuleFilePath)) {
- Other.ModuleName.clear();
- Other.ModuleFilePath.clear();
-}
+Other.ModuleName.clear();
+Other.ModuleFilePath.clear();
+ }
ModuleFile &operator=(ModuleFile &&Other) = delete;
@@ -140,7 +140,8 @@ buildModuleFile(StringRef ModuleName, PathRef
ModuleUnitFile,
IgnoreDiagnostics IgnoreDiags;
auto CI = buildCompilerInvocation(Inputs, IgnoreDiags);
if (!CI) {
-log("Failed to build module {0} since build Compiler invocation failed",
ModuleName);
+log("Failed to build module {0} since build Compiler invocation failed",
+ModuleName);
return std::nullopt;
}
@@ -151,7 +152,7 @@ buildModuleFile(StringRef ModuleName, PathRef
ModuleUnitFile,
log("Failed to build module {0} since get buffer failed", ModuleName);
return std::nullopt;
}
-
+
// Try to use the built module files from clangd first.
BuiltModuleFiles.adjustHeaderSearchOptions(CI->getHeaderSearchOpts());
@@ -167,7 +168,8 @@ buildModuleFile(StringRef ModuleName, PathRef
ModuleUnitFile,
prepareCompilerInstance(std::move(CI), /*Preamble=*/nullptr,
std::move(*Buf), std::move(FS), IgnoreDiags);
if (!Clang) {
-log("Failed to build module {0} since build compiler instance failed",
ModuleName);
+log("Failed to build module {0} since build compiler instance failed",
+ModuleName);
return std::nullopt;
}
@@ -192,8 +194,7 @@ public:
/// We shouldn't adjust the compilation commands based on
/// FailedPrerequisiteModules.
- void adjustHeaderSearchOptions(HeaderSearchOptions &Options) const override {
- }
+ void adjustHeaderSearchOptions(HeaderSearchOptions &Options) const override
{}
/// FailedPrerequisiteModules can never be reused.
bool
canReuse(const CompilerInvocation &CI,
@@ -492,8 +493,8 @@ private:
std::shared_ptr &CV,
ReusableModulesBuilder &Builder)
: ModuleName(ModuleName), Mutex(Mutex), CV(CV), Builder(Builder) {
- IsFirstTask = (Mutex.use_count() == 2);
-}
+ IsFirstTask = (Mutex.use_count() == 2);
+}
~ModuleBuildingSharedOwner();
@@ -641,7 +642,8 @@ bool ReusableModulesBuilder::getOrBuildModuleFile(
if (std::shared_ptr Cached =
getValidModuleFile(ModuleName, MDB)) {
-log("Reusing Built Module {0} with {1}", Cached->ModuleName,
Cached->ModuleFilePath);
+log("Reusing Built Module {0} with {1}", Cached->ModuleName,
+Cached->ModuleFilePath);
BuiltModuleFiles.addModuleFile(Cached);
return true;
}
diff --git a/clang-tools-extra/clangd/ProjectModules.h
b/clang-tools-extra/clangd/ProjectModules.h
index 3c47863cf8..5a3ca3a05f 100644
--- a/clang-tools-extra/clangd/ProjectModules.h
+++ b/clang-tools-extra/clangd/ProjectModules.h
@@ -46,7 +46,7 @@ public:
virtual PathRef
getSourceForModuleName(StringRef ModuleName,
[clang-tools-extra] [clangd] Introduce reusable modules builder (PR #73483)
llvmbot wrote: @llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clangd Author: Chuanqi Xu (ChuanqiXu9) Changes This is a draft based on https://github.com/llvm/llvm-project/pull/66462. The main goal of the patch is to implement the TODO step 1: "reuse module files" across source files. In my mind, the modules in clangd will become relatively usable after this patch. I hope we can land this in clang18 too. A big assumption for this patch is that there is exactly one source file declaring the same module (unit) in the project. This is not technically true since we can multiple unrelated modules in the same project. I think this is a understandable assumption for users too. To fully address the underlying issue, we need input from build systems as @mathstuf mentioned in https://github.com/llvm/llvm-project/pull/66462#discussion_r1350209248. But I don't think we have to wait for that. The core ideas for this patch are: - We reuse the ASTWorker thread to build the modules as we did in the previous patch. - We store the built module files in the modules builder. Then the other ASTWorker which needs the module files can get it quickly. - If the module unit file get changed, the modules builder needs to make sure other ASTWorkers don't get the outdated module files. - The built module files are shared by ASTWorkers. So that even if the modules builder remove the module files in its own state, the actual module files won't be deleted until they are released by the ASTWorkers. - When multiple ASTWorkers need the same module file, only one of the workers will build that module file actually and the rest of them will wait for the built result. The model can be improved in future by introducing a thread pool for building modules. The difference is that now when we only open a source file which imports a lot of modules (indirectly), we would only build the modules one by one. However, if we have a thread pool for building modules specially, we can try to build the modules in parallel. To achieve this, we need an explicit module graph. I did this in the first patch: https://reviews.llvm.org/D153114. I think we can try to introduce that thread pool later than sooner since it may be better to keep the changes small instead of large. (This is what required in the first review page.) I tested this patch with real workloads. I'll add in-tree test once https://github.com/llvm/llvm-project/pull/66462 landed (Otherwise refactoring interfaces may be painful) --- Patch is 92.78 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/73483.diff 31 Files Affected: - (modified) clang-tools-extra/clangd/CMakeLists.txt (+4) - (modified) clang-tools-extra/clangd/ClangdServer.cpp (+2) - (modified) clang-tools-extra/clangd/ClangdServer.h (+3) - (modified) clang-tools-extra/clangd/GlobalCompilationDatabase.cpp (+23) - (modified) clang-tools-extra/clangd/GlobalCompilationDatabase.h (+14) - (added) clang-tools-extra/clangd/ModuleDependencyScanner.cpp (+90) - (added) clang-tools-extra/clangd/ModuleDependencyScanner.h (+108) - (added) clang-tools-extra/clangd/ModulesBuilder.cpp (+706) - (added) clang-tools-extra/clangd/ModulesBuilder.h (+61) - (modified) clang-tools-extra/clangd/ParsedAST.cpp (+7) - (modified) clang-tools-extra/clangd/Preamble.cpp (+22-6) - (modified) clang-tools-extra/clangd/Preamble.h (+10) - (added) clang-tools-extra/clangd/PrerequisiteModules.h (+83) - (added) clang-tools-extra/clangd/ProjectModules.cpp (+66) - (added) clang-tools-extra/clangd/ProjectModules.h (+56) - (modified) clang-tools-extra/clangd/TUScheduler.cpp (+33-18) - (modified) clang-tools-extra/clangd/TUScheduler.h (+7) - (modified) clang-tools-extra/clangd/test/CMakeLists.txt (+1) - (added) clang-tools-extra/clangd/test/modules.test (+83) - (modified) clang-tools-extra/clangd/tool/Check.cpp (+13-2) - (modified) clang-tools-extra/clangd/tool/ClangdMain.cpp (+8) - (modified) clang-tools-extra/clangd/unittests/CMakeLists.txt (+2) - (modified) clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp (+17-5) - (modified) clang-tools-extra/clangd/unittests/FileIndexTests.cpp (+3-1) - (added) clang-tools-extra/clangd/unittests/ModuleDependencyScannerTest.cpp (+176) - (added) clang-tools-extra/clangd/unittests/ModulesTestSetup.h (+103) - (modified) clang-tools-extra/clangd/unittests/ParsedASTTests.cpp (+6-2) - (modified) clang-tools-extra/clangd/unittests/PreambleTests.cpp (+4-2) - (added) clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp (+227) - (modified) clang-tools-extra/clangd/unittests/TestTU.cpp (+10-4) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+3) ``diff diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt index 3911fb6c6c746a8..242a8ad2e350be7 100644 --- a/clang-tools-extra/clangd/CMakeLists.txt +++ b/clang-tools-extra/clangd/CMakeLists.txt @@ -97,7 +97,10 @@
[clang-tools-extra] [clangd] Introduce reusable modules builder (PR #73483)
https://github.com/ChuanqiXu9 converted_to_draft https://github.com/llvm/llvm-project/pull/73483 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
