[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