<div dir="ltr"><div dir="ltr">Temporarily disabled the test in r350512.</div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jan 7, 2019 at 10:54 AM Diana Picus <<a href="mailto:diana.picus@linaro.org">diana.picus@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Eric,<br>
<br>
This is still failing intermittently on AArch64 in spite of your and<br>
Ilya's timeout increases. Could you please revert and rework this<br>
test?<br>
<br>
Thanks,<br>
Diana<br>
<br>
<a href="http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/5872" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/5872</a><br>
<a href="http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/5878" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/5878</a><br>
<a href="http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/5879" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/5879</a><br>
<br>
On Tue, 18 Dec 2018 at 16:42, Eric Liu via cfe-commits<br>
<<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Author: ioeric<br>
> Date: Tue Dec 18 07:39:33 2018<br>
> New Revision: 349496<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=349496&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=349496&view=rev</a><br>
> Log:<br>
> [clangd] BackgroundIndex rebuilds symbol index periodically.<br>
><br>
> Summary:<br>
> Currently, background index rebuilds symbol index on every indexed file,<br>
> which can be inefficient. This patch makes it only rebuild symbol index periodically.<br>
> As the rebuild no longer happens too often, we could also build more efficient<br>
> dex index.<br>
><br>
> Reviewers: ilya-biryukov, kadircet<br>
><br>
> Reviewed By: kadircet<br>
><br>
> Subscribers: dblaikie, MaskRay, jkorous, arphaman, jfb, cfe-commits<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D55770" rel="noreferrer" target="_blank">https://reviews.llvm.org/D55770</a><br>
><br>
> Modified:<br>
> clang-tools-extra/trunk/clangd/ClangdServer.cpp<br>
> clang-tools-extra/trunk/clangd/ClangdServer.h<br>
> clang-tools-extra/trunk/clangd/index/Background.cpp<br>
> clang-tools-extra/trunk/clangd/index/Background.h<br>
> clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp<br>
> clang-tools-extra/trunk/test/clangd/background-index.test<br>
> clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp<br>
><br>
> Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=349496&r1=349495&r2=349496&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=349496&r1=349495&r2=349496&view=diff</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)<br>
> +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Tue Dec 18 07:39:33 2018<br>
> @@ -139,7 +139,8 @@ ClangdServer::ClangdServer(const GlobalC<br>
> if (Opts.BackgroundIndex) {<br>
> BackgroundIdx = llvm::make_unique<BackgroundIndex>(<br>
> Context::current().clone(), ResourceDir, FSProvider, CDB,<br>
> - BackgroundIndexStorage::createDiskBackedStorageFactory());<br>
> + BackgroundIndexStorage::createDiskBackedStorageFactory(),<br>
> + Opts.BackgroundIndexRebuildPeriodMs);<br>
> AddIndex(BackgroundIdx.get());<br>
> }<br>
> if (DynamicIdx)<br>
><br>
> Modified: clang-tools-extra/trunk/clangd/ClangdServer.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=349496&r1=349495&r2=349496&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=349496&r1=349495&r2=349496&view=diff</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/clangd/ClangdServer.h (original)<br>
> +++ clang-tools-extra/trunk/clangd/ClangdServer.h Tue Dec 18 07:39:33 2018<br>
> @@ -85,6 +85,10 @@ public:<br>
> /// If true, ClangdServer automatically indexes files in the current project<br>
> /// on background threads. The index is stored in the project root.<br>
> bool BackgroundIndex = false;<br>
> + /// If set to non-zero, the background index rebuilds the symbol index<br>
> + /// periodically every BuildIndexPeriodMs milliseconds; otherwise, the<br>
> + /// symbol index will be updated for each indexed file.<br>
> + size_t BackgroundIndexRebuildPeriodMs = 0;<br>
><br>
> /// If set, use this index to augment code completion results.<br>
> SymbolIndex *StaticIndex = nullptr;<br>
><br>
> Modified: clang-tools-extra/trunk/clangd/index/Background.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=349496&r1=349495&r2=349496&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=349496&r1=349495&r2=349496&view=diff</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/clangd/index/Background.cpp (original)<br>
> +++ clang-tools-extra/trunk/clangd/index/Background.cpp Tue Dec 18 07:39:33 2018<br>
> @@ -27,11 +27,13 @@<br>
> #include "llvm/ADT/StringRef.h"<br>
> #include "llvm/Support/SHA1.h"<br>
><br>
> +#include <chrono><br>
> #include <memory><br>
> #include <numeric><br>
> #include <queue><br>
> #include <random><br>
> #include <string><br>
> +#include <thread><br>
><br>
> using namespace llvm;<br>
> namespace clang {<br>
> @@ -125,10 +127,13 @@ createFileFilter(const llvm::StringMap<F<br>
> BackgroundIndex::BackgroundIndex(<br>
> Context BackgroundContext, StringRef ResourceDir,<br>
> const FileSystemProvider &FSProvider, const GlobalCompilationDatabase &CDB,<br>
> - BackgroundIndexStorage::Factory IndexStorageFactory, size_t ThreadPoolSize)<br>
> + BackgroundIndexStorage::Factory IndexStorageFactory,<br>
> + size_t BuildIndexPeriodMs, size_t ThreadPoolSize)<br>
> : SwapIndex(make_unique<MemIndex>()), ResourceDir(ResourceDir),<br>
> FSProvider(FSProvider), CDB(CDB),<br>
> BackgroundContext(std::move(BackgroundContext)),<br>
> + BuildIndexPeriodMs(BuildIndexPeriodMs),<br>
> + SymbolsUpdatedSinceLastIndex(false),<br>
> IndexStorageFactory(std::move(IndexStorageFactory)),<br>
> CommandsChanged(<br>
> CDB.watch([&](const std::vector<std::string> &ChangedFiles) {<br>
> @@ -138,6 +143,11 @@ BackgroundIndex::BackgroundIndex(<br>
> assert(this->IndexStorageFactory && "Storage factory can not be null!");<br>
> while (ThreadPoolSize--)<br>
> ThreadPool.emplace_back([this] { run(); });<br>
> + if (BuildIndexPeriodMs > 0) {<br>
> + log("BackgroundIndex: build symbol index periodically every {0} ms.",<br>
> + BuildIndexPeriodMs);<br>
> + ThreadPool.emplace_back([this] { buildIndex(); });<br>
> + }<br>
> }<br>
><br>
> BackgroundIndex::~BackgroundIndex() {<br>
> @@ -148,10 +158,12 @@ BackgroundIndex::~BackgroundIndex() {<br>
><br>
> void BackgroundIndex::stop() {<br>
> {<br>
> - std::lock_guard<std::mutex> Lock(QueueMu);<br>
> + std::lock_guard<std::mutex> QueueLock(QueueMu);<br>
> + std::lock_guard<std::mutex> IndexLock(IndexMu);<br>
> ShouldStop = true;<br>
> }<br>
> QueueCV.notify_all();<br>
> + IndexCV.notify_all();<br>
> }<br>
><br>
> void BackgroundIndex::run() {<br>
> @@ -332,6 +344,30 @@ void BackgroundIndex::update(StringRef M<br>
> }<br>
> }<br>
><br>
> +void BackgroundIndex::buildIndex() {<br>
> + assert(BuildIndexPeriodMs > 0);<br>
> + while (true) {<br>
> + {<br>
> + std::unique_lock<std::mutex> Lock(IndexMu);<br>
> + if (ShouldStop) // Avoid waiting if stopped.<br>
> + break;<br>
> + // Wait until this is notified to stop or `BuildIndexPeriodMs` has past.<br>
> + IndexCV.wait_for(Lock, std::chrono::milliseconds(BuildIndexPeriodMs));<br>
> + if (ShouldStop) // Avoid rebuilding index if stopped.<br>
> + break;<br>
> + }<br>
> + if (!SymbolsUpdatedSinceLastIndex.exchange(false))<br>
> + continue;<br>
> + // There can be symbol update right after the flag is reset above and before<br>
> + // index is rebuilt below. The new index would contain the updated symbols<br>
> + // but the flag would still be true. This is fine as we would simply run an<br>
> + // extra index build.<br>
> + reset(<br>
> + IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));<br>
> + log("BackgroundIndex: rebuilt symbol index.");<br>
> + }<br>
> +}<br>
> +<br>
> Error BackgroundIndex::index(tooling::CompileCommand Cmd,<br>
> BackgroundIndexStorage *IndexStorage) {<br>
> trace::Span Tracer("BackgroundIndex");<br>
> @@ -362,7 +398,7 @@ Error BackgroundIndex::index(tooling::Co<br>
> DigestsSnapshot = IndexedFileDigests;<br>
> }<br>
><br>
> - log("Indexing {0}", Cmd.Filename, toHex(Hash));<br>
> + log("Indexing {0} (digest:={1})", Cmd.Filename, toHex(Hash));<br>
> ParseInputs Inputs;<br>
> Inputs.FS = std::move(FS);<br>
> Inputs.FS->setCurrentWorkingDirectory(Cmd.Directory);<br>
> @@ -424,10 +460,11 @@ Error BackgroundIndex::index(tooling::Co<br>
> IndexedFileDigests[AbsolutePath] = Hash;<br>
> }<br>
><br>
> - // FIXME: this should rebuild once-in-a-while, not after every file.<br>
> - // At that point we should use Dex, too.<br>
> - vlog("Rebuilding automatic index");<br>
> - reset(IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));<br>
> + if (BuildIndexPeriodMs > 0)<br>
> + SymbolsUpdatedSinceLastIndex = true;<br>
> + else<br>
> + reset(<br>
> + IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));<br>
><br>
> return Error::success();<br>
> }<br>
><br>
> Modified: clang-tools-extra/trunk/clangd/index/Background.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=349496&r1=349495&r2=349496&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=349496&r1=349495&r2=349496&view=diff</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/clangd/index/Background.h (original)<br>
> +++ clang-tools-extra/trunk/clangd/index/Background.h Tue Dec 18 07:39:33 2018<br>
> @@ -21,8 +21,10 @@<br>
> #include "llvm/ADT/StringMap.h"<br>
> #include "llvm/Support/SHA1.h"<br>
> #include "llvm/Support/Threading.h"<br>
> +#include <atomic><br>
> #include <condition_variable><br>
> #include <deque><br>
> +#include <mutex><br>
> #include <string><br>
> #include <thread><br>
> #include <vector><br>
> @@ -63,11 +65,15 @@ public:<br>
> // FIXME: it should watch for changes to files on disk.<br>
> class BackgroundIndex : public SwapIndex {<br>
> public:<br>
> + /// If BuildIndexPeriodMs is greater than 0, the symbol index will only be<br>
> + /// rebuilt periodically (one per \p BuildIndexPeriodMs); otherwise, index is<br>
> + /// rebuilt for each indexed file.<br>
> // FIXME: resource-dir injection should be hoisted somewhere common.<br>
> BackgroundIndex(Context BackgroundContext, llvm::StringRef ResourceDir,<br>
> const FileSystemProvider &,<br>
> const GlobalCompilationDatabase &CDB,<br>
> BackgroundIndexStorage::Factory IndexStorageFactory,<br>
> + size_t BuildIndexPeriodMs = 0,<br>
> size_t ThreadPoolSize = llvm::hardware_concurrency());<br>
> ~BackgroundIndex(); // Blocks while the current task finishes.<br>
><br>
> @@ -101,6 +107,11 @@ private:<br>
> // index state<br>
> llvm::Error index(tooling::CompileCommand,<br>
> BackgroundIndexStorage *IndexStorage);<br>
> + void buildIndex(); // Rebuild index periodically every BuildIndexPeriodMs.<br>
> + const size_t BuildIndexPeriodMs;<br>
> + std::atomic<bool> SymbolsUpdatedSinceLastIndex;<br>
> + std::mutex IndexMu;<br>
> + std::condition_variable IndexCV;<br>
><br>
> FileSymbols IndexedSymbols;<br>
> llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path.<br>
><br>
> Modified: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp?rev=349496&r1=349495&r2=349496&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp?rev=349496&r1=349495&r2=349496&view=diff</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp (original)<br>
> +++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp Tue Dec 18 07:39:33 2018<br>
> @@ -170,6 +170,14 @@ static cl::opt<bool> EnableBackgroundInd<br>
> "Experimental"),<br>
> cl::init(false), cl::Hidden);<br>
><br>
> +static cl::opt<int> BackgroundIndexRebuildPeriod(<br>
> + "background-index-rebuild-period",<br>
> + cl::desc(<br>
> + "If set to non-zero, the background index rebuilds the symbol index "<br>
> + "periodically every X milliseconds; otherwise, the "<br>
> + "symbol index will be updated for each indexed file."),<br>
> + cl::init(5000), cl::Hidden);<br>
> +<br>
> enum CompileArgsFrom { LSPCompileArgs, FilesystemCompileArgs };<br>
> static cl::opt<CompileArgsFrom> CompileArgsFrom(<br>
> "compile_args_from", cl::desc("The source of compile commands"),<br>
> @@ -352,6 +360,7 @@ int main(int argc, char *argv[]) {<br>
> Opts.BuildDynamicSymbolIndex = EnableIndex;<br>
> Opts.HeavyweightDynamicSymbolIndex = UseDex;<br>
> Opts.BackgroundIndex = EnableBackgroundIndex;<br>
> + Opts.BackgroundIndexRebuildPeriodMs = BackgroundIndexRebuildPeriod;<br>
> std::unique_ptr<SymbolIndex> StaticIdx;<br>
> std::future<void> AsyncIndexLoad; // Block exit while loading the index.<br>
> if (EnableIndex && !IndexFile.empty()) {<br>
><br>
> Modified: clang-tools-extra/trunk/test/clangd/background-index.test<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/background-index.test?rev=349496&r1=349495&r2=349496&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/background-index.test?rev=349496&r1=349495&r2=349496&view=diff</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/test/clangd/background-index.test (original)<br>
> +++ clang-tools-extra/trunk/test/clangd/background-index.test Tue Dec 18 07:39:33 2018<br>
> @@ -10,7 +10,7 @@<br>
> # We're editing bar.cpp, which includes foo.h.<br>
> # foo() is declared in foo.h and defined in foo.cpp.<br>
> # The background index should allow us to go-to-definition on foo().<br>
> -# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc<br>
> +# RUN: clangd -background-index -background-index-rebuild-period=0 -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc<br>
><br>
> # Test that the index is writing files in the expected location.<br>
> # RUN: ls %t/.clangd-index/foo.cpp.*.idx<br>
><br>
> Modified: clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp?rev=349496&r1=349495&r2=349496&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp?rev=349496&r1=349495&r2=349496&view=diff</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp (original)<br>
> +++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp Tue Dec 18 07:39:33 2018<br>
> @@ -2,11 +2,14 @@<br>
> #include "TestFS.h"<br>
> #include "index/Background.h"<br>
> #include "llvm/Support/ScopedPrinter.h"<br>
> +#include "llvm/Support/Threading.h"<br>
> #include "gmock/gmock.h"<br>
> #include "gtest/gtest.h"<br>
> +#include <thread><br>
><br>
> using testing::_;<br>
> using testing::AllOf;<br>
> +using testing::ElementsAre;<br>
> using testing::Not;<br>
> using testing::UnorderedElementsAre;<br>
><br>
> @@ -240,5 +243,39 @@ TEST_F(BackgroundIndexTest, DirectInclud<br>
> EmptyIncludeNode());<br>
> }<br>
><br>
> +TEST_F(BackgroundIndexTest, PeriodicalIndex) {<br>
> + MockFSProvider FS;<br>
> + llvm::StringMap<std::string> Storage;<br>
> + size_t CacheHits = 0;<br>
> + MemoryShardStorage MSS(Storage, CacheHits);<br>
> + OverlayCDB CDB(/*Base=*/nullptr);<br>
> + BackgroundIndex Idx(<br>
> + Context::empty(), "", FS, CDB, [&](llvm::StringRef) { return &MSS; },<br>
> + /*BuildIndexPeriodMs=*/10);<br>
> +<br>
> + FS.Files[testPath("root/A.cc")] = "#include \"A.h\"";<br>
> +<br>
> + tooling::CompileCommand Cmd;<br>
> + FS.Files[testPath("root/A.h")] = "class X {};";<br>
> + Cmd.Filename = testPath("root/A.cc");<br>
> + Cmd.CommandLine = {"clang++", Cmd.Filename};<br>
> + CDB.setCompileCommand(testPath("root/A.cc"), Cmd);<br>
> +<br>
> + ASSERT_TRUE(Idx.blockUntilIdleForTest());<br>
> + EXPECT_THAT(runFuzzyFind(Idx, ""), ElementsAre());<br>
> + std::this_thread::sleep_for(std::chrono::milliseconds(15));<br>
> + EXPECT_THAT(runFuzzyFind(Idx, ""), ElementsAre(Named("X")));<br>
> +<br>
> + FS.Files[testPath("root/A.h")] = "class Y {};";<br>
> + FS.Files[testPath("root/A.cc")] += " "; // Force reindex the file.<br>
> + Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};<br>
> + CDB.setCompileCommand(testPath("root/A.cc"), Cmd);<br>
> +<br>
> + ASSERT_TRUE(Idx.blockUntilIdleForTest());<br>
> + EXPECT_THAT(runFuzzyFind(Idx, ""), ElementsAre(Named("X")));<br>
> + std::this_thread::sleep_for(std::chrono::milliseconds(15));<br>
> + EXPECT_THAT(runFuzzyFind(Idx, ""), ElementsAre(Named("Y")));<br>
> +}<br>
> +<br>
> } // namespace clangd<br>
> } // namespace clang<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>