[clang-tools-extra] r349496 - [clangd] BackgroundIndex rebuilds symbol index periodically.
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Mon Jan 7 03:21:48 PST 2019
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190107/aeaf5c52/attachment-0001.html>
More information about the cfe-commits
mailing list