[clang-tools-extra] r349496 - [clangd] BackgroundIndex rebuilds symbol index periodically.

Diana Picus via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 7 04:21:04 PST 2019


Thanks, Ilya!

On Mon, 7 Jan 2019 at 12:22, Ilya Biryukov <ibiryukov at google.com> wrote:
>
> Temporarily disabled the test in r350512.
>
> On Mon, Jan 7, 2019 at 10:54 AM Diana Picus <diana.picus at linaro.org> wrote:
>>
>> Hi Eric,
>>
>> This is still failing intermittently on AArch64 in spite of your and
>> Ilya's timeout increases. Could you please revert and rework this
>> test?
>>
>> Thanks,
>> Diana
>>
>> http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/5872
>> http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/5878
>> http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/5879
>>
>> On Tue, 18 Dec 2018 at 16:42, Eric Liu via cfe-commits
>> <cfe-commits at lists.llvm.org> wrote:
>> >
>> > Author: ioeric
>> > Date: Tue Dec 18 07:39:33 2018
>> > New Revision: 349496
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=349496&view=rev
>> > Log:
>> > [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
>> >
>> > Modified:
>> >     clang-tools-extra/trunk/clangd/ClangdServer.cpp
>> >     clang-tools-extra/trunk/clangd/ClangdServer.h
>> >     clang-tools-extra/trunk/clangd/index/Background.cpp
>> >     clang-tools-extra/trunk/clangd/index/Background.h
>> >     clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
>> >     clang-tools-extra/trunk/test/clangd/background-index.test
>> >     clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
>> >
>> > Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
>> > URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=349496&r1=349495&r2=349496&view=diff
>> > ==============================================================================
>> > --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
>> > +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Tue Dec 18 07:39:33 2018
>> > @@ -139,7 +139,8 @@ ClangdServer::ClangdServer(const GlobalC
>> >    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)
>> >
>> > Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
>> > URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=349496&r1=349495&r2=349496&view=diff
>> > ==============================================================================
>> > --- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
>> > +++ clang-tools-extra/trunk/clangd/ClangdServer.h Tue Dec 18 07:39:33 2018
>> > @@ -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;
>> >
>> > Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
>> > URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=349496&r1=349495&r2=349496&view=diff
>> > ==============================================================================
>> > --- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
>> > +++ clang-tools-extra/trunk/clangd/index/Background.cpp Tue Dec 18 07:39:33 2018
>> > @@ -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<F
>> >  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 M
>> >    }
>> >  }
>> >
>> > +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::Co
>> >      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::Co
>> >      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();
>> >  }
>> >
>> > Modified: clang-tools-extra/trunk/clangd/index/Background.h
>> > URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=349496&r1=349495&r2=349496&view=diff
>> > ==============================================================================
>> > --- clang-tools-extra/trunk/clangd/index/Background.h (original)
>> > +++ clang-tools-extra/trunk/clangd/index/Background.h Tue Dec 18 07:39:33 2018
>> > @@ -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.
>> >
>> > Modified: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
>> > URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp?rev=349496&r1=349495&r2=349496&view=diff
>> > ==============================================================================
>> > --- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp (original)
>> > +++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp Tue Dec 18 07:39:33 2018
>> > @@ -170,6 +170,14 @@ static cl::opt<bool> EnableBackgroundInd
>> >               "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()) {
>> >
>> > Modified: clang-tools-extra/trunk/test/clangd/background-index.test
>> > URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/background-index.test?rev=349496&r1=349495&r2=349496&view=diff
>> > ==============================================================================
>> > --- clang-tools-extra/trunk/test/clangd/background-index.test (original)
>> > +++ clang-tools-extra/trunk/test/clangd/background-index.test Tue Dec 18 07:39:33 2018
>> > @@ -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
>> >
>> > Modified: clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
>> > URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp?rev=349496&r1=349495&r2=349496&view=diff
>> > ==============================================================================
>> > --- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp (original)
>> > +++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp Tue Dec 18 07:39:33 2018
>> > @@ -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, DirectInclud
>> >                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
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
> --
> Regards,
> Ilya Biryukov


More information about the cfe-commits mailing list