[PATCH] D64682: [clangd] Don't rebuild background index until we indexed one TU per thread.

[Sam McCall via Phabricator via llvm-commits]

This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rL366199: [clangd] Don't rebuild background index until we indexed one TU per thread. (authored by sammccall, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D64682?vs=209651&id=210056#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64682/new/

https://reviews.llvm.org/D64682

Files:
  clang-tools-extra/trunk/clangd/index/Background.cpp
  clang-tools-extra/trunk/clangd/index/BackgroundRebuild.h
  clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp


Index: clang-tools-extra/trunk/clangd/index/BackgroundRebuild.h
===================================================================
--- clang-tools-extra/trunk/clangd/index/BackgroundRebuild.h
+++ clang-tools-extra/trunk/clangd/index/BackgroundRebuild.h
@@ -16,6 +16,7 @@
 
 #include "index/FileIndex.h"
 #include "index/Index.h"
+#include "llvm/Support/Threading.h"
 
 namespace clang {
 namespace clangd {
@@ -45,12 +46,9 @@
 // This class is exposed in the header so it can be tested.
 class BackgroundIndexRebuilder {
 public:
-  // Thresholds for rebuilding as TUs get indexed.
-  static constexpr unsigned TUsBeforeFirstBuild = 5;
-  static constexpr unsigned TUsBeforeRebuild = 100;
-
-  BackgroundIndexRebuilder(SwapIndex *Target, FileSymbols *Source)
-      : Target(Target), Source(Source) {}
+  BackgroundIndexRebuilder(SwapIndex *Target, FileSymbols *Source,
+                           unsigned Threads)
+      : TUsBeforeFirstBuild(Threads), Target(Target), Source(Source) {}
 
   // Called to indicate a TU has been indexed.
   // May rebuild, if enough TUs have been indexed.
@@ -71,6 +69,10 @@
   // Ensures we won't start any more rebuilds.
   void shutdown();
 
+  // Thresholds for rebuilding as TUs get indexed.
+  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);
Index: clang-tools-extra/trunk/clangd/index/Background.cpp
===================================================================
--- clang-tools-extra/trunk/clangd/index/Background.cpp
+++ clang-tools-extra/trunk/clangd/index/Background.cpp
@@ -127,7 +127,7 @@
     BackgroundIndexStorage::Factory IndexStorageFactory, size_t ThreadPoolSize)
     : SwapIndex(llvm::make_unique<MemIndex>()), FSProvider(FSProvider),
       CDB(CDB), BackgroundContext(std::move(BackgroundContext)),
-      Rebuilder(this, &IndexedSymbols),
+      Rebuilder(this, &IndexedSymbols, ThreadPoolSize),
       IndexStorageFactory(std::move(IndexStorageFactory)),
       CommandsChanged(
           CDB.watch([&](const std::vector<std::string> &ChangedFiles) {
Index: clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp
===================================================================
--- clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp
@@ -575,7 +575,8 @@
 class BackgroundIndexRebuilderTest : public testing::Test {
 protected:
   BackgroundIndexRebuilderTest()
-      : Target(llvm::make_unique<MemIndex>()), Rebuilder(&Target, &Source) {
+      : Target(llvm::make_unique<MemIndex>()),
+        Rebuilder(&Target, &Source, /*Threads=*/10) {
     // Prepare FileSymbols with TestSymbol in it, for checkRebuild.
     TestSymbol.ID = SymbolID("foo");
   }
@@ -610,11 +611,10 @@
 };
 
 TEST_F(BackgroundIndexRebuilderTest, IndexingTUs) {
-  for (unsigned I = 0; I < BackgroundIndexRebuilder::TUsBeforeFirstBuild - 1;
-       ++I)
+  for (unsigned I = 0; I < Rebuilder.TUsBeforeFirstBuild - 1; ++I)
     EXPECT_FALSE(checkRebuild([&] { Rebuilder.indexedTU(); }));
   EXPECT_TRUE(checkRebuild([&] { Rebuilder.indexedTU(); }));
-  for (unsigned I = 0; I < BackgroundIndexRebuilder::TUsBeforeRebuild - 1; ++I)
+  for (unsigned I = 0; I < Rebuilder.TUsBeforeRebuild - 1; ++I)
     EXPECT_FALSE(checkRebuild([&] { Rebuilder.indexedTU(); }));
   EXPECT_TRUE(checkRebuild([&] { Rebuilder.indexedTU(); }));
 }
@@ -640,7 +640,7 @@
 
   // No rebuilding for indexed files while loading.
   Rebuilder.startLoading();
-  for (unsigned I = 0; I < 3 * BackgroundIndexRebuilder::TUsBeforeRebuild; ++I)
+  for (unsigned I = 0; I < 3 * Rebuilder.TUsBeforeRebuild; ++I)
     EXPECT_FALSE(checkRebuild([&] { Rebuilder.indexedTU(); }));
   // But they get indexed when we're done, even if no shards were loaded.
   EXPECT_TRUE(checkRebuild([&] { Rebuilder.doneLoading(); }));


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D64682.210056.patch
Type: text/x-patch
Size: 4030 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190716/33e0fcd4/attachment.bin>