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
