[clangd] BackgroundIndex rebuilds symbol index periodically.

Summary:
Currently, background index rebuilds symbol index on every indexed file,
which can be inefficient. This patch makes it only rebuild symbol index periodically.
As the rebuild no longer happens too often, we could also build more efficient
dex index.

Reviewers: ilya-biryukov, kadircet

Reviewed By: kadircet

Subscribers: dblaikie, MaskRay, jkorous, arphaman, jfb, cfe-commits

Differential Revision: https://reviews.llvm.org/D55770

llvm-svn: 349496

clang-tools-extra/clangd/ClangdServer.cpp

@@ -139,7 +139,8 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
   if (Opts.BackgroundIndex) {
     BackgroundIdx = llvm::make_unique<BackgroundIndex>(
         Context::current().clone(), ResourceDir, FSProvider, CDB,
-        BackgroundIndexStorage::createDiskBackedStorageFactory());
+        BackgroundIndexStorage::createDiskBackedStorageFactory(),
+        Opts.BackgroundIndexRebuildPeriodMs);
     AddIndex(BackgroundIdx.get());
   }
   if (DynamicIdx)

clang-tools-extra/clangd/ClangdServer.h

@@ -85,6 +85,10 @@ public:
     /// If true, ClangdServer automatically indexes files in the current project
     /// on background threads. The index is stored in the project root.
     bool BackgroundIndex = false;
+    /// If set to non-zero, the background index rebuilds the symbol index
+    /// periodically every BuildIndexPeriodMs milliseconds; otherwise, the
+    /// symbol index will be updated for each indexed file.
+    size_t BackgroundIndexRebuildPeriodMs = 0;
 
     /// If set, use this index to augment code completion results.
     SymbolIndex *StaticIndex = nullptr;

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

@@ -27,11 +27,13 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SHA1.h"
 
+#include <chrono>
 #include <memory>
 #include <numeric>
 #include <queue>
 #include <random>
 #include <string>
+#include <thread>
 
 using namespace llvm;
 namespace clang {
@@ -125,10 +127,13 @@ createFileFilter(const llvm::StringMap<FileDigest> &FileDigests,
 BackgroundIndex::BackgroundIndex(
     Context BackgroundContext, StringRef ResourceDir,
     const FileSystemProvider &FSProvider, const GlobalCompilationDatabase &CDB,
-    BackgroundIndexStorage::Factory IndexStorageFactory, size_t ThreadPoolSize)
+    BackgroundIndexStorage::Factory IndexStorageFactory,
+    size_t BuildIndexPeriodMs, size_t ThreadPoolSize)
     : SwapIndex(make_unique<MemIndex>()), ResourceDir(ResourceDir),
       FSProvider(FSProvider), CDB(CDB),
       BackgroundContext(std::move(BackgroundContext)),
+      BuildIndexPeriodMs(BuildIndexPeriodMs),
+      SymbolsUpdatedSinceLastIndex(false),
       IndexStorageFactory(std::move(IndexStorageFactory)),
       CommandsChanged(
           CDB.watch([&](const std::vector<std::string> &ChangedFiles) {
@@ -138,6 +143,11 @@ BackgroundIndex::BackgroundIndex(
   assert(this->IndexStorageFactory && "Storage factory can not be null!");
   while (ThreadPoolSize--)
     ThreadPool.emplace_back([this] { run(); });
+  if (BuildIndexPeriodMs > 0) {
+    log("BackgroundIndex: build symbol index periodically every {0} ms.",
+        BuildIndexPeriodMs);
+    ThreadPool.emplace_back([this] { buildIndex(); });
+  }
 }
 
 BackgroundIndex::~BackgroundIndex() {
@@ -148,10 +158,12 @@ BackgroundIndex::~BackgroundIndex() {
 
 void BackgroundIndex::stop() {
   {
-    std::lock_guard<std::mutex> Lock(QueueMu);
+    std::lock_guard<std::mutex> QueueLock(QueueMu);
+    std::lock_guard<std::mutex> IndexLock(IndexMu);
     ShouldStop = true;
   }
   QueueCV.notify_all();
+  IndexCV.notify_all();
 }
 
 void BackgroundIndex::run() {
@@ -332,6 +344,30 @@ void BackgroundIndex::update(StringRef MainFile, IndexFileIn Index,
   }
 }
 
+void BackgroundIndex::buildIndex() {
+  assert(BuildIndexPeriodMs > 0);
+  while (true) {
+    {
+      std::unique_lock<std::mutex> Lock(IndexMu);
+      if (ShouldStop)  // Avoid waiting if stopped.
+        break;
+      // Wait until this is notified to stop or `BuildIndexPeriodMs` has past.
+      IndexCV.wait_for(Lock, std::chrono::milliseconds(BuildIndexPeriodMs));
+      if (ShouldStop)  // Avoid rebuilding index if stopped.
+        break;
+    }
+    if (!SymbolsUpdatedSinceLastIndex.exchange(false))
+      continue;
+    // There can be symbol update right after the flag is reset above and before
+    // index is rebuilt below. The new index would contain the updated symbols
+    // but the flag would still be true. This is fine as we would simply run an
+    // extra index build.
+    reset(
+        IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
+    log("BackgroundIndex: rebuilt symbol index.");
+  }
+}
+
 Error BackgroundIndex::index(tooling::CompileCommand Cmd,
                              BackgroundIndexStorage *IndexStorage) {
   trace::Span Tracer("BackgroundIndex");
@@ -362,7 +398,7 @@ Error BackgroundIndex::index(tooling::CompileCommand Cmd,
     DigestsSnapshot = IndexedFileDigests;
   }
 
-  log("Indexing {0}", Cmd.Filename, toHex(Hash));
+  log("Indexing {0} (digest:={1})", Cmd.Filename, toHex(Hash));
   ParseInputs Inputs;
   Inputs.FS = std::move(FS);
   Inputs.FS->setCurrentWorkingDirectory(Cmd.Directory);
@@ -424,10 +460,11 @@ Error BackgroundIndex::index(tooling::CompileCommand Cmd,
     IndexedFileDigests[AbsolutePath] = Hash;
   }
 
-  // FIXME: this should rebuild once-in-a-while, not after every file.
-  //       At that point we should use Dex, too.
-  vlog("Rebuilding automatic index");
-  reset(IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
+  if (BuildIndexPeriodMs > 0)
+    SymbolsUpdatedSinceLastIndex = true;
+  else
+    reset(
+        IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
 
   return Error::success();
 }

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

@@ -21,8 +21,10 @@
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/Threading.h"
+#include <atomic>
 #include <condition_variable>
 #include <deque>
+#include <mutex>
 #include <string>
 #include <thread>
 #include <vector>
@@ -63,11 +65,15 @@ public:
 // FIXME: it should watch for changes to files on disk.
 class BackgroundIndex : public SwapIndex {
 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.
   // FIXME: resource-dir injection should be hoisted somewhere common.
   BackgroundIndex(Context BackgroundContext, llvm::StringRef ResourceDir,
                   const FileSystemProvider &,
                   const GlobalCompilationDatabase &CDB,
                   BackgroundIndexStorage::Factory IndexStorageFactory,
+                  size_t BuildIndexPeriodMs = 0,
                   size_t ThreadPoolSize = llvm::hardware_concurrency());
   ~BackgroundIndex(); // Blocks while the current task finishes.
 
@@ -101,6 +107,11 @@ private:
   // index state
   llvm::Error index(tooling::CompileCommand,
                     BackgroundIndexStorage *IndexStorage);
+  void buildIndex(); // Rebuild index periodically every BuildIndexPeriodMs.
+  const size_t BuildIndexPeriodMs;
+  std::atomic<bool> SymbolsUpdatedSinceLastIndex;
+  std::mutex IndexMu;
+  std::condition_variable IndexCV;
 
   FileSymbols IndexedSymbols;
   llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path.

clang-tools-extra/clangd/tool/ClangdMain.cpp

@@ -170,6 +170,14 @@ static cl::opt<bool> EnableBackgroundIndex(
              "Experimental"),
     cl::init(false), cl::Hidden);
 
+static cl::opt<int> BackgroundIndexRebuildPeriod(
+    "background-index-rebuild-period",
+    cl::desc(
+        "If set to non-zero, the background index rebuilds the symbol index "
+        "periodically every X milliseconds; otherwise, the "
+        "symbol index will be updated for each indexed file."),
+    cl::init(5000), cl::Hidden);
+
 enum CompileArgsFrom { LSPCompileArgs, FilesystemCompileArgs };
 static cl::opt<CompileArgsFrom> CompileArgsFrom(
     "compile_args_from", cl::desc("The source of compile commands"),
@@ -352,6 +360,7 @@ int main(int argc, char *argv[]) {
   Opts.BuildDynamicSymbolIndex = EnableIndex;
   Opts.HeavyweightDynamicSymbolIndex = UseDex;
   Opts.BackgroundIndex = EnableBackgroundIndex;
+  Opts.BackgroundIndexRebuildPeriodMs = BackgroundIndexRebuildPeriod;
   std::unique_ptr<SymbolIndex> StaticIdx;
   std::future<void> AsyncIndexLoad; // Block exit while loading the index.
   if (EnableIndex && !IndexFile.empty()) {

clang-tools-extra/test/clangd/background-index.test

@@ -10,7 +10,7 @@
 # We're editing bar.cpp, which includes foo.h.
 # foo() is declared in foo.h and defined in foo.cpp.
 # The background index should allow us to go-to-definition on foo().
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
+# RUN: clangd -background-index -background-index-rebuild-period=0 -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
 
 # Test that the index is writing files in the expected location.
 # RUN: ls %t/.clangd-index/foo.cpp.*.idx

clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp

@@ -2,11 +2,14 @@
 #include "TestFS.h"
 #include "index/Background.h"
 #include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/Threading.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <thread>
 
 using testing::_;
 using testing::AllOf;
+using testing::ElementsAre;
 using testing::Not;
 using testing::UnorderedElementsAre;
 
@@ -240,5 +243,39 @@ TEST_F(BackgroundIndexTest, DirectIncludesTest) {
               EmptyIncludeNode());
 }
 
+TEST_F(BackgroundIndexTest, PeriodicalIndex) {
+  MockFSProvider FS;
+  llvm::StringMap<std::string> Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+  OverlayCDB CDB(/*Base=*/nullptr);
+  BackgroundIndex Idx(
+      Context::empty(), "", FS, CDB, [&](llvm::StringRef) { return &MSS; },
+      /*BuildIndexPeriodMs=*/10);
+
+  FS.Files[testPath("root/A.cc")] = "#include \"A.h\"";
+
+  tooling::CompileCommand Cmd;
+  FS.Files[testPath("root/A.h")] = "class X {};";
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.CommandLine = {"clang++", Cmd.Filename};
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+
+  ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  EXPECT_THAT(runFuzzyFind(Idx, ""), ElementsAre());
+  std::this_thread::sleep_for(std::chrono::milliseconds(15));
+  EXPECT_THAT(runFuzzyFind(Idx, ""), ElementsAre(Named("X")));
+
+  FS.Files[testPath("root/A.h")] = "class Y {};";
+  FS.Files[testPath("root/A.cc")] += " "; // Force reindex the file.
+  Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+
+  ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  EXPECT_THAT(runFuzzyFind(Idx, ""), ElementsAre(Named("X")));
+  std::this_thread::sleep_for(std::chrono::milliseconds(15));
+  EXPECT_THAT(runFuzzyFind(Idx, ""), ElementsAre(Named("Y")));
+}
+
 } // namespace clangd
 } // namespace clang