From e6ce8da0252aba2642eb7879e1ff87beba568f67 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Thu, 30 Aug 2018 15:07:34 +0000 Subject: [PATCH] [clangd] Run SignatureHelp using an up-to-date preamble, waiting if needed. Summary: After code completion inserts a header, running signature help using the old preamble will usually fail. So we add support for consistent preamble reads. Reviewers: ilya-biryukov Subscribers: javed.absar, ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits Differential Revision: https://reviews.llvm.org/D51438 llvm-svn: 341076 --- clang-tools-extra/clangd/ClangdServer.cpp | 9 ++- clang-tools-extra/clangd/TUScheduler.cpp | 66 +++++++++++++++-- clang-tools-extra/clangd/TUScheduler.h | 31 +++++--- .../unittests/clangd/TUSchedulerTests.cpp | 85 +++++++++++++++++++--- 4 files changed, 158 insertions(+), 33 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 708a276..6bdd082 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -230,7 +230,8 @@ TaskHandle ClangdServer::codeComplete(PathRef File, Position Pos, // is called as soon as results are available. }; - WorkScheduler.runWithPreamble("CodeComplete", File, + // We use a potentially-stale preamble because latency is critical here. + WorkScheduler.runWithPreamble("CodeComplete", File, TUScheduler::Stale, Bind(Task, File.str(), std::move(CB))); return TH; } @@ -252,7 +253,11 @@ void ClangdServer::signatureHelp(PathRef File, Position Pos, IP->Contents, Pos, FS, PCHs, Index)); }; - WorkScheduler.runWithPreamble("SignatureHelp", File, + // Unlike code completion, we wait for an up-to-date preamble here. + // Signature help is often triggered after code completion. If the code + // completion inserted a header to make the symbol available, then using + // the old preamble would yield useless results. + WorkScheduler.runWithPreamble("SignatureHelp", File, TUScheduler::Consistent, Bind(Action, File.str(), std::move(CB))); } diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp index 8db8b2c..48dc445 100644 --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -183,6 +183,10 @@ public: bool blockUntilIdle(Deadline Timeout) const; std::shared_ptr getPossiblyStalePreamble() const; + /// Obtain a preamble reflecting all updates so far. Threadsafe. + /// It may be delivered immediately, or later on the worker thread. + void getCurrentPreamble( + llvm::unique_function)>); /// Wait for the first build of preamble to finish. Preamble itself can be /// accessed via getPossibleStalePreamble(). Note that this function will /// return after an unsuccessful build of the preamble too, i.e. result of @@ -464,6 +468,34 @@ ASTWorker::getPossiblyStalePreamble() const { return LastBuiltPreamble; } +void ASTWorker::getCurrentPreamble( + llvm::unique_function)> Callback) { + // We could just call startTask() to throw the read on the queue, knowing + // it will run after any updates. But we know this task is cheap, so to + // improve latency we cheat: insert it on the queue after the last update. + std::unique_lock Lock(Mutex); + auto LastUpdate = + std::find_if(Requests.rbegin(), Requests.rend(), + [](const Request &R) { return R.UpdateType.hasValue(); }); + // If there were no writes in the queue, the preamble is ready now. + if (LastUpdate == Requests.rend()) { + Lock.unlock(); + return Callback(getPossiblyStalePreamble()); + } + assert(!RunSync && "Running synchronously, but queue is non-empty!"); + Requests.insert(LastUpdate.base(), + Request{Bind( + [this](decltype(Callback) Callback) { + Callback(getPossiblyStalePreamble()); + }, + std::move(Callback)), + "GetPreamble", steady_clock::now(), + Context::current().clone(), + /*UpdateType=*/llvm::None}); + Lock.unlock(); + RequestsCV.notify_all(); +} + void ASTWorker::waitForFirstPreamble() const { PreambleWasBuilt.wait(); } @@ -711,7 +743,7 @@ void TUScheduler::runWithAST( } void TUScheduler::runWithPreamble( - llvm::StringRef Name, PathRef File, + llvm::StringRef Name, PathRef File, PreambleConsistency Consistency, llvm::unique_function)> Action) { auto It = Files.find(File); if (It == Files.end()) { @@ -731,22 +763,40 @@ void TUScheduler::runWithPreamble( return; } + // Future is populated if the task needs a specific preamble. + std::future> ConsistentPreamble; + if (Consistency == Consistent) { + std::promise> Promise; + ConsistentPreamble = Promise.get_future(); + It->second->Worker->getCurrentPreamble(Bind( + [](decltype(Promise) Promise, + std::shared_ptr Preamble) { + Promise.set_value(std::move(Preamble)); + }, + std::move(Promise))); + } + std::shared_ptr Worker = It->second->Worker.lock(); auto Task = [Worker, this](std::string Name, std::string File, std::string Contents, tooling::CompileCommand Command, Context Ctx, + decltype(ConsistentPreamble) ConsistentPreamble, decltype(Action) Action) mutable { - // We don't want to be running preamble actions before the preamble was - // built for the first time. This avoids extra work of processing the - // preamble headers in parallel multiple times. - Worker->waitForFirstPreamble(); + std::shared_ptr Preamble; + if (ConsistentPreamble.valid()) { + Preamble = ConsistentPreamble.get(); + } else { + // We don't want to be running preamble actions before the preamble was + // built for the first time. This avoids extra work of processing the + // preamble headers in parallel multiple times. + Worker->waitForFirstPreamble(); + Preamble = Worker->getPossiblyStalePreamble(); + } std::lock_guard BarrierLock(Barrier); WithContext Guard(std::move(Ctx)); trace::Span Tracer(Name); SPAN_ATTACH(Tracer, "file", File); - std::shared_ptr Preamble = - Worker->getPossiblyStalePreamble(); Action(InputsAndPreamble{Contents, Command, Preamble.get()}); }; @@ -755,7 +805,7 @@ void TUScheduler::runWithPreamble( Bind(Task, std::string(Name), std::string(File), It->second->Contents, It->second->Command, Context::current().derive(kFileBeingProcessed, File), - std::move(Action))); + std::move(ConsistentPreamble), std::move(Action))); } std::vector> diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h index edb19ee..3ef17c6 100644 --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -119,19 +119,28 @@ public: void runWithAST(llvm::StringRef Name, PathRef File, Callback Action); - /// Schedule an async read of the Preamble. - /// The preamble may be stale, generated from an older version of the file. - /// Reading from locations in the preamble may cause the files to be re-read. - /// This gives callers two options: - /// - validate that the preamble is still valid, and only use it in this case - /// - accept that preamble contents may be outdated, and try to avoid reading - /// source code from headers. + /// Controls whether preamble reads wait for the preamble to be up-to-date. + enum PreambleConsistency { + /// The preamble is generated from the current version of the file. + /// If the content was recently updated, we will wait until we have a + /// preamble that reflects that update. + /// This is the slowest option, and may be delayed by other tasks. + Consistent, + /// The preamble may be generated from an older version of the file. + /// Reading from locations in the preamble may cause files to be re-read. + /// This gives callers two options: + /// - validate that the preamble is still valid, and only use it if so + /// - accept that the preamble contents may be outdated, and try to avoid + /// reading source code from headers. + /// This is the fastest option, usually a preamble is available immediately. + Stale, + }; + /// Schedule an async read of the preamble. /// If there's no preamble yet (because the file was just opened), we'll wait - /// for it to build. The preamble may still be null if it fails to build or is - /// empty. - /// If an error occurs during processing, it is forwarded to the \p Action - /// callback. + /// for it to build. The result may be null if it fails to build or is empty. + /// If an error occurs, it is forwarded to the \p Action callback. void runWithPreamble(llvm::StringRef Name, PathRef File, + PreambleConsistency Consistency, Callback Action); /// Wait until there are no scheduled or running tasks. diff --git a/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp b/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp index d51500d..5222d79 100644 --- a/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp +++ b/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp @@ -22,6 +22,7 @@ namespace { using ::testing::_; using ::testing::AnyOf; using ::testing::Each; +using ::testing::ElementsAre; using ::testing::Pair; using ::testing::Pointee; using ::testing::UnorderedElementsAre; @@ -63,7 +64,7 @@ TEST_F(TUSchedulerTests, MissingFiles) { ASSERT_FALSE(bool(AST)); ignoreError(AST.takeError()); }); - S.runWithPreamble("", Missing, + S.runWithPreamble("", Missing, TUScheduler::Stale, [&](llvm::Expected Preamble) { ASSERT_FALSE(bool(Preamble)); ignoreError(Preamble.takeError()); @@ -75,9 +76,10 @@ TEST_F(TUSchedulerTests, MissingFiles) { S.runWithAST("", Added, [&](llvm::Expected AST) { EXPECT_TRUE(bool(AST)); }); - S.runWithPreamble("", Added, [&](llvm::Expected Preamble) { - EXPECT_TRUE(bool(Preamble)); - }); + S.runWithPreamble("", Added, TUScheduler::Stale, + [&](llvm::Expected Preamble) { + EXPECT_TRUE(bool(Preamble)); + }); S.remove(Added); // Assert that all operations fail after removing the file. @@ -85,10 +87,11 @@ TEST_F(TUSchedulerTests, MissingFiles) { ASSERT_FALSE(bool(AST)); ignoreError(AST.takeError()); }); - S.runWithPreamble("", Added, [&](llvm::Expected Preamble) { - ASSERT_FALSE(bool(Preamble)); - ignoreError(Preamble.takeError()); - }); + S.runWithPreamble("", Added, TUScheduler::Stale, + [&](llvm::Expected Preamble) { + ASSERT_FALSE(bool(Preamble)); + ignoreError(Preamble.takeError()); + }); // remove() shouldn't crash on missing files. S.remove(Added); } @@ -148,6 +151,64 @@ TEST_F(TUSchedulerTests, Debounce) { EXPECT_EQ(2, CallbackCount); } +static std::vector includes(const PreambleData *Preamble) { + std::vector Result; + if (Preamble) + for (const auto &Inclusion : Preamble->Includes.MainFileIncludes) + Result.push_back(Inclusion.Written); + return Result; +} + +TEST_F(TUSchedulerTests, PreambleConsistency) { + std::atomic CallbackCount(0); + { + Notification InconsistentReadDone; // Must live longest. + TUScheduler S( + getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true, + noopParsingCallbacks(), + /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), + ASTRetentionPolicy()); + auto Path = testPath("foo.cpp"); + // Schedule two updates (A, B) and two preamble reads (stale, consistent). + // The stale read should see A, and the consistent read should see B. + // (We recognize the preambles by their included files). + S.update(Path, getInputs(Path, "#include "), WantDiagnostics::Yes, + [&](std::vector Diags) { + // This callback runs in between the two preamble updates. + + // This blocks update B, preventing it from winning the race + // against the stale read. + // If the first read was instead consistent, this would deadlock. + InconsistentReadDone.wait(); + // This delays update B, preventing it from winning a race + // against the consistent read. The consistent read sees B + // only because it waits for it. + // If the second read was stale, it would usually see A. + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + }); + S.update(Path, getInputs(Path, "#include "), WantDiagnostics::Yes, + [&](std::vector Diags) {}); + + S.runWithPreamble("StaleRead", Path, TUScheduler::Stale, + [&](llvm::Expected Pre) { + ASSERT_TRUE(bool(Pre)); + assert(bool(Pre)); + EXPECT_THAT(includes(Pre->Preamble), + ElementsAre("")); + InconsistentReadDone.notify(); + ++CallbackCount; + }); + S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent, + [&](llvm::Expected Pre) { + ASSERT_TRUE(bool(Pre)); + EXPECT_THAT(includes(Pre->Preamble), + ElementsAre("")); + ++CallbackCount; + }); + } + EXPECT_EQ(2, CallbackCount); +} + TEST_F(TUSchedulerTests, ManyUpdates) { const int FilesCount = 3; const int UpdatesPerFile = 10; @@ -229,7 +290,7 @@ TEST_F(TUSchedulerTests, ManyUpdates) { { WithContextValue WithNonce(NonceKey, ++Nonce); S.runWithPreamble( - "CheckPreamble", File, + "CheckPreamble", File, TUScheduler::Stale, [File, Inputs, Nonce, &Mut, &TotalPreambleReads]( llvm::Expected Preamble) { EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce)); @@ -324,7 +385,7 @@ TEST_F(TUSchedulerTests, EmptyPreamble) { auto WithEmptyPreamble = R"cpp(int main() {})cpp"; S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto, [](std::vector) {}); - S.runWithPreamble("getNonEmptyPreamble", Foo, + S.runWithPreamble("getNonEmptyPreamble", Foo, TUScheduler::Stale, [&](llvm::Expected Preamble) { // We expect to get a non-empty preamble. EXPECT_GT(cantFail(std::move(Preamble)) @@ -340,7 +401,7 @@ TEST_F(TUSchedulerTests, EmptyPreamble) { [](std::vector) {}); // Wait for the preamble is being built. ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); - S.runWithPreamble("getEmptyPreamble", Foo, + S.runWithPreamble("getEmptyPreamble", Foo, TUScheduler::Stale, [&](llvm::Expected Preamble) { // We expect to get an empty preamble. EXPECT_EQ(cantFail(std::move(Preamble)) @@ -372,7 +433,7 @@ TEST_F(TUSchedulerTests, RunWaitsForPreamble) { [](std::vector) {}); for (int I = 0; I < ReadsToSchedule; ++I) { S.runWithPreamble( - "test", Foo, + "test", Foo, TUScheduler::Stale, [I, &PreamblesMut, &Preambles](llvm::Expected IP) { std::lock_guard Lock(PreamblesMut); Preambles[I] = cantFail(std::move(IP)).Preamble; -- 2.7.4