Thanks, Ilya!
On Mon, 7 Jan 2019 at 12:22, Ilya Biryukov <ibiryu...@google.com> wrote: > > Temporarily disabled the test in r350512. > > On Mon, Jan 7, 2019 at 10:54 AM Diana Picus <diana.pi...@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@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 > > > > -- > Regards, > Ilya Biryukov _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits