[clang-tools-extra] c627b12 - [clangd] Cancel certain operations if the file changes before we start.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 4 15:12:13 PST 2020
Author: Sam McCall
Date: 2020-03-05T00:10:07+01:00
New Revision: c627b120eb8b7add3f9cf893721335c367d9f037
URL: https://github.com/llvm/llvm-project/commit/c627b120eb8b7add3f9cf893721335c367d9f037
DIFF: https://github.com/llvm/llvm-project/commit/c627b120eb8b7add3f9cf893721335c367d9f037.diff
LOG: [clangd] Cancel certain operations if the file changes before we start.
Summary:
Otherwise they can force us to build lots of snapshots that we don't need.
Particularly, try to do this for operations that are frequently
generated by editors without explicit user interaction, and where
editing the file makes the result less useful. (Code action
enumeration is a good example).
https://github.com/clangd/clangd/issues/298
This doesn't return the "right" LSP error code (ContentModified) to the client,
we need to teach the cancellation API to distinguish between different causes.
Reviewers: kadircet
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, jfb, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D75602
Added:
Modified:
clang-tools-extra/clangd/Cancellation.cpp
clang-tools-extra/clangd/Cancellation.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/JSONTransport.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/CancellationTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/Cancellation.cpp b/clang-tools-extra/clangd/Cancellation.cpp
index 6b0e21d34e77..ccd27a1452d1 100644
--- a/clang-tools-extra/clangd/Cancellation.cpp
+++ b/clang-tools-extra/clangd/Cancellation.cpp
@@ -13,20 +13,30 @@ namespace clang {
namespace clangd {
char CancelledError::ID = 0;
-static Key<std::shared_ptr<std::atomic<bool>>> FlagKey;
+
+// We don't want a cancelable scope to "shadow" an enclosing one.
+struct CancelState {
+ std::shared_ptr<std::atomic<bool>> Cancelled;
+ const CancelState *Parent;
+};
+static Key<CancelState> StateKey;
std::pair<Context, Canceler> cancelableTask() {
- auto Flag = std::make_shared<std::atomic<bool>>();
+ CancelState State;
+ State.Cancelled = std::make_shared<std::atomic<bool>>();
+ State.Parent = Context::current().get(StateKey);
return {
- Context::current().derive(FlagKey, Flag),
- [Flag] { *Flag = true; },
+ Context::current().derive(StateKey, State),
+ [Flag(State.Cancelled)] { *Flag = true; },
};
}
bool isCancelled(const Context &Ctx) {
- if (auto *Flag = Ctx.get(FlagKey))
- return **Flag;
- return false; // Not in scope of a task.
+ for (const CancelState *State = Ctx.get(StateKey); State != nullptr;
+ State = State->Parent)
+ if (State->Cancelled->load())
+ return true;
+ return false;
}
} // namespace clangd
diff --git a/clang-tools-extra/clangd/Cancellation.h b/clang-tools-extra/clangd/Cancellation.h
index 6872c448de86..bdb0bd4ef09f 100644
--- a/clang-tools-extra/clangd/Cancellation.h
+++ b/clang-tools-extra/clangd/Cancellation.h
@@ -76,6 +76,7 @@ using Canceler = std::function<void()>;
std::pair<Context, Canceler> cancelableTask();
/// True if the current context is within a cancelable task which was cancelled.
+/// (If the context is within multiple nested tasks, true if any are 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(const Context &Ctx = Context::current());
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 5dd00322a5ab..cd833af13e49 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -437,7 +437,8 @@ void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
CB(std::move(Res));
};
- WorkScheduler.runWithAST("EnumerateTweaks", File, std::move(Action));
+ WorkScheduler.runWithAST("EnumerateTweaks", File, std::move(Action),
+ TUScheduler::InvalidateOnUpdate);
}
void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
@@ -556,7 +557,8 @@ void ClangdServer::findDocumentHighlights(
CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
};
- WorkScheduler.runWithAST("Highlights", File, std::move(Action));
+ WorkScheduler.runWithAST("Highlights", File, std::move(Action),
+ TUScheduler::InvalidateOnUpdate);
}
void ClangdServer::findHover(PathRef File, Position Pos,
@@ -570,7 +572,8 @@ void ClangdServer::findHover(PathRef File, Position Pos,
CB(clangd::getHover(InpAST->AST, Pos, std::move(Style), Index));
};
- WorkScheduler.runWithAST("Hover", File, std::move(Action));
+ WorkScheduler.runWithAST("Hover", File, std::move(Action),
+ TUScheduler::InvalidateOnUpdate);
}
void ClangdServer::typeHierarchy(PathRef File, Position Pos, int Resolve,
@@ -618,7 +621,8 @@ void ClangdServer::documentSymbols(llvm::StringRef File,
return CB(InpAST.takeError());
CB(clangd::getDocumentSymbols(InpAST->AST));
};
- WorkScheduler.runWithAST("documentSymbols", File, std::move(Action));
+ WorkScheduler.runWithAST("documentSymbols", File, std::move(Action),
+ TUScheduler::InvalidateOnUpdate);
}
void ClangdServer::findReferences(PathRef File, Position Pos, uint32_t Limit,
@@ -664,7 +668,8 @@ void ClangdServer::documentLinks(PathRef File,
return CB(InpAST.takeError());
CB(clangd::getDocumentLinks(InpAST->AST));
};
- WorkScheduler.runWithAST("DocumentLinks", File, std::move(Action));
+ WorkScheduler.runWithAST("DocumentLinks", File, std::move(Action),
+ TUScheduler::InvalidateOnUpdate);
}
std::vector<std::pair<Path, std::size_t>>
diff --git a/clang-tools-extra/clangd/JSONTransport.cpp b/clang-tools-extra/clangd/JSONTransport.cpp
index 43be9f71aaf8..d82dca167f3b 100644
--- a/clang-tools-extra/clangd/JSONTransport.cpp
+++ b/clang-tools-extra/clangd/JSONTransport.cpp
@@ -19,6 +19,8 @@ namespace {
llvm::json::Object encodeError(llvm::Error E) {
std::string Message;
ErrorCode Code = ErrorCode::UnknownErrorCode;
+ // FIXME: encode cancellation errors using RequestCancelled or ContentModified
+ // as appropriate.
if (llvm::Error Unhandled = llvm::handleErrors(
std::move(E), [&](const LSPError &L) -> llvm::Error {
Message = L.Message;
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 3f3162a33c30..714bc7217b8b 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -184,7 +184,8 @@ class ASTWorker {
void update(ParseInputs Inputs, WantDiagnostics);
void
runWithAST(llvm::StringRef Name,
- llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action);
+ llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
+ TUScheduler::ASTActionInvalidation);
bool blockUntilIdle(Deadline Timeout) const;
std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
@@ -214,7 +215,8 @@ class ASTWorker {
void stop();
/// Adds a new task to the end of the request queue.
void startTask(llvm::StringRef Name, llvm::unique_function<void()> Task,
- llvm::Optional<WantDiagnostics> UpdateType);
+ llvm::Optional<WantDiagnostics> UpdateType,
+ TUScheduler::ASTActionInvalidation);
/// Updates the TUStatus and emits it. Only called in the worker thread.
void emitTUStatus(TUAction FAction,
const TUStatus::BuildDetails *Detail = nullptr);
@@ -237,6 +239,8 @@ class ASTWorker {
steady_clock::time_point AddTime;
Context Ctx;
llvm::Optional<WantDiagnostics> UpdateType;
+ TUScheduler::ASTActionInvalidation InvalidationPolicy;
+ Canceler Invalidate;
};
/// Handles retention of ASTs.
@@ -542,12 +546,13 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
// Stash the AST in the cache for further use.
IdleASTs.put(this, std::move(*AST));
};
- startTask(TaskName, std::move(Task), WantDiags);
+ startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation);
}
void ASTWorker::runWithAST(
llvm::StringRef Name,
- llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action) {
+ llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
+ TUScheduler::ASTActionInvalidation Invalidation) {
auto Task = [=, Action = std::move(Action)]() mutable {
if (isCancelled())
return Action(llvm::make_error<CancelledError>());
@@ -576,7 +581,7 @@ void ASTWorker::runWithAST(
"invalid AST", llvm::errc::invalid_argument));
Action(InputsAndAST{*CurrentInputs, **AST});
};
- startTask(Name, std::move(Task), /*UpdateType=*/None);
+ startTask(Name, std::move(Task), /*UpdateType=*/None, Invalidation);
}
std::shared_ptr<const PreambleData>
@@ -606,7 +611,9 @@ void ASTWorker::getCurrentPreamble(
},
"GetPreamble", steady_clock::now(),
Context::current().clone(),
- /*UpdateType=*/None});
+ /*UpdateType=*/None,
+ /*InvalidationPolicy=*/TUScheduler::NoInvalidation,
+ /*Invalidate=*/nullptr});
Lock.unlock();
RequestsCV.notify_all();
}
@@ -650,7 +657,8 @@ void ASTWorker::stop() {
void ASTWorker::startTask(llvm::StringRef Name,
llvm::unique_function<void()> Task,
- llvm::Optional<WantDiagnostics> UpdateType) {
+ llvm::Optional<WantDiagnostics> UpdateType,
+ TUScheduler::ASTActionInvalidation Invalidation) {
if (RunSync) {
assert(!Done && "running a task after stop()");
trace::Span Tracer(Name + ":" + llvm::sys::path::filename(FileName));
@@ -661,9 +669,26 @@ void ASTWorker::startTask(llvm::StringRef Name,
{
std::lock_guard<std::mutex> Lock(Mutex);
assert(!Done && "running a task after stop()");
- Requests.push_back(
- {std::move(Task), std::string(Name), steady_clock::now(),
- Context::current().derive(kFileBeingProcessed, FileName), UpdateType});
+ // Cancel any requests invalidated by this request.
+ if (UpdateType) {
+ for (auto &R : llvm::reverse(Requests)) {
+ if (R.InvalidationPolicy == TUScheduler::InvalidateOnUpdate)
+ R.Invalidate();
+ if (R.UpdateType)
+ break; // Older requests were already invalidated by the older update.
+ }
+ }
+
+ // Allow this request to be cancelled if invalidated.
+ Context Ctx = Context::current().derive(kFileBeingProcessed, FileName);
+ Canceler Invalidate = nullptr;
+ if (Invalidation) {
+ WithContext WC(std::move(Ctx));
+ std::tie(Ctx, Invalidate) = cancelableTask();
+ }
+ Requests.push_back({std::move(Task), std::string(Name), steady_clock::now(),
+ std::move(Ctx), UpdateType, Invalidation,
+ std::move(Invalidate)});
}
RequestsCV.notify_all();
}
@@ -942,7 +967,8 @@ void TUScheduler::run(llvm::StringRef Name,
void TUScheduler::runWithAST(
llvm::StringRef Name, PathRef File,
- llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action) {
+ llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
+ TUScheduler::ASTActionInvalidation Invalidation) {
auto It = Files.find(File);
if (It == Files.end()) {
Action(llvm::make_error<LSPError>(
@@ -950,7 +976,7 @@ void TUScheduler::runWithAST(
return;
}
- It->second->Worker->runWithAST(Name, std::move(Action));
+ It->second->Worker->runWithAST(Name, std::move(Action), Invalidation);
}
void TUScheduler::runWithPreamble(llvm::StringRef Name, PathRef File,
@@ -1067,5 +1093,15 @@ DebouncePolicy DebouncePolicy::fixed(clock::duration T) {
return P;
}
+void TUScheduler::InvalidatedError::log(llvm::raw_ostream &OS) const {
+ switch (Policy) {
+ case InvalidateOnUpdate:
+ OS << "Task was cancelled due to a subsequent change to the file.";
+ break;
+ case NoInvalidation:
+ llvm_unreachable("Invalidated for no reason?");
+ }
+}
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h
index 948fde7ed109..074404c23c11 100644
--- a/clang-tools-extra/clangd/TUScheduler.h
+++ b/clang-tools-extra/clangd/TUScheduler.h
@@ -219,6 +219,29 @@ class TUScheduler {
/// Schedule an async task with no dependencies.
void run(llvm::StringRef Name, llvm::unique_function<void()> Action);
+ /// Defines how a runWithAST action is implicitly cancelled by other actions.
+ enum ASTActionInvalidation {
+ /// The request will run unless explicitly cancelled.
+ NoInvalidation,
+ /// The request will be implicitly cancelled by a subsequent update().
+ /// (Only if the request was not yet cancelled).
+ /// Useful for requests that are generated by clients, without any explicit
+ /// user action. These can otherwise e.g. force every version to be built.
+ InvalidateOnUpdate,
+ };
+
+ /// Error to return when an ASTActionInvalidation policy fires.
+ class InvalidatedError : public llvm::ErrorInfo<InvalidatedError> {
+ public:
+ static char ID;
+ ASTActionInvalidation Policy;
+
+ void log(llvm::raw_ostream &OS) const override;
+ std::error_code convertToErrorCode() const override {
+ return std::make_error_code(std::errc::interrupted);
+ }
+ };
+
/// Schedule an async read of the AST. \p Action will be called when AST is
/// ready. The AST passed to \p Action refers to the version of \p File
/// tracked at the time of the call, even if new updates are received before
@@ -226,9 +249,11 @@ class TUScheduler {
/// 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.
+ /// receive a CancelledError. If the invalidation policy is triggered, the
+ /// callback will receive an InvalidatedError.
void runWithAST(llvm::StringRef Name, PathRef File,
- Callback<InputsAndAST> Action);
+ Callback<InputsAndAST> Action,
+ ASTActionInvalidation = NoInvalidation);
/// Controls whether preamble reads wait for the preamble to be up-to-date.
enum PreambleConsistency {
diff --git a/clang-tools-extra/clangd/unittests/CancellationTests.cpp b/clang-tools-extra/clangd/unittests/CancellationTests.cpp
index 611ce07dd8ee..3fe71901a206 100644
--- a/clang-tools-extra/clangd/unittests/CancellationTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CancellationTests.cpp
@@ -44,6 +44,30 @@ TEST(CancellationTest, TaskContextDiesHandleLives) {
Task.second();
}
+struct NestedTasks {
+ std::pair<Context, Canceler> Outer, Inner;
+ NestedTasks() {
+ Outer = cancelableTask();
+ {
+ WithContext WithOuter(Outer.first.clone());
+ Inner = cancelableTask();
+ }
+ }
+};
+
+TEST(CancellationTest, Nested) {
+ // Cancelling inner task works but leaves outer task unaffected.
+ NestedTasks CancelInner;
+ CancelInner.Inner.second();
+ EXPECT_TRUE(isCancelled(CancelInner.Inner.first));
+ EXPECT_FALSE(isCancelled(CancelInner.Outer.first));
+ // Cancellation of outer task is inherited by inner task.
+ NestedTasks CancelOuter;
+ CancelOuter.Outer.second();
+ EXPECT_TRUE(isCancelled(CancelOuter.Inner.first));
+ EXPECT_TRUE(isCancelled(CancelOuter.Outer.first));
+}
+
TEST(CancellationTest, AsynCancellationTest) {
std::atomic<bool> HasCancelled(false);
Notification Cancelled;
diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
index 9e5952fe2cb5..365e534fd66c 100644
--- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "Annotations.h"
+#include "Cancellation.h"
#include "Context.h"
#include "Diagnostics.h"
#include "Matchers.h"
@@ -346,6 +347,63 @@ TEST_F(TUSchedulerTests, Cancellation) {
<< "All reads other than R2B were cancelled";
}
+TEST_F(TUSchedulerTests, Invalidation) {
+ auto Path = testPath("foo.cpp");
+ TUScheduler S(CDB, optsForTest(), captureDiags());
+ std::atomic<int> Builds(0), Actions(0);
+
+ Notification Start;
+ updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector<Diag>) {
+ ++Builds;
+ Start.wait();
+ });
+ S.runWithAST(
+ "invalidatable", Path,
+ [&](llvm::Expected<InputsAndAST> AST) {
+ ++Actions;
+ EXPECT_FALSE(bool(AST));
+ llvm::Error E = AST.takeError();
+ EXPECT_TRUE(E.isA<CancelledError>());
+ consumeError(std::move(E));
+ },
+ TUScheduler::InvalidateOnUpdate);
+ S.runWithAST(
+ "not-invalidatable", Path,
+ [&](llvm::Expected<InputsAndAST> AST) {
+ ++Actions;
+ EXPECT_TRUE(bool(AST));
+ },
+ TUScheduler::NoInvalidation);
+ updateWithDiags(S, Path, "b", WantDiagnostics::Auto, [&](std::vector<Diag>) {
+ ++Builds;
+ ADD_FAILURE() << "Shouldn't build, all dependents invalidated";
+ });
+ S.runWithAST(
+ "invalidatable", Path,
+ [&](llvm::Expected<InputsAndAST> AST) {
+ ++Actions;
+ EXPECT_FALSE(bool(AST));
+ llvm::Error E = AST.takeError();
+ EXPECT_TRUE(E.isA<CancelledError>());
+ consumeError(std::move(E));
+ },
+ TUScheduler::InvalidateOnUpdate);
+ updateWithDiags(S, Path, "c", WantDiagnostics::Auto,
+ [&](std::vector<Diag>) { ++Builds; });
+ S.runWithAST(
+ "invalidatable", Path,
+ [&](llvm::Expected<InputsAndAST> AST) {
+ ++Actions;
+ EXPECT_TRUE(bool(AST)) << "Shouldn't be invalidated, no update follows";
+ },
+ TUScheduler::InvalidateOnUpdate);
+ Start.notify();
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+
+ EXPECT_EQ(2, Builds.load()) << "Middle build should be skipped";
+ EXPECT_EQ(4, Actions.load()) << "All actions should run (some with error)";
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
More information about the cfe-commits
mailing list