[clang-tools-extra] r347450 - [clangd] Respect task cancellation in TUScheduler.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 22 02:22:16 PST 2018
Author: sammccall
Date: Thu Nov 22 02:22:16 2018
New Revision: 347450
URL: http://llvm.org/viewvc/llvm-project?rev=347450&view=rev
Log:
[clangd] Respect task cancellation in TUScheduler.
Summary:
- Reads are never executed if canceled before ready-to run.
In practice, we finalize cancelled reads eagerly and out-of-order.
- Cancelled reads don't prevent prior updates from being elided, as they don't
actually depend on the result of the update.
- Updates are downgraded from WantDiagnostics::Yes to WantDiagnostics::Auto when
cancelled, which allows them to be elided when all dependent reads are
cancelled and there are subsequent writes. (e.g. when the queue is backed up
with cancelled requests).
The queue operations aren't optimal (we scan the whole queue for cancelled
tasks every time the scheduler runs, and check cancellation twice in the end).
However I believe these costs are still trivial in practice (compared to any
AST operation) and the logic can be cleanly separated from the rest of the
scheduler.
Reviewers: ilya-biryukov
Subscribers: javed.absar, ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits
Differential Revision: https://reviews.llvm.org/D54746
Modified:
clang-tools-extra/trunk/clangd/Cancellation.cpp
clang-tools-extra/trunk/clangd/Cancellation.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Modified: clang-tools-extra/trunk/clangd/Cancellation.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Cancellation.cpp?rev=347450&r1=347449&r2=347450&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Cancellation.cpp (original)
+++ clang-tools-extra/trunk/clangd/Cancellation.cpp Thu Nov 22 02:22:16 2018
@@ -24,8 +24,8 @@ std::pair<Context, Canceler> cancelableT
};
}
-bool isCancelled() {
- if (auto *Flag = Context::current().get(FlagKey))
+bool isCancelled(const Context &Ctx) {
+ if (auto *Flag = Ctx.get(FlagKey))
return **Flag;
return false; // Not in scope of a task.
}
Modified: clang-tools-extra/trunk/clangd/Cancellation.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Cancellation.h?rev=347450&r1=347449&r2=347450&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Cancellation.h (original)
+++ clang-tools-extra/trunk/clangd/Cancellation.h Thu Nov 22 02:22:16 2018
@@ -79,7 +79,7 @@ std::pair<Context, Canceler> cancelableT
/// True if the current context is within a cancelable task which was cancelled.
/// Always false if there is no active cancelable task.
/// This isn't free (context lookup) - don't call it in a tight loop.
-bool isCancelled();
+bool isCancelled(const Context &Ctx = Context::current());
/// Conventional error when no result is returned due to cancellation.
class CancelledError : public llvm::ErrorInfo<CancelledError> {
Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=347450&r1=347449&r2=347450&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Thu Nov 22 02:22:16 2018
@@ -43,6 +43,7 @@
// immediately.
#include "TUScheduler.h"
+#include "Cancellation.h"
#include "Logger.h"
#include "Trace.h"
#include "clang/Frontend/CompilerInvocation.h"
@@ -432,6 +433,8 @@ void ASTWorker::update(ParseInputs Input
void ASTWorker::runWithAST(
StringRef Name, unique_function<void(Expected<InputsAndAST>)> Action) {
auto Task = [=](decltype(Action) Action) {
+ if (isCancelled())
+ return Action(make_error<CancelledError>());
Optional<std::unique_ptr<ParsedAST>> AST = IdleASTs.take(this);
if (!AST) {
std::unique_ptr<CompilerInvocation> Invocation =
@@ -588,6 +591,26 @@ void ASTWorker::run() {
Deadline ASTWorker::scheduleLocked() {
if (Requests.empty())
return Deadline::infinity(); // Wait for new requests.
+ // Handle cancelled requests first so the rest of the scheduler doesn't.
+ for (auto I = Requests.begin(), E = Requests.end(); I != E; ++I) {
+ if (!isCancelled(I->Ctx)) {
+ // Cancellations after the first read don't affect current scheduling.
+ if (I->UpdateType == None)
+ break;
+ continue;
+ }
+ // Cancelled reads are moved to the front of the queue and run immediately.
+ if (I->UpdateType == None) {
+ Request R = std::move(*I);
+ Requests.erase(I);
+ Requests.push_front(std::move(R));
+ return Deadline::zero();
+ }
+ // Cancelled updates are downgraded to auto-diagnostics, and may be elided.
+ if (I->UpdateType == WantDiagnostics::Yes)
+ I->UpdateType = WantDiagnostics::Auto;
+ }
+
while (shouldSkipHeadLocked())
Requests.pop_front();
assert(!Requests.empty() && "skipped the whole queue");
Modified: clang-tools-extra/trunk/clangd/TUScheduler.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.h?rev=347450&r1=347449&r2=347450&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.h (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.h Thu Nov 22 02:22:16 2018
@@ -100,6 +100,9 @@ public:
/// Schedule an update for \p File. Adds \p File to a list of tracked files if
/// \p File was not part of it before.
+ /// If diagnostics are requested (Yes), and the context is cancelled before
+ /// they are prepared, they may be skipped if eventual-consistency permits it
+ /// (i.e. WantDiagnostics is downgraded to Auto).
/// FIXME(ibiryukov): remove the callback from this function.
void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD,
llvm::unique_function<void(std::vector<Diag>)> OnUpdated);
@@ -117,6 +120,8 @@ public:
/// \p Action is executed.
/// If an error occurs during processing, it is forwarded to the \p Action
/// callback.
+ /// If the context is cancelled before the AST is ready, the callback will
+ /// receive a CancelledError.
void runWithAST(llvm::StringRef Name, PathRef File,
Callback<InputsAndAST> Action);
@@ -140,6 +145,8 @@ public:
/// If there's no preamble yet (because the file was just opened), we'll wait
/// 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.
+ /// Context cancellation is ignored and should be handled by the Action.
+ /// (In practice, the Action is almost always executed immediately).
void runWithPreamble(llvm::StringRef Name, PathRef File,
PreambleConsistency Consistency,
Callback<InputsAndPreamble> Action);
Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=347450&r1=347449&r2=347450&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Thu Nov 22 02:22:16 2018
@@ -209,6 +209,75 @@ TEST_F(TUSchedulerTests, PreambleConsist
EXPECT_EQ(2, CallbackCount);
}
+TEST_F(TUSchedulerTests, Cancellation) {
+ // We have the following update/read sequence
+ // U0
+ // U1(WantDiags=Yes) <-- cancelled
+ // R1 <-- cancelled
+ // U2(WantDiags=Yes) <-- cancelled
+ // R2A <-- cancelled
+ // R2B
+ // U3(WantDiags=Yes)
+ // R3 <-- cancelled
+ std::vector<std::string> DiagsSeen, ReadsSeen, ReadsCanceled;
+ {
+ TUScheduler S(
+ getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+ /*ASTCallbacks=*/nullptr,
+ /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ ASTRetentionPolicy());
+ auto Path = testPath("foo.cpp");
+ // Helper to schedule a named update and return a function to cancel it.
+ auto Update = [&](std::string ID) -> Canceler {
+ auto T = cancelableTask();
+ WithContext C(std::move(T.first));
+ S.update(Path, getInputs(Path, "//" + ID), WantDiagnostics::Yes,
+ [&, ID](std::vector<Diag> Diags) { DiagsSeen.push_back(ID); });
+ return std::move(T.second);
+ };
+ // Helper to schedule a named read and return a function to cancel it.
+ auto Read = [&](std::string ID) -> Canceler {
+ auto T = cancelableTask();
+ WithContext C(std::move(T.first));
+ S.runWithAST(ID, Path, [&, ID](llvm::Expected<InputsAndAST> E) {
+ if (auto Err = E.takeError()) {
+ if (Err.isA<CancelledError>()) {
+ ReadsCanceled.push_back(ID);
+ consumeError(std::move(Err));
+ } else {
+ ADD_FAILURE() << "Non-cancelled error for " << ID << ": "
+ << llvm::toString(std::move(Err));
+ }
+ } else {
+ ReadsSeen.push_back(ID);
+ }
+ });
+ return std::move(T.second);
+ };
+
+ Notification Proceed; // Ensure we schedule everything.
+ S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
+ [&](std::vector<Diag> Diags) { Proceed.wait(); });
+ // The second parens indicate cancellation, where present.
+ Update("U1")();
+ Read("R1")();
+ Update("U2")();
+ Read("R2A")();
+ Read("R2B");
+ Update("U3");
+ Read("R3")();
+ Proceed.notify();
+ }
+ EXPECT_THAT(DiagsSeen, ElementsAre("U2", "U3"))
+ << "U1 and all dependent reads were cancelled. "
+ "U2 has a dependent read R2A. "
+ "U3 was not cancelled.";
+ EXPECT_THAT(ReadsSeen, ElementsAre("R2B"))
+ << "All reads other than R2B were cancelled";
+ EXPECT_THAT(ReadsCanceled, ElementsAre("R1", "R2A", "R3"))
+ << "All reads other than R2B were cancelled";
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
More information about the cfe-commits
mailing list