Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp =================================================================== --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp @@ -141,7 +141,8 @@ if (Params.metadata && !Params.metadata->extraFlags.empty()) CDB.setExtraFlagsForFile(Params.textDocument.uri.file(), std::move(Params.metadata->extraFlags)); - Server.addDocument(Params.textDocument.uri.file(), Params.textDocument.text); + Server.addDocument(Params.textDocument.uri.file(), Params.textDocument.text, + WantDiagnostics::Yes); } void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) { @@ -150,7 +151,7 @@ "can only apply one change at a time"); // We only support full syncing right now. Server.addDocument(Params.textDocument.uri.file(), - Params.contentChanges[0].text); + Params.contentChanges[0].text, WantDiagnostics::Auto); } void ClangdLSPServer::onFileEvent(DidChangeWatchedFilesParams &Params) { Index: clang-tools-extra/trunk/clangd/ClangdServer.h =================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.h +++ clang-tools-extra/trunk/clangd/ClangdServer.h @@ -150,7 +150,8 @@ /// \p File is already tracked. Also schedules parsing of the AST for it on a /// separate thread. When the parsing is complete, DiagConsumer passed in /// constructor will receive onDiagnosticsReady callback. - void addDocument(PathRef File, StringRef Contents); + void addDocument(PathRef File, StringRef Contents, + WantDiagnostics WD = WantDiagnostics::Auto); /// Remove \p File from list of tracked files, schedule a request to free /// resources associated with it. @@ -278,6 +279,7 @@ void scheduleReparseAndDiags(PathRef File, VersionedDraft Contents, + WantDiagnostics WD, Tagged> TaggedFS); CompileArgsCache CompileArgs; Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp =================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.cpp +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp @@ -110,11 +110,12 @@ this->RootPath = NewRootPath; } -void ClangdServer::addDocument(PathRef File, StringRef Contents) { +void ClangdServer::addDocument(PathRef File, StringRef Contents, + WantDiagnostics WantDiags) { DocVersion Version = DraftMgr.updateDraft(File, Contents); auto TaggedFS = FSProvider.getTaggedFileSystem(File); scheduleReparseAndDiags(File, VersionedDraft{Version, Contents.str()}, - std::move(TaggedFS)); + WantDiags, std::move(TaggedFS)); } void ClangdServer::removeDocument(PathRef File) { @@ -133,7 +134,8 @@ CompileArgs.invalidate(File); auto TaggedFS = FSProvider.getTaggedFileSystem(File); - scheduleReparseAndDiags(File, std::move(FileContents), std::move(TaggedFS)); + scheduleReparseAndDiags(File, std::move(FileContents), WantDiagnostics::Yes, + std::move(TaggedFS)); } void ClangdServer::codeComplete( @@ -519,20 +521,16 @@ } void ClangdServer::scheduleReparseAndDiags( - PathRef File, VersionedDraft Contents, + PathRef File, VersionedDraft Contents, WantDiagnostics WantDiags, Tagged> TaggedFS) { tooling::CompileCommand Command = CompileArgs.getCompileCommand(File); - using OptDiags = llvm::Optional>; - DocVersion Version = Contents.Version; Path FileStr = File.str(); VFSTag Tag = std::move(TaggedFS.Tag); - auto Callback = [this, Version, FileStr, Tag](OptDiags Diags) { - if (!Diags) - return; // A new reparse was requested before this one completed. - + auto Callback = [this, Version, FileStr, + Tag](std::vector Diags) { // We need to serialize access to resulting diagnostics to avoid calling // `onDiagnosticsReady` in the wrong order. std::lock_guard DiagsLock(DiagnosticsMutex); @@ -546,14 +544,14 @@ LastReportedDiagsVersion = Version; DiagConsumer.onDiagnosticsReady( - FileStr, make_tagged(std::move(*Diags), std::move(Tag))); + FileStr, make_tagged(std::move(Diags), std::move(Tag))); }; WorkScheduler.update(File, ParseInputs{std::move(Command), std::move(TaggedFS.Value), std::move(*Contents.Draft)}, - std::move(Callback)); + WantDiags, std::move(Callback)); } void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) { Index: clang-tools-extra/trunk/clangd/TUScheduler.h =================================================================== --- clang-tools-extra/trunk/clangd/TUScheduler.h +++ clang-tools-extra/trunk/clangd/TUScheduler.h @@ -32,6 +32,14 @@ const PreambleData *Preamble; }; +/// Determines whether diagnostics should be generated for a file snapshot. +enum class WantDiagnostics { + Yes, /// Diagnostics must be generated for this snapshot. + No, /// Diagnostics must not be generated for this snapshot. + Auto, /// Diagnostics must be generated for this snapshot or a subsequent one, + /// within a bounded amount of time. +}; + /// Handles running tasks for ClangdServer and managing the resources (e.g., /// preambles and ASTs) for opened files. /// TUScheduler is not thread-safe, only one thread should be providing updates @@ -51,9 +59,8 @@ /// Schedule an update for \p File. Adds \p File to a list of tracked files if /// \p File was not part of it before. /// FIXME(ibiryukov): remove the callback from this function. - void update(PathRef File, ParseInputs Inputs, - UniqueFunction>)> - OnUpdated); + void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD, + UniqueFunction)> OnUpdated); /// Remove \p File from the list of tracked files and schedule removal of its /// resources. Index: clang-tools-extra/trunk/clangd/TUScheduler.cpp =================================================================== --- clang-tools-extra/trunk/clangd/TUScheduler.cpp +++ clang-tools-extra/trunk/clangd/TUScheduler.cpp @@ -82,9 +82,8 @@ Semaphore &Barrier, CppFile AST); ~ASTWorker(); - void update(ParseInputs Inputs, - UniqueFunction>)> - OnUpdated); + void update(ParseInputs Inputs, WantDiagnostics, + UniqueFunction)> OnUpdated); void runWithAST(llvm::StringRef Name, UniqueFunction)> Action); bool blockUntilIdle(Deadline Timeout) const; @@ -101,12 +100,15 @@ void stop(); /// Adds a new task to the end of the request queue. void startTask(llvm::StringRef Name, UniqueFunction Task, - bool isUpdate, llvm::Optional CF); + llvm::Optional UpdateType); + /// Should the first task in the queue be skipped instead of run? + bool shouldSkipHeadLocked() const; struct Request { UniqueFunction Action; std::string Name; Context Ctx; + llvm::Optional UpdateType; }; std::string File; @@ -123,10 +125,7 @@ std::size_t LastASTSize; /* GUARDED_BY(Mutex) */ // Set to true to signal run() to finish processing. bool Done; /* GUARDED_BY(Mutex) */ - std::queue Requests; /* GUARDED_BY(Mutex) */ - // Only set when last request is an update. This allows us to cancel an update - // that was never read, if a subsequent update comes in. - llvm::Optional LastUpdateCF; /* GUARDED_BY(Mutex) */ + std::deque Requests; /* GUARDED_BY(Mutex) */ mutable std::condition_variable RequestsCV; }; @@ -200,14 +199,9 @@ } void ASTWorker::update( - ParseInputs Inputs, - UniqueFunction>)> - OnUpdated) { - auto Task = [=](CancellationFlag CF, decltype(OnUpdated) OnUpdated) mutable { - if (CF.isCancelled()) { - OnUpdated(llvm::None); - return; - } + ParseInputs Inputs, WantDiagnostics WantDiags, + UniqueFunction)> OnUpdated) { + auto Task = [=](decltype(OnUpdated) OnUpdated) mutable { FileInputs = Inputs; auto Diags = AST.rebuild(std::move(Inputs)); @@ -220,12 +214,11 @@ // We want to report the diagnostics even if this update was cancelled. // It seems more useful than making the clients wait indefinitely if they // spam us with updates. - OnUpdated(std::move(Diags)); + if (Diags && WantDiags != WantDiagnostics::No) + OnUpdated(std::move(*Diags)); }; - CancellationFlag UpdateCF; - startTask("Update", BindWithForward(Task, UpdateCF, std::move(OnUpdated)), - /*isUpdate=*/true, UpdateCF); + startTask("Update", BindWithForward(Task, std::move(OnUpdated)), WantDiags); } void ASTWorker::runWithAST( @@ -247,7 +240,7 @@ }; startTask(Name, BindWithForward(Task, std::move(Action)), - /*isUpdate=*/false, llvm::None); + /*UpdateType=*/llvm::None); } std::shared_ptr @@ -271,10 +264,7 @@ } void ASTWorker::startTask(llvm::StringRef Name, UniqueFunction Task, - bool isUpdate, llvm::Optional CF) { - assert(isUpdate == CF.hasValue() && - "Only updates are expected to pass CancellationFlag"); - + llvm::Optional UpdateType) { if (RunSync) { assert(!Done && "running a task after stop()"); trace::Span Tracer(Name + ":" + llvm::sys::path::filename(File)); @@ -285,18 +275,9 @@ { std::lock_guard Lock(Mutex); assert(!Done && "running a task after stop()"); - if (isUpdate) { - if (!Requests.empty() && LastUpdateCF) { - // There were no reads for the last unprocessed update, let's cancel it - // to not waste time on it. - LastUpdateCF->cancel(); - } - LastUpdateCF = std::move(*CF); - } else { - LastUpdateCF = llvm::None; - } - Requests.push({std::move(Task), Name, Context::current().clone()}); - } // unlock Mutex. + Requests.push_back( + {std::move(Task), Name, Context::current().clone(), UpdateType}); + } RequestsCV.notify_all(); } @@ -313,6 +294,9 @@ // Even when Done is true, we finish processing all pending requests // before exiting the processing loop. + while (shouldSkipHeadLocked()) + Requests.pop_front(); + assert(!Requests.empty() && "skipped the whole queue"); Req = std::move(Requests.front()); // Leave it on the queue for now, so waiters don't see an empty queue. } // unlock Mutex @@ -326,12 +310,40 @@ { std::lock_guard Lock(Mutex); - Requests.pop(); + Requests.pop_front(); } RequestsCV.notify_all(); } } +// Returns true if Requests.front() is a dead update that can be skipped. +bool ASTWorker::shouldSkipHeadLocked() const { + assert(!Requests.empty()); + auto Next = Requests.begin(); + auto UpdateType = Next->UpdateType; + if (!UpdateType) // 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) + return false; + // The other way an update can be live is if its diagnostics might be used. + switch (*UpdateType) { + case WantDiagnostics::Yes: + return false; // Always used. + case WantDiagnostics::No: + return true; // Always dead. + 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) + return true; // Prefer later diagnostics. + return false; + } +} + bool ASTWorker::blockUntilIdle(Deadline Timeout) const { std::unique_lock Lock(Mutex); return wait(Lock, RequestsCV, Timeout, [&] { return Requests.empty(); }); @@ -389,9 +401,8 @@ } void TUScheduler::update( - PathRef File, ParseInputs Inputs, - UniqueFunction>)> - OnUpdated) { + PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags, + UniqueFunction)> OnUpdated) { std::unique_ptr &FD = Files[File]; if (!FD) { // Create a new worker to process the AST-related tasks. @@ -402,7 +413,7 @@ } else { FD->Inputs = Inputs; } - FD->Worker->update(std::move(Inputs), std::move(OnUpdated)); + FD->Worker->update(std::move(Inputs), WantDiags, std::move(OnUpdated)); } void TUScheduler::remove(PathRef File) { Index: clang-tools-extra/trunk/clangd/Threading.h =================================================================== --- clang-tools-extra/trunk/clangd/Threading.h +++ clang-tools-extra/trunk/clangd/Threading.h @@ -13,7 +13,6 @@ #include "Context.h" #include "Function.h" #include "llvm/ADT/Twine.h" -#include #include #include #include @@ -23,24 +22,18 @@ namespace clang { namespace clangd { -/// A shared boolean flag indicating if the computation was cancelled. -/// Once cancelled, cannot be returned to the previous state. -class CancellationFlag { +/// A threadsafe flag that is initially clear. +class Notification { public: - CancellationFlag(); - - void cancel() { - assert(WasCancelled && "the object was moved"); - WasCancelled->store(true); - } - - bool isCancelled() const { - assert(WasCancelled && "the object was moved"); - return WasCancelled->load(); - } + // Sets the flag. No-op if already set. + void notify(); + // Blocks until flag is set. + void wait() const; private: - std::shared_ptr> WasCancelled; + bool Notified = false; + mutable std::condition_variable CV; + mutable std::mutex Mu; }; /// Limits the number of threads that can acquire the lock at the same time. Index: clang-tools-extra/trunk/clangd/Threading.cpp =================================================================== --- clang-tools-extra/trunk/clangd/Threading.cpp +++ clang-tools-extra/trunk/clangd/Threading.cpp @@ -7,8 +7,18 @@ namespace clang { namespace clangd { -CancellationFlag::CancellationFlag() - : WasCancelled(std::make_shared>(false)) {} +void Notification::notify() { + { + std::lock_guard Lock(Mu); + Notified = true; + } + CV.notify_all(); +} + +void Notification::wait() const { + std::unique_lock Lock(Mu); + CV.wait(Lock, [this] { return Notified; }); +} Semaphore::Semaphore(std::size_t MaxLocks) : FreeSlots(MaxLocks) {} Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp =================================================================== --- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp @@ -50,7 +50,7 @@ auto Missing = testPath("missing.cpp"); Files[Missing] = ""; - S.update(Added, getInputs(Added, ""), ignoreUpdate); + S.update(Added, getInputs(Added, ""), WantDiagnostics::No, ignoreUpdate); // Assert each operation for missing file is an error (even if it's available // in VFS). @@ -88,6 +88,37 @@ S.remove(Added); } +TEST_F(TUSchedulerTests, WantDiagnostics) { + std::atomic CallbackCount(0); + { + TUScheduler S(getDefaultAsyncThreadsCount(), + /*StorePreamblesInMemory=*/true, + /*ASTParsedCallback=*/nullptr); + auto Path = testPath("foo.cpp"); + + // To avoid a racy test, don't allow tasks to actualy run on the worker + // thread until we've scheduled them all. + Notification Ready; + S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes, + [&](std::vector) { Ready.wait(); }); + + S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes, + [&](std::vector Diags) { ++CallbackCount; }); + S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto, + [&](std::vector Diags) { + ADD_FAILURE() << "auto should have been cancelled by auto"; + }); + S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No, + [&](std::vector Diags) { + ADD_FAILURE() << "no diags should not be called back"; + }); + S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto, + [&](std::vector Diags) { ++CallbackCount; }); + Ready.notify(); + } + EXPECT_EQ(2, CallbackCount); +} + TEST_F(TUSchedulerTests, ManyUpdates) { const int FilesCount = 3; const int UpdatesPerFile = 10; @@ -132,7 +163,7 @@ { WithContextValue WithNonce(NonceKey, ++Nonce); - S.update(File, Inputs, + S.update(File, Inputs, WantDiagnostics::Auto, [Nonce, &Mut, &TotalUpdates]( llvm::Optional> Diags) { EXPECT_THAT(Context::current().get(NonceKey),