From 2fced5a07b45ef527ac00a13e63bfca61e407ee3 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Sat, 19 Dec 2020 01:55:26 +0100 Subject: [PATCH] [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 --- clang-tools-extra/clangd/TUScheduler.cpp | 63 ++++++++++++++-------- .../clangd/unittests/TUSchedulerTests.cpp | 32 +++++++++++ 2 files changed, 72 insertions(+), 23 deletions(-) diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp index 443abcf..813a000 100644 --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -385,7 +385,7 @@ public: ParsingCallbacks &Callbacks); ~ASTWorker(); - void update(ParseInputs Inputs, WantDiagnostics); + void update(ParseInputs Inputs, WantDiagnostics, bool ContentChanged); void runWithAST(llvm::StringRef Name, llvm::unique_function)> Action, @@ -419,6 +419,18 @@ public: 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 @@ private: 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 Task, - llvm::Optional UpdateType, + llvm::Optional Update, TUScheduler::ASTActionInvalidation); /// Determines the next action to perform. @@ -449,7 +462,7 @@ private: std::string Name; steady_clock::time_point AddTime; Context Ctx; - llvm::Optional UpdateType; + llvm::Optional 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 Task, - llvm::Optional UpdateType, + llvm::Optional 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 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(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 &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( 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 61ac4f7..a510678 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 Actions(0); + + Notification Start; + updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector) { + Start.wait(); + }); + S.runWithAST( + "invalidatable", Path, + [&](llvm::Expected 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) { + 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; -- 2.7.4