[clang-tools-extra] 2fced5a - [clangd] Don't cancel requests based on "updates" with same content
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 18 17:03:48 PST 2020
Author: Sam McCall
Date: 2020-12-19T02:03:40+01:00
New Revision: 2fced5a07b45ef527ac00a13e63bfca61e407ee3
URL: https://github.com/llvm/llvm-project/commit/2fced5a07b45ef527ac00a13e63bfca61e407ee3
DIFF: https://github.com/llvm/llvm-project/commit/2fced5a07b45ef527ac00a13e63bfca61e407ee3.diff
LOG: [clangd] Don't cancel requests based on "updates" with same content
There's an unfortunate collision between two features:
- we implicitly cancel certain requests when the file changes, to avoid
the queue getting clogged building old revisions to service stale requests
- we "reparse-if-needed" by synthesizing a file change, e.g. on didSave
We could explicitly mark these synthetic requests to avoid this, but
looking for changes in file content clutters our APIs less and is
arguably the correct thing to do in any case.
Fixes https://github.com/clangd/clangd/issues/620
Added:
Modified:
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 443abcfe847a..813a000b41a5 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -385,7 +385,7 @@ class ASTWorker {
ParsingCallbacks &Callbacks);
~ASTWorker();
- void update(ParseInputs Inputs, WantDiagnostics);
+ void update(ParseInputs Inputs, WantDiagnostics, bool ContentChanged);
void
runWithAST(llvm::StringRef Name,
llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
@@ -419,6 +419,18 @@ class ASTWorker {
bool isASTCached() const;
private:
+ // Details of an update request that are relevant to scheduling.
+ struct UpdateType {
+ // Do we want diagnostics from this version?
+ // If Yes, we must always build this version.
+ // If No, we only need to build this version if it's read.
+ // If Auto, we build if it's read or if the debounce expires.
+ WantDiagnostics Diagnostics;
+ // Did the main-file content of the document change?
+ // If so, we're allowed to cancel certain invalidated preceding reads.
+ bool ContentChanged;
+ };
+
/// Publishes diagnostics for \p Inputs. It will build an AST or reuse the
/// cached one if applicable. Assumes LatestPreamble is compatible for \p
/// Inputs.
@@ -431,9 +443,10 @@ class ASTWorker {
void run();
/// Signal that run() should finish processing pending requests and exit.
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<UpdateType> Update,
TUScheduler::ASTActionInvalidation);
/// Determines the next action to perform.
@@ -449,7 +462,7 @@ class ASTWorker {
std::string Name;
steady_clock::time_point AddTime;
Context Ctx;
- llvm::Optional<WantDiagnostics> UpdateType;
+ llvm::Optional<UpdateType> Update;
TUScheduler::ASTActionInvalidation InvalidationPolicy;
Canceler Invalidate;
};
@@ -598,7 +611,8 @@ ASTWorker::~ASTWorker() {
#endif
}
-void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
+void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags,
+ bool ContentChanged) {
std::string TaskName = llvm::formatv("Update ({0})", Inputs.Version);
auto Task = [=]() mutable {
// Get the actual command as `Inputs` does not have a command.
@@ -679,7 +693,8 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
});
return;
};
- startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation);
+ startTask(TaskName, std::move(Task), UpdateType{WantDiags, ContentChanged},
+ TUScheduler::NoInvalidation);
}
void ASTWorker::runWithAST(
@@ -723,7 +738,7 @@ void ASTWorker::runWithAST(
FileInputs.Version);
Action(InputsAndAST{FileInputs, **AST});
};
- startTask(Name, std::move(Task), /*UpdateType=*/None, Invalidation);
+ startTask(Name, std::move(Task), /*Update=*/None, Invalidation);
}
void PreambleThread::build(Request Req) {
@@ -960,7 +975,7 @@ void ASTWorker::stop() {
void ASTWorker::startTask(llvm::StringRef Name,
llvm::unique_function<void()> Task,
- llvm::Optional<WantDiagnostics> UpdateType,
+ llvm::Optional<UpdateType> Update,
TUScheduler::ASTActionInvalidation Invalidation) {
if (RunSync) {
assert(!Done && "running a task after stop()");
@@ -974,11 +989,11 @@ void ASTWorker::startTask(llvm::StringRef Name,
std::lock_guard<std::mutex> Lock(Mutex);
assert(!Done && "running a task after stop()");
// Cancel any requests invalidated by this request.
- if (UpdateType) {
+ if (Update && Update->ContentChanged) {
for (auto &R : llvm::reverse(Requests)) {
if (R.InvalidationPolicy == TUScheduler::InvalidateOnUpdate)
R.Invalidate();
- if (R.UpdateType)
+ if (R.Update && R.Update->ContentChanged)
break; // Older requests were already invalidated by the older update.
}
}
@@ -992,7 +1007,7 @@ void ASTWorker::startTask(llvm::StringRef Name,
/*Reason=*/static_cast<int>(ErrorCode::ContentModified));
}
Requests.push_back({std::move(Task), std::string(Name), steady_clock::now(),
- std::move(Ctx), UpdateType, Invalidation,
+ std::move(Ctx), Update, Invalidation,
std::move(Invalidate)});
}
RequestsCV.notify_all();
@@ -1093,20 +1108,20 @@ Deadline ASTWorker::scheduleLocked() {
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)
+ if (I->Update == None)
break;
continue;
}
// Cancelled reads are moved to the front of the queue and run immediately.
- if (I->UpdateType == None) {
+ if (I->Update == 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;
+ if (I->Update->Diagnostics == WantDiagnostics::Yes)
+ I->Update->Diagnostics = WantDiagnostics::Auto;
}
while (shouldSkipHeadLocked()) {
@@ -1119,7 +1134,7 @@ Deadline ASTWorker::scheduleLocked() {
// We debounce "maybe-unused" writes, sleeping in case they become dead.
// But don't delay reads (including updates where diagnostics are needed).
for (const auto &R : Requests)
- if (R.UpdateType == None || R.UpdateType == WantDiagnostics::Yes)
+ if (R.Update == None || R.Update->Diagnostics == WantDiagnostics::Yes)
return Deadline::zero();
// Front request needs to be debounced, so determine when we're ready.
Deadline D(Requests.front().AddTime + UpdateDebounce.compute(RebuildTimes));
@@ -1130,16 +1145,16 @@ Deadline ASTWorker::scheduleLocked() {
bool ASTWorker::shouldSkipHeadLocked() const {
assert(!Requests.empty());
auto Next = Requests.begin();
- auto UpdateType = Next->UpdateType;
- if (!UpdateType) // Only skip updates.
+ auto Update = Next->Update;
+ if (!Update) // Only skip updates.
return false;
++Next;
// An update is live if its AST might still be read.
// That is, if it's not immediately followed by another update.
- if (Next == Requests.end() || !Next->UpdateType)
+ if (Next == Requests.end() || !Next->Update)
return false;
// The other way an update can be live is if its diagnostics might be used.
- switch (*UpdateType) {
+ switch (Update->Diagnostics) {
case WantDiagnostics::Yes:
return false; // Always used.
case WantDiagnostics::No:
@@ -1147,8 +1162,7 @@ bool ASTWorker::shouldSkipHeadLocked() const {
case WantDiagnostics::Auto:
// Used unless followed by an update that generates diagnostics.
for (; Next != Requests.end(); ++Next)
- if (Next->UpdateType == WantDiagnostics::Yes ||
- Next->UpdateType == WantDiagnostics::Auto)
+ if (Next->Update && Next->Update->Diagnostics != WantDiagnostics::No)
return true; // Prefer later diagnostics.
return false;
}
@@ -1274,6 +1288,7 @@ bool TUScheduler::update(PathRef File, ParseInputs Inputs,
WantDiagnostics WantDiags) {
std::unique_ptr<FileData> &FD = Files[File];
bool NewFile = FD == nullptr;
+ bool ContentChanged = false;
if (!FD) {
// Create a new worker to process the AST-related tasks.
ASTWorkerHandle Worker =
@@ -1282,10 +1297,12 @@ bool TUScheduler::update(PathRef File, ParseInputs Inputs,
Barrier, Opts, *Callbacks);
FD = std::unique_ptr<FileData>(
new FileData{Inputs.Contents, std::move(Worker)});
- } else {
+ ContentChanged = true;
+ } else if (FD->Contents != Inputs.Contents) {
+ ContentChanged = true;
FD->Contents = Inputs.Contents;
}
- FD->Worker->update(std::move(Inputs), WantDiags);
+ FD->Worker->update(std::move(Inputs), WantDiags, ContentChanged);
return NewFile;
}
diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
index 61ac4f7a27a4..a51067896432 100644
--- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -417,6 +417,38 @@ TEST_F(TUSchedulerTests, Invalidation) {
EXPECT_EQ(4, Actions.load()) << "All actions should run (some with error)";
}
+// We don't invalidate requests for updates that don't change the file content.
+// These are mostly "refresh this file" events synthesized inside clangd itself.
+// (Usually the AST rebuild is elided after verifying that all inputs are
+// unchanged, but invalidation decisions happen earlier and so independently).
+// See https://github.com/clangd/clangd/issues/620
+TEST_F(TUSchedulerTests, InvalidationUnchanged) {
+ auto Path = testPath("foo.cpp");
+ TUScheduler S(CDB, optsForTest(), captureDiags());
+ std::atomic<int> Actions(0);
+
+ Notification Start;
+ updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector<Diag>) {
+ Start.wait();
+ });
+ S.runWithAST(
+ "invalidatable", Path,
+ [&](llvm::Expected<InputsAndAST> AST) {
+ ++Actions;
+ EXPECT_TRUE(bool(AST))
+ << "Should not invalidate based on an update with same content: "
+ << llvm::toString(AST.takeError());
+ },
+ TUScheduler::InvalidateOnUpdate);
+ updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector<Diag>) {
+ ADD_FAILURE() << "Shouldn't build, identical to previous";
+ });
+ Start.notify();
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+
+ EXPECT_EQ(1, Actions.load()) << "All actions should run";
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
More information about the cfe-commits
mailing list