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@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@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
