[clangd] Extract BackgroundIndex::Options struct. NFC

[sammccall]

Summary

I've dropped the background context parameter, since we in practice just pass the
current context there, and we now have a different way to specify context too.
While here, clean up a couple of comments.

[kadircet]

thanks for doing this!

clang-tools-extra/clangd/ClangdServer.cpp

185

I think we should move storage factory into options too ?

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

127

🎉

[sammccall]

clang-tools-extra/clangd/ClangdServer.cpp

185

The storage factory isn't optional, nor does it have a good default. So it seems mechanically awkward to put it in the struct, and I'm not sure there's much benefit...

How would you see this being initialized?

[kadircet]

clang-tools-extra/clangd/ClangdServer.cpp

185

I thought we were exposing NullStorage so I was planning for it to be the default for ignorant call sites. But seeing it is not exposed, and this only being used by ClangdServer (and tests), I suppose it is not that important.

clang-tools-extra/clangd/ClangdServer.cpp

if (this->Index != nullptr) {
       this->Index = MergedIdx.back().get();
     } else {
       this->Index = Idx;
     }
   };
   if (Opts.StaticIndex)
     AddIndex(Opts.StaticIndex);
   if (Opts.BackgroundIndex) {
+    BackgroundIndex::Options BGOpts;
+    BGOpts.ThreadPoolSize = std::max(Opts.AsyncThreadsCount, 1u);
+    BGOpts.OnProgress = [Callbacks](BackgroundQueue::Stats S) {
+      if (Callbacks)
+        Callbacks->onBackgroundIndexProgress(S);
+    };
+    BGOpts.ContextProvider = [this](PathRef P) {
+      return createProcessingContext(P);
+    };
     BackgroundIdx = std::make_unique<BackgroundIndex>(
-        Context::current().clone(), TFS, CDB,
+        TFS, CDB,
         BackgroundIndexStorage::createDiskBackedStorageFactory(
             [&CDB](llvm::StringRef File) { return CDB.getProjectInfo(File); }),
-        std::max(Opts.AsyncThreadsCount, 1u),
-        [Callbacks](BackgroundQueue::Stats S) {
-          if (Callbacks)
-            Callbacks->onBackgroundIndexProgress(S);
-        },
-        [this](PathRef P) { return createProcessingContext(P); });
+        std::move(BGOpts));
     AddIndex(BackgroundIdx.get());
   }
   if (DynamicIdx)
     AddIndex(DynamicIdx.get());
 }
 
 void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
                                llvm::StringRef Version,

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

private:
   bool ShouldStop = false;
   std::vector<Task> Queue; // max-heap
   llvm::StringMap<unsigned> Boosts;
   std::function<void(Stats)> OnProgress;
 };
 
 // Builds an in-memory index by by running the static indexer action over
 // all commands in a compilation database. Indexing happens in the background.
-// FIXME: it should also persist its state on disk for fast start.
 // 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.
-  BackgroundIndex(
-      Context BackgroundContext, const ThreadsafeFS &,
-      const GlobalCompilationDatabase &CDB,
-      BackgroundIndexStorage::Factory IndexStorageFactory,
-      // 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,
-      std::function<Context(PathRef)> ContextProvider = nullptr);
+  struct Options {
+    // In production an explicit value is specified.
+    size_t ThreadPoolSize = 4;
+    // Callback that provides notifications as indexing makes progress.
+    std::function<void(BackgroundQueue::Stats)> OnProgress = nullptr;
+    // Function called to obtain the Context to use while indexing the specified
+    // file. Called with the empty string for other tasks.
+    // (When called, the context from BackgroundIndex construction is active).
+    std::function<Context(PathRef)> ContextProvider = nullptr;
+  };
+
+  /// Creates a new background index and starts its threads.
+  /// The current Context will be propagated to each worker thread.
+  BackgroundIndex(const ThreadsafeFS &, const GlobalCompilationDatabase &CDB,
+                  BackgroundIndexStorage::Factory IndexStorageFactory,
+                  Options Opts);
   ~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));
   }
private:
   /// information on IndexStorage.
   void update(llvm::StringRef MainFile, IndexFileIn Index,
               const llvm::StringMap<ShardVersion> &ShardVersionsSnapshot,
               bool HadErrors);
 
   // configuration
   const ThreadsafeFS &TFS;
   const GlobalCompilationDatabase &CDB;
-  Context BackgroundContext;
   std::function<Context(PathRef)> ContextProvider;
 
   llvm::Error index(tooling::CompileCommand);
 
   FileSymbols IndexedSymbols;
   BackgroundIndexRebuilder Rebuilder;
   llvm::StringMap<ShardVersion> ShardVersions; // Key is absolute file path.
   std::mutex ShardVersionsMu;

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

if (!Buf) {
     return false;
   }
   return digest(Buf->get()->getBuffer()) != LS.Digest;
 }
 
 } // namespace
 
 BackgroundIndex::BackgroundIndex(
-    Context BackgroundContext, const ThreadsafeFS &TFS,
-    const GlobalCompilationDatabase &CDB,
-    BackgroundIndexStorage::Factory IndexStorageFactory, size_t ThreadPoolSize,
-    std::function<void(BackgroundQueue::Stats)> OnProgress,
-    std::function<Context(PathRef)> ContextProvider)
+    const ThreadsafeFS &TFS, const GlobalCompilationDatabase &CDB,
+    BackgroundIndexStorage::Factory IndexStorageFactory, Options Opts)
     : SwapIndex(std::make_unique<MemIndex>()), TFS(TFS), CDB(CDB),
-      BackgroundContext(std::move(BackgroundContext)),
-      ContextProvider(std::move(ContextProvider)),
-      Rebuilder(this, &IndexedSymbols, ThreadPoolSize),
+      ContextProvider(std::move(Opts.ContextProvider)),
+      Rebuilder(this, &IndexedSymbols, Opts.ThreadPoolSize),
       IndexStorageFactory(std::move(IndexStorageFactory)),
-      Queue(std::move(OnProgress)),
+      Queue(std::move(Opts.OnProgress)),
       CommandsChanged(
           CDB.watch([&](const std::vector<std::string> &ChangedFiles) {
             enqueue(ChangedFiles);
           })) {
-  assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+  assert(Opts.ThreadPoolSize > 0 && "Thread pool size can't be zero.");
   assert(this->IndexStorageFactory && "Storage factory can not be null!");
-  for (unsigned I = 0; I < ThreadPoolSize; ++I) {
-    ThreadPool.runAsync("background-worker-" + llvm::Twine(I + 1), [this] {
-      WithContext Ctx(this->BackgroundContext.clone());
+  for (unsigned I = 0; I < Opts.ThreadPoolSize; ++I) {
+    ThreadPool.runAsync("background-worker-" + llvm::Twine(I + 1),
+                        [this, Ctx(Context::current().clone())]() mutable {
+                          WithContext BGContext(std::move(Ctx));
                           Queue.work([&] { Rebuilder.idle(); });
                         });
   }
 }
 
 BackgroundIndex::~BackgroundIndex() {
   stop();
   ThreadPool.wait();
 }
 

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

 
 TEST_F(BackgroundIndexTest, NoCrashOnErrorFile) {
   MockFS FS;
   FS.Files[testPath("root/A.cc")] = "error file";
   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; });
+  BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                      /*Opts=*/{});
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
   CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
for (std::string Name : {"foo", "bar", "baz"}) {
     Cmd.Filename = Filename;
     Cmd.Directory = testRoot();
     Cmd.CommandLine = {"clang++", Filename};
     Cmds.push_back(std::move(Cmd));
   }
   // Context provider that installs a configuration mutating foo's command.
   // This causes it to define foo::two instead of foo::one.
   // It also disables indexing of baz entirely.
-  auto ContextProvider = [](PathRef P) {
+  BackgroundIndex::Options Opts;
+  Opts.ContextProvider = [](PathRef P) {
     Config C;
     if (P.endswith("foo.cpp"))
       C.CompileFlags.Edits.push_back(
           [](std::vector<std::string> &Argv) { Argv.push_back("-Done=two"); });
     if (P.endswith("baz.cpp"))
       C.Index.Background = Config::BackgroundPolicy::Skip;
     return Context::current().derive(Config::Key, std::move(C));
   };
   // Create the background index.
   llvm::StringMap<std::string> Storage;
   size_t CacheHits = 0;
   MemoryShardStorage MSS(Storage, CacheHits);
   // We need the CommandMangler, because that applies the config we're testing.
   OverlayCDB CDB(/*Base=*/nullptr, /*FallbackFlags=*/{},
                  tooling::ArgumentsAdjuster(CommandMangler::forTests()));
+
   BackgroundIndex Idx(
-      Context::empty(), FS, CDB, [&](llvm::StringRef) { return &MSS; },
-      /*ThreadPoolSize=*/4, /*OnProgress=*/nullptr, std::move(ContextProvider));
+      FS, CDB, [&](llvm::StringRef) { return &MSS; }, std::move(Opts));
   // Index the two files.
   for (auto &Cmd : Cmds) {
     std::string FullPath = testPath(Cmd.Filename);
     CDB.setCompileCommand(FullPath, std::move(Cmd));
   }
   // Wait for both files to be indexed.
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   EXPECT_THAT(runFuzzyFind(Idx, ""),
FS.Files[testPath("root/B.cc")] =
         (void)common;
         (void)common;
         (void)common;
       })cpp";
   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; });
+  BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                      /*Opts=*/{});
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
   CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
TEST_F(BackgroundIndexTest, ShardStorageTest) {
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
   // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), FS, CDB,
-                        [&](llvm::StringRef) { return &MSS; });
+    BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                        /*Opts=*/{});
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 0U);
   EXPECT_EQ(Storage.size(), 2U);
 
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), FS, CDB,
-                        [&](llvm::StringRef) { return &MSS; });
+    BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                        /*Opts=*/{});
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
   EXPECT_EQ(Storage.size(), 2U);
 
   auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
TEST_F(BackgroundIndexTest, DirectIncludesTest) {
   MemoryShardStorage MSS(Storage, CacheHits);
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), FS, CDB,
-                        [&](llvm::StringRef) { return &MSS; });
+    BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                        /*Opts=*/{});
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
 
   auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
   EXPECT_TRUE(ShardSource->Sources);
   EXPECT_EQ(ShardSource->Sources->size(), 2U); // A.cc, A.h
   EXPECT_THAT(
TEST_F(BackgroundIndexTest, ShardStorageLoad) {
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
   // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), FS, CDB,
-                        [&](llvm::StringRef) { return &MSS; });
+    BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                        /*Opts=*/{});
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
 
   // Change header.
   FS.Files[testPath("root/A.h")] = R"cpp(
       void common();
       void f_b();
       class A_CC {};
       class A_CCnew {};
       )cpp";
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), FS, CDB,
-                        [&](llvm::StringRef) { return &MSS; });
+    BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                        /*Opts=*/{});
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
 
   // Check if the new symbol has arrived.
   auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
 
   // Change source.
   FS.Files[testPath("root/A.cc")] =
       "#include \"A.h\"\nvoid g() { (void)common; }\nvoid f_b() {}";
   {
     CacheHits = 0;
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), FS, CDB,
-                        [&](llvm::StringRef) { return &MSS; });
+    BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                        /*Opts=*/{});
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
 
   // Check if the new symbol has arrived.
   ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
TEST_F(BackgroundIndexTest, ShardStorageEmptyFile) {
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
   // Check that A.cc, A.h and B.h has been stored.
   {
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), FS, CDB,
-                        [&](llvm::StringRef) { return &MSS; });
+    BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                        /*Opts=*/{});
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_THAT(Storage.keys(),
               UnorderedElementsAre(testPath("root/A.cc"), testPath("root/A.h"),
                                    testPath("root/B.h")));
   auto ShardHeader = MSS.loadShard(testPath("root/B.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_TRUE(ShardHeader->Symbols->empty());
 
   // Check that A.cc, A.h and B.h has been loaded.
   {
     CacheHits = 0;
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), FS, CDB,
-                        [&](llvm::StringRef) { return &MSS; });
+    BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                        /*Opts=*/{});
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 3U);
 
   // Update B.h to contain some symbols.
   FS.Files[testPath("root/B.h")] = R"cpp(
       #include "A.h"
       void new_func();
       )cpp";
   // Check that B.h has been stored with new contents.
   {
     CacheHits = 0;
     OverlayCDB CDB(/*Base=*/nullptr);
-    BackgroundIndex Idx(Context::empty(), FS, CDB,
-                        [&](llvm::StringRef) { return &MSS; });
+    BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                        /*Opts=*/{});
     CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
     ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 3U);
   ShardHeader = MSS.loadShard(testPath("root/B.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_THAT(*ShardHeader->Symbols,
               Contains(AllOf(Named("new_func"), Declared(), Not(Defined()))));
 }
 
 TEST_F(BackgroundIndexTest, NoDotsInAbsPath) {
   MockFS 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; });
+  BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                      /*Opts=*/{});
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
 
   tooling::CompileCommand Cmd;
   FS.Files[testPath("root/A.cc")] = "";
   Cmd.Filename = "../A.cc";
   Cmd.Directory = testPath("root/build");
   Cmd.CommandLine = {"clang++", "../A.cc"};
   CDB.setCompileCommand(testPath("root/build/../A.cc"), Cmd);

 }
 
 TEST_F(BackgroundIndexTest, UncompilableFiles) {
   MockFS 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; });
+  BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                      /*Opts=*/{});
 
   tooling::CompileCommand Cmd;
   FS.Files[testPath("A.h")] = "void foo();";
   FS.Files[testPath("B.h")] = "#include \"C.h\"\nasdf;";
   FS.Files[testPath("C.h")] = "";
   FS.Files[testPath("A.cc")] = R"cpp(
   #include "A.h"
   #include "B.h"

 }
 
 TEST_F(BackgroundIndexTest, CmdLineHash) {
   MockFS 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; });
+  BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
+                      /*Opts=*/{});
 
   tooling::CompileCommand Cmd;
   FS.Files[testPath("A.cc")] = "#include \"A.h\"";
   FS.Files[testPath("A.h")] = "";
   Cmd.Filename = "../A.cc";
   Cmd.Directory = testPath("build");
   Cmd.CommandLine = {"clang++", "../A.cc", "-fsyntax-only"};
   CDB.setCompileCommand(testPath("build/../A.cc"), Cmd);