[clang-tools-extra] r349496 - [clangd] BackgroundIndex rebuilds symbol index periodically.
Diana Picus via cfe-commits
cfe-commits at lists.llvm.org
Mon Jan 7 01:54:36 PST 2019
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
More information about the cfe-commits
mailing list