sammccall created this revision.
sammccall added reviewers: kadircet, aganea.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
This confusion was inadvertently introduced in a change to the
heavyweight_hardware_concurrency API: 8404aeb56a73ab24f9b295111de3b37a37f0b841
<https://reviews.llvm.org/rG8404aeb56a73ab24f9b295111de3b37a37f0b841>
- don't indirect through the rebuilder policy when building the thread pool
- document that rebuilder thresholds are exposed for testing only
- don't use 0 as a sentinel value for "all threads", as we use it as a sentinel
value for "synchronous" (though unsupported for BackgroundIndex)
- rather than pick some new sentinel value, just always use 4 threads for tests
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D82352
Files:
clang-tools-extra/clangd/index/Background.cpp
clang-tools-extra/clangd/index/Background.h
clang-tools-extra/clangd/index/BackgroundRebuild.h
Index: clang-tools-extra/clangd/index/BackgroundRebuild.h
===================================================================
--- clang-tools-extra/clangd/index/BackgroundRebuild.h
+++ clang-tools-extra/clangd/index/BackgroundRebuild.h
@@ -49,9 +49,7 @@
public:
BackgroundIndexRebuilder(SwapIndex *Target, FileSymbols *Source,
unsigned Threads)
- : TUsBeforeFirstBuild(llvm::heavyweight_hardware_concurrency(Threads)
- .compute_thread_count()),
- Target(Target), Source(Source) {}
+ : TUsBeforeFirstBuild(Threads), Target(Target), Source(Source) {}
// Called to indicate a TU has been indexed.
// May rebuild, if enough TUs have been indexed.
@@ -72,7 +70,7 @@
// Ensures we won't start any more rebuilds.
void shutdown();
- // Thresholds for rebuilding as TUs get indexed.
+ // Thresholds for rebuilding as TUs get indexed. Exposed for testing.
const unsigned TUsBeforeFirstBuild; // Typically one per worker thread.
const unsigned TUsBeforeRebuild = 100;
Index: clang-tools-extra/clangd/index/Background.h
===================================================================
--- clang-tools-extra/clangd/index/Background.h
+++ clang-tools-extra/clangd/index/Background.h
@@ -135,7 +135,9 @@
Context BackgroundContext, const ThreadsafeFS &,
const GlobalCompilationDatabase &CDB,
BackgroundIndexStorage::Factory IndexStorageFactory,
- size_t ThreadPoolSize = 0, // 0 = use all hardware threads
+ // Arbitrary value to ensure some concurrency in tests.
+ // In production an explicit value is passed.
+ size_t ThreadPoolSize = 4,
std::function<void(BackgroundQueue::Stats)> OnProgress = nullptr);
~BackgroundIndex(); // Blocks while the current task finishes.
Index: clang-tools-extra/clangd/index/Background.cpp
===================================================================
--- clang-tools-extra/clangd/index/Background.cpp
+++ clang-tools-extra/clangd/index/Background.cpp
@@ -103,10 +103,9 @@
CDB.watch([&](const std::vector<std::string> &ChangedFiles) {
enqueue(ChangedFiles);
})) {
- assert(Rebuilder.TUsBeforeFirstBuild > 0 &&
- "Thread pool size can't be zero.");
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
assert(this->IndexStorageFactory && "Storage factory can not be null!");
- for (unsigned I = 0; I < Rebuilder.TUsBeforeFirstBuild; ++I) {
+ for (unsigned I = 0; I < ThreadPoolSize; ++I) {
ThreadPool.runAsync("background-worker-" + llvm::Twine(I + 1), [this] {
WithContext Ctx(this->BackgroundContext.clone());
Queue.work([&] { Rebuilder.idle(); });
Index: clang-tools-extra/clangd/index/BackgroundRebuild.h
===================================================================
--- clang-tools-extra/clangd/index/BackgroundRebuild.h
+++ clang-tools-extra/clangd/index/BackgroundRebuild.h
@@ -49,9 +49,7 @@
public:
BackgroundIndexRebuilder(SwapIndex *Target, FileSymbols *Source,
unsigned Threads)
- : TUsBeforeFirstBuild(llvm::heavyweight_hardware_concurrency(Threads)
- .compute_thread_count()),
- Target(Target), Source(Source) {}
+ : TUsBeforeFirstBuild(Threads), Target(Target), Source(Source) {}
// Called to indicate a TU has been indexed.
// May rebuild, if enough TUs have been indexed.
@@ -72,7 +70,7 @@
// Ensures we won't start any more rebuilds.
void shutdown();
- // Thresholds for rebuilding as TUs get indexed.
+ // Thresholds for rebuilding as TUs get indexed. Exposed for testing.
const unsigned TUsBeforeFirstBuild; // Typically one per worker thread.
const unsigned TUsBeforeRebuild = 100;
Index: clang-tools-extra/clangd/index/Background.h
===================================================================
--- clang-tools-extra/clangd/index/Background.h
+++ clang-tools-extra/clangd/index/Background.h
@@ -135,7 +135,9 @@
Context BackgroundContext, const ThreadsafeFS &,
const GlobalCompilationDatabase &CDB,
BackgroundIndexStorage::Factory IndexStorageFactory,
- size_t ThreadPoolSize = 0, // 0 = use all hardware threads
+ // Arbitrary value to ensure some concurrency in tests.
+ // In production an explicit value is passed.
+ size_t ThreadPoolSize = 4,
std::function<void(BackgroundQueue::Stats)> OnProgress = nullptr);
~BackgroundIndex(); // Blocks while the current task finishes.
Index: clang-tools-extra/clangd/index/Background.cpp
===================================================================
--- clang-tools-extra/clangd/index/Background.cpp
+++ clang-tools-extra/clangd/index/Background.cpp
@@ -103,10 +103,9 @@
CDB.watch([&](const std::vector<std::string> &ChangedFiles) {
enqueue(ChangedFiles);
})) {
- assert(Rebuilder.TUsBeforeFirstBuild > 0 &&
- "Thread pool size can't be zero.");
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
assert(this->IndexStorageFactory && "Storage factory can not be null!");
- for (unsigned I = 0; I < Rebuilder.TUsBeforeFirstBuild; ++I) {
+ for (unsigned I = 0; I < ThreadPoolSize; ++I) {
ThreadPool.runAsync("background-worker-" + llvm::Twine(I + 1), [this] {
WithContext Ctx(this->BackgroundContext.clone());
Queue.work([&] { Rebuilder.idle(); });
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
