[clangd] Make background index thread count calculation clearer

[sammccall]

Summary

This confusion was inadvertently introduced in a change to the
heavyweight_hardware_concurrency API: 8404aeb56a73ab24f9b295111de3b37a37f0b841

• 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

[aganea]

LGTM. Thanks for reverting.

[kadircet]

Thanks LGTM!

clang-tools-extra/clangd/index/Background.h

140

maybe ClangdServer::optsForTest().AsyncThreadsCount instead of hardcoding 4 in another place ?

[sammccall]

clang-tools-extra/clangd/index/Background.h

140

This file doesn't/shouldn't depend on clangdserver though :-(

clang-tools-extra/clangd/index/Background.h

 public:
   /// If BuildIndexPeriodMs is greater than 0, the symbol index will only be
   /// rebuilt periodically (one per \p BuildIndexPeriodMs); otherwise, index is
   /// rebuilt for each indexed file.
   BackgroundIndex(
       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.
 
   // Enqueue translation units for indexing.
   // The indexing happens in a background thread, so the symbols will be
   // available sometime later.
   void enqueue(const std::vector<std::string> &ChangedFiles) {
     Queue.push(changedFilesTask(ChangedFiles));

clang-tools-extra/clangd/index/Background.cpp

: SwapIndex(std::make_unique<MemIndex>()), TFS(TFS), CDB(CDB),
       BackgroundContext(std::move(BackgroundContext)),
       Rebuilder(this, &IndexedSymbols, ThreadPoolSize),
       IndexStorageFactory(std::move(IndexStorageFactory)),
       Queue(std::move(OnProgress)),
       CommandsChanged(
           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(); });
     });
   }
 }
 
 BackgroundIndex::~BackgroundIndex() {

clang-tools-extra/clangd/index/BackgroundRebuild.h

 // etc. Without external locking, the rebuilt index may include more updates
 // than intended, which is fine.
 //
 // This class is exposed in the header so it can be tested.
 class BackgroundIndexRebuilder {
 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.
   void indexedTU();
   // Called to indicate that all worker threads are idle.
   // May reindex, if the index is not up to date.
   void idle();
   // Called to indicate we're going to load a batch of shards from disk.
   // startLoading() and doneLoading() must be paired, but multiple loading
   // sessions may happen concurrently.
   void startLoading();
   // Called to indicate some shards were actually loaded from disk.
   void loadedShard(size_t ShardCount);
   // Called to indicate we're finished loading shards from disk.
   // May rebuild (if any were loaded).
   void doneLoading();
 
   // 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;
 
 private:
   // Run Check under the lock, and rebuild if it returns true.
   void maybeRebuild(const char *Reason, std::function<bool()> Check);
   bool enoughTUsToRebuild() const;