Index: clangd/ClangdLSPServer.h =================================================================== --- clangd/ClangdLSPServer.h +++ clangd/ClangdLSPServer.h @@ -76,6 +76,7 @@ void onCommand(ExecuteCommandParams &Params) override; void onRename(RenameParams &Parames) override; void onHover(TextDocumentPositionParams &Params) override; + void onChangeConfiguration(DidChangeConfigurationParams &Params) override; std::vector getFixIts(StringRef File, const clangd::Diagnostic &D); Index: clangd/ClangdLSPServer.cpp =================================================================== --- clangd/ClangdLSPServer.cpp +++ 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) { @@ -369,6 +370,18 @@ }); } +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( + DidChangeConfigurationParams &Params) { + ClangdConfigurationParamsChange &Settings = Params.settings; + + // Compilation database change. + if (Settings.compilationDatabasePath.hasValue()) { + CDB.setCompileCommandsDir(Settings.compilationDatabasePath.getValue()); + Server.reparseOpenedFiles(); + } +} + ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount, bool StorePreamblesInMemory, const clangd::CodeCompleteOptions &CCOpts, Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -125,6 +125,8 @@ /// \p DiagConsumer. Note that a callback to \p DiagConsumer happens on a /// worker thread. Therefore, instances of \p DiagConsumer must properly /// synchronize access to shared state. + /// UpdateDebounce determines how long to wait after a new version of the file + /// before starting to compute diagnostics. /// /// \p StorePreamblesInMemory defines whether the Preambles generated by /// clangd are stored in-memory or on disk. @@ -135,13 +137,16 @@ /// /// If \p StaticIdx is set, ClangdServer uses the index for global code /// completion. + /// FIXME(sammccall): pull out an options struct. ClangdServer(GlobalCompilationDatabase &CDB, DiagnosticsConsumer &DiagConsumer, FileSystemProvider &FSProvider, unsigned AsyncThreadsCount, bool StorePreamblesInMemory, bool BuildDynamicSymbolIndex = false, SymbolIndex *StaticIdx = nullptr, - llvm::Optional ResourceDir = llvm::None); + llvm::Optional ResourceDir = llvm::None, + std::chrono::steady_clock::duration UpdateDebounce = + std::chrono::milliseconds(500)); /// Set the root path of the workspace. void setRootPath(PathRef RootPath); @@ -150,7 +155,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. @@ -162,6 +168,11 @@ /// and AST and rebuild them from scratch. void forceReparse(PathRef File); + /// Calls forceReparse() on all currently opened files. + /// As a result, this method may be very expensive. + /// This method is normally called when the compilation database is changed. + void reparseOpenedFiles(); + /// Run code completion for \p File at \p Pos. /// Request is processed asynchronously. /// @@ -278,6 +289,7 @@ void scheduleReparseAndDiags(PathRef File, VersionedDraft Contents, + WantDiagnostics WD, Tagged> TaggedFS); CompileArgsCache CompileArgs; Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -76,7 +76,8 @@ unsigned AsyncThreadsCount, bool StorePreamblesInMemory, bool BuildDynamicSymbolIndex, SymbolIndex *StaticIdx, - llvm::Optional ResourceDir) + llvm::Optional ResourceDir, + std::chrono::steady_clock::duration UpdateDebounce) : CompileArgs(CDB, ResourceDir ? ResourceDir->str() : getStandardResourceDir()), DiagConsumer(DiagConsumer), FSProvider(FSProvider), @@ -91,7 +92,8 @@ FileIdx ? [this](PathRef Path, ParsedAST *AST) { FileIdx->update(Path, AST); } - : ASTParsedCallback()) { + : ASTParsedCallback(), + UpdateDebounce) { if (FileIdx && StaticIdx) { MergedIndex = mergeIndex(FileIdx.get(), StaticIdx); Index = MergedIndex.get(); @@ -110,11 +112,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 +136,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 +523,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 +546,19 @@ 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::reparseOpenedFiles() { + for (const Path &FilePath : DraftMgr.getActiveFiles()) + forceReparse(FilePath); } void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) { Index: clangd/CodeComplete.cpp =================================================================== --- clangd/CodeComplete.cpp +++ clangd/CodeComplete.cpp @@ -696,11 +696,11 @@ Input.Preamble->CanReuse(*CI, ContentsBuffer.get(), Bounds, Input.VFS.get()); } + // The diagnostic options must be set before creating a CompilerInstance. + CI->getDiagnosticOpts().IgnoreWarnings = true; auto Clang = prepareCompilerInstance( std::move(CI), Input.Preamble, std::move(ContentsBuffer), std::move(Input.PCHs), std::move(Input.VFS), DummyDiagsConsumer); - auto &DiagOpts = Clang->getDiagnosticOpts(); - DiagOpts.IgnoreWarnings = true; // Disable typo correction in Sema. Clang->getLangOpts().SpellChecking = false; Index: clangd/DraftStore.h =================================================================== --- clangd/DraftStore.h +++ clangd/DraftStore.h @@ -40,6 +40,12 @@ /// \return version and contents of the stored document. /// For untracked files, a (0, None) pair is returned. VersionedDraft getDraft(PathRef File) const; + + /// \return List of names of active drafts in this store. Drafts that were + /// removed (which still have an entry in the Drafts map) are not returned by + /// this function. + std::vector getActiveFiles() const; + /// \return version of the tracked document. /// For untracked files, 0 is returned. DocVersion getVersion(PathRef File) const; Index: clangd/DraftStore.cpp =================================================================== --- clangd/DraftStore.cpp +++ clangd/DraftStore.cpp @@ -21,6 +21,17 @@ return It->second; } +std::vector DraftStore::getActiveFiles() const { + std::lock_guard Lock(Mutex); + std::vector ResultVector; + + for (auto DraftIt = Drafts.begin(); DraftIt != Drafts.end(); DraftIt++) + if (DraftIt->second.Draft) + ResultVector.push_back(DraftIt->getKey()); + + return ResultVector; +} + DocVersion DraftStore::getVersion(PathRef File) const { std::lock_guard Lock(Mutex); Index: clangd/GlobalCompilationDatabase.h =================================================================== --- clangd/GlobalCompilationDatabase.h +++ clangd/GlobalCompilationDatabase.h @@ -61,6 +61,9 @@ /// Uses the default fallback command, adding any extra flags. tooling::CompileCommand getFallbackCommand(PathRef File) const override; + /// Set the compile commands directory to \p P. + void setCompileCommandsDir(Path P); + /// Sets the extra flags that should be added to a file. void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags); Index: clangd/GlobalCompilationDatabase.cpp =================================================================== --- clangd/GlobalCompilationDatabase.cpp +++ clangd/GlobalCompilationDatabase.cpp @@ -51,6 +51,12 @@ return C; } +void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) { + std::lock_guard Lock(Mutex); + CompileCommandsDir = P; + CompilationDatabases.clear(); +} + void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile( PathRef File, std::vector ExtraFlags) { std::lock_guard Lock(Mutex); Index: clangd/Headers.cpp =================================================================== --- clangd/Headers.cpp +++ clangd/Headers.cpp @@ -79,12 +79,12 @@ // added more than once. CI->getPreprocessorOpts().SingleFileParseMode = true; + // The diagnostic options must be set before creating a CompilerInstance. + CI->getDiagnosticOpts().IgnoreWarnings = true; auto Clang = prepareCompilerInstance( std::move(CI), /*Preamble=*/nullptr, llvm::MemoryBuffer::getMemBuffer(Code, File), std::make_shared(), FS, IgnoreDiags); - auto &DiagOpts = Clang->getDiagnosticOpts(); - DiagOpts.IgnoreWarnings = true; if (Clang->getFrontendOpts().Inputs.empty()) return llvm::make_error( Index: clangd/Protocol.h =================================================================== --- clangd/Protocol.h +++ clangd/Protocol.h @@ -324,6 +324,20 @@ }; bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &); +/// Clangd extension to manage a workspace/didChangeConfiguration notification +/// since the data received is described as 'any' type in LSP. +struct ClangdConfigurationParamsChange { + llvm::Optional compilationDatabasePath; +}; +bool fromJSON(const json::Expr &, ClangdConfigurationParamsChange &); + +struct DidChangeConfigurationParams { + // We use this predefined struct because it is easier to use + // than the protocol specified type of 'any'. + ClangdConfigurationParamsChange settings; +}; +bool fromJSON(const json::Expr &, DidChangeConfigurationParams &); + struct FormattingOptions { /// Size of a tab in spaces. int tabSize = 0; Index: clangd/Protocol.cpp =================================================================== --- clangd/Protocol.cpp +++ clangd/Protocol.cpp @@ -497,5 +497,15 @@ }; } +bool fromJSON(const json::Expr &Params, DidChangeConfigurationParams &CCP) { + json::ObjectMapper O(Params); + return O && O.map("settings", CCP.settings); +} + +bool fromJSON(const json::Expr &Params, ClangdConfigurationParamsChange &CCPC) { + json::ObjectMapper O(Params); + return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath); +} + } // namespace clangd } // namespace clang Index: clangd/ProtocolHandlers.h =================================================================== --- clangd/ProtocolHandlers.h +++ clangd/ProtocolHandlers.h @@ -52,6 +52,7 @@ virtual void onRename(RenameParams &Parames) = 0; virtual void onDocumentHighlight(TextDocumentPositionParams &Params) = 0; virtual void onHover(TextDocumentPositionParams &Params) = 0; + virtual void onChangeConfiguration(DidChangeConfigurationParams &Params) = 0; }; void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out, Index: clangd/ProtocolHandlers.cpp =================================================================== --- clangd/ProtocolHandlers.cpp +++ clangd/ProtocolHandlers.cpp @@ -72,4 +72,6 @@ Register("workspace/executeCommand", &ProtocolCallbacks::onCommand); Register("textDocument/documentHighlight", &ProtocolCallbacks::onDocumentHighlight); + Register("workspace/didChangeConfiguration", + &ProtocolCallbacks::onChangeConfiguration); } Index: clangd/TUScheduler.h =================================================================== --- clangd/TUScheduler.h +++ clangd/TUScheduler.h @@ -17,6 +17,7 @@ namespace clang { namespace clangd { + /// Returns a number of a default async threads to use for TUScheduler. /// Returned value is always >= 1 (i.e. will not cause requests to be processed /// synchronously). @@ -32,16 +33,27 @@ 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 /// and scheduling tasks. /// Callbacks are run on a threadpool and it's appropriate to do slow work in /// them. Each task has a name, used for tracing (should be UpperCamelCase). +/// FIXME(sammccall): pull out a scheduler options struct. class TUScheduler { public: TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory, - ASTParsedCallback ASTCallback); + ASTParsedCallback ASTCallback, + std::chrono::steady_clock::duration UpdateDebounce = + std::chrono::milliseconds(500)); ~TUScheduler(); /// Returns estimated memory usage for each of the currently open files. @@ -51,9 +63,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. @@ -94,6 +105,7 @@ // asynchronously. llvm::Optional PreambleTasks; llvm::Optional WorkerThreads; + std::chrono::steady_clock::duration UpdateDebounce; }; } // namespace clangd } // namespace clang Index: clangd/TUScheduler.cpp =================================================================== --- clangd/TUScheduler.cpp +++ clangd/TUScheduler.cpp @@ -54,6 +54,7 @@ namespace clang { namespace clangd { +using std::chrono::steady_clock; namespace { class ASTWorkerHandle; @@ -69,8 +70,8 @@ /// worker. class ASTWorker { friend class ASTWorkerHandle; - ASTWorker(llvm::StringRef File, Semaphore &Barrier, CppFile AST, - bool RunSync); + ASTWorker(llvm::StringRef File, Semaphore &Barrier, CppFile AST, bool RunSync, + steady_clock::duration UpdateDebounce); public: /// Create a new ASTWorker and return a handle to it. @@ -79,12 +80,12 @@ /// synchronously instead. \p Barrier is acquired when processing each /// request, it is be used to limit the number of actively running threads. static ASTWorkerHandle Create(llvm::StringRef File, AsyncTaskRunner *Tasks, - Semaphore &Barrier, CppFile AST); + Semaphore &Barrier, CppFile AST, + steady_clock::duration UpdateDebounce); ~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,16 +102,28 @@ 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); + /// Determines the next action to perform. + /// All actions that should never run are disarded. + /// Returns a deadline for the next action. If it's expired, run now. + /// scheduleLocked() is called again at the deadline, or if requests arrive. + Deadline scheduleLocked(); + /// Should the first task in the queue be skipped instead of run? + bool shouldSkipHeadLocked() const; struct Request { UniqueFunction Action; std::string Name; + steady_clock::time_point AddTime; Context Ctx; + llvm::Optional UpdateType; }; - std::string File; + const std::string File; const bool RunSync; + // Time to wait after an update to see whether another update obsoletes it. + const steady_clock::duration UpdateDebounce; + Semaphore &Barrier; // AST and FileInputs are only accessed on the processing thread from run(). CppFile AST; @@ -123,10 +136,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; }; @@ -173,9 +183,10 @@ }; ASTWorkerHandle ASTWorker::Create(llvm::StringRef File, AsyncTaskRunner *Tasks, - Semaphore &Barrier, CppFile AST) { - std::shared_ptr Worker( - new ASTWorker(File, Barrier, std::move(AST), /*RunSync=*/!Tasks)); + Semaphore &Barrier, CppFile AST, + steady_clock::duration UpdateDebounce) { + std::shared_ptr Worker(new ASTWorker( + File, Barrier, std::move(AST), /*RunSync=*/!Tasks, UpdateDebounce)); if (Tasks) Tasks->runAsync("worker:" + llvm::sys::path::filename(File), [Worker]() { Worker->run(); }); @@ -184,9 +195,9 @@ } ASTWorker::ASTWorker(llvm::StringRef File, Semaphore &Barrier, CppFile AST, - bool RunSync) - : File(File), RunSync(RunSync), Barrier(Barrier), AST(std::move(AST)), - Done(false) { + bool RunSync, steady_clock::duration UpdateDebounce) + : File(File), RunSync(RunSync), UpdateDebounce(UpdateDebounce), + Barrier(Barrier), AST(std::move(AST)), Done(false) { if (RunSync) return; } @@ -200,14 +211,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 +226,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 +252,7 @@ }; startTask(Name, BindWithForward(Task, std::move(Action)), - /*isUpdate=*/false, llvm::None); + /*UpdateType=*/llvm::None); } std::shared_ptr @@ -271,10 +276,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 +287,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, steady_clock::now(), + Context::current().clone(), UpdateType}); + } RequestsCV.notify_all(); } @@ -305,14 +298,31 @@ Request Req; { std::unique_lock Lock(Mutex); - RequestsCV.wait(Lock, [&]() { return Done || !Requests.empty(); }); - if (Requests.empty()) { - assert(Done); - return; + for (auto Wait = scheduleLocked(); !Wait.expired(); + Wait = scheduleLocked()) { + if (Done) { + if (Requests.empty()) + return; + else // Even though Done is set, finish pending requests. + break; // However, skip delays to shutdown fast. + } + + // Tracing: we have a next request, attribute this sleep to it. + Optional Ctx; + Optional Tracer; + if (!Requests.empty()) { + Ctx.emplace(Requests.front().Ctx.clone()); + Tracer.emplace("Debounce"); + SPAN_ATTACH(*Tracer, "next_request", Requests.front().Name); + if (!(Wait == Deadline::infinity())) + SPAN_ATTACH(*Tracer, "sleep_ms", + std::chrono::duration_cast( + Wait.time() - steady_clock::now()) + .count()); + } + + wait(Lock, RequestsCV, Wait); } - // Even when Done is true, we finish processing all pending requests - // before exiting the processing loop. - Req = std::move(Requests.front()); // Leave it on the queue for now, so waiters don't see an empty queue. } // unlock Mutex @@ -326,12 +336,58 @@ { std::lock_guard Lock(Mutex); - Requests.pop(); + Requests.pop_front(); } RequestsCV.notify_all(); } } +Deadline ASTWorker::scheduleLocked() { + if (Requests.empty()) + return Deadline::infinity(); // Wait for new requests. + while (shouldSkipHeadLocked()) + Requests.pop_front(); + assert(!Requests.empty() && "skipped the whole queue"); + // Some updates aren't dead yet, but never end up being used. + // e.g. the first keystroke is live until obsoleted by the second. + // We debounce "maybe-unused" writes, sleeping 500ms 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) + return Deadline::zero(); + // Front request needs to be debounced, so determine when we're ready. + Deadline D(Requests.front().AddTime + UpdateDebounce); + return D; +} + +// 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(); }); @@ -357,10 +413,12 @@ TUScheduler::TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory, - ASTParsedCallback ASTCallback) + ASTParsedCallback ASTCallback, + steady_clock::duration UpdateDebounce) : StorePreamblesInMemory(StorePreamblesInMemory), PCHOps(std::make_shared()), - ASTCallback(std::move(ASTCallback)), Barrier(AsyncThreadsCount) { + ASTCallback(std::move(ASTCallback)), Barrier(AsyncThreadsCount), + UpdateDebounce(UpdateDebounce) { if (0 < AsyncThreadsCount) { PreambleTasks.emplace(); WorkerThreads.emplace(); @@ -389,20 +447,20 @@ } 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. ASTWorkerHandle Worker = ASTWorker::Create( File, WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier, - CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback)); + CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback), + UpdateDebounce); FD = std::unique_ptr(new FileData{Inputs, std::move(Worker)}); } 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: clangd/Threading.h =================================================================== --- clangd/Threading.h +++ 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. @@ -57,18 +50,48 @@ std::size_t FreeSlots; }; -/// A point in time we may wait for, or None to wait forever. +/// A point in time we can wait for. +/// Can be zero (don't wait) or infinity (wait forever). /// (Not time_point::max(), because many std::chrono implementations overflow). -using Deadline = llvm::Optional; -/// Makes a deadline from a timeout in seconds. +class Deadline { +public: + Deadline(std::chrono::steady_clock::time_point Time) + : Type(Finite), Time(Time) {} + static Deadline zero() { return Deadline(Zero); } + static Deadline infinity() { return Deadline(Infinite); } + + std::chrono::steady_clock::time_point time() const { + assert(Type == Finite); + return Time; + } + bool expired() const { + return (Type == Zero) || + (Type == Finite && Time < std::chrono::steady_clock::now()); + } + bool operator==(const Deadline &Other) const { + return (Type == Other.Type) && (Type != Finite || Time == Other.Time); + } + +private: + enum Type { Zero, Infinite, Finite } Type; + Deadline(enum Type Type) : Type(Type) {} + std::chrono::steady_clock::time_point Time; +}; + +/// Makes a deadline from a timeout in seconds. None means wait forever. Deadline timeoutSeconds(llvm::Optional Seconds); +/// Wait once for on CV for the specified duration. +void wait(std::unique_lock &Lock, std::condition_variable &CV, + Deadline D); /// Waits on a condition variable until F() is true or D expires. template LLVM_NODISCARD bool wait(std::unique_lock &Lock, std::condition_variable &CV, Deadline D, Func F) { - if (D) - return CV.wait_until(Lock, *D, F); - CV.wait(Lock, F); + while (!F()) { + if (D.expired()) + return false; + wait(Lock, CV, D); + } return true; } @@ -80,7 +103,7 @@ /// Destructor waits for all pending tasks to finish. ~AsyncTaskRunner(); - void wait() const { (void) wait(llvm::None); } + void wait() const { (void)wait(Deadline::infinity()); } LLVM_NODISCARD bool wait(Deadline D) const; // The name is used for tracing and debugging (e.g. to name a spawned thread). void runAsync(llvm::Twine Name, UniqueFunction Action); Index: clangd/Threading.cpp =================================================================== --- clangd/Threading.cpp +++ 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) {} @@ -66,10 +76,19 @@ Deadline timeoutSeconds(llvm::Optional Seconds) { using namespace std::chrono; if (!Seconds) - return llvm::None; + return Deadline::infinity(); return steady_clock::now() + duration_cast(duration(*Seconds)); } +void wait(std::unique_lock &Lock, std::condition_variable &CV, + Deadline D) { + if (D == Deadline::zero()) + return; + if (D == Deadline::infinity()) + return CV.wait(Lock); + CV.wait_until(Lock, D.time()); +} + } // namespace clangd } // namespace clang Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -7,6 +7,7 @@ // //===----------------------------------------------------------------------===// +#include "Annotations.h" #include "ClangdLSPServer.h" #include "ClangdServer.h" #include "Matchers.h" @@ -77,6 +78,43 @@ VFSTag LastVFSTag = VFSTag(); }; +/// For each file, record whether the last published diagnostics contained at +/// least one error. +class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer { +public: + void + onDiagnosticsReady(PathRef File, + Tagged> Diagnostics) override { + bool HadError = diagsContainErrors(Diagnostics.Value); + + std::lock_guard Lock(Mutex); + LastDiagsHadError[File] = HadError; + } + + /// Exposes all files consumed by onDiagnosticsReady in an unspecified order. + /// For each file, a bool value indicates whether the last diagnostics + /// contained an error. + std::vector> filesWithDiags() const { + std::vector> Result; + std::lock_guard Lock(Mutex); + + for (const auto &it : LastDiagsHadError) { + Result.emplace_back(it.first(), it.second); + } + + return Result; + } + + void clear() { + std::lock_guard Lock(Mutex); + LastDiagsHadError.clear(); + } + +private: + mutable std::mutex Mutex; + llvm::StringMap LastDiagsHadError; +}; + /// Replaces all patterns of the form 0x123abc with spaces std::string replacePtrsInDump(std::string const &Dump) { llvm::Regex RE("0x[0-9a-fA-F]+"); @@ -413,6 +451,72 @@ EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); } +// Test ClangdServer.reparseOpenedFiles. +TEST_F(ClangdVFSTest, ReparseOpenedFiles) { + Annotations FooSource(R"cpp( +#ifdef MACRO +$one[[static void bob() {}]] +#else +$two[[static void bob() {}]] +#endif + +int main () { bo^b (); return 0; } +)cpp"); + + Annotations BarSource(R"cpp( +#ifdef MACRO +this is an error +#endif +)cpp"); + + Annotations BazSource(R"cpp( +int hello; +)cpp"); + + MockFSProvider FS; + MockCompilationDatabase CDB; + MultipleErrorCheckingDiagConsumer DiagConsumer; + ClangdServer Server(CDB, DiagConsumer, FS, + /*AsyncThreadsCount=*/0, + /*StorePreamblesInMemory=*/true); + + auto FooCpp = testPath("foo.cpp"); + auto BarCpp = testPath("bar.cpp"); + auto BazCpp = testPath("baz.cpp"); + + FS.Files[FooCpp] = ""; + FS.Files[BarCpp] = ""; + FS.Files[BazCpp] = ""; + + CDB.ExtraClangFlags = {"-DMACRO=1"}; + Server.addDocument(FooCpp, FooSource.code()); + Server.addDocument(BarCpp, BarSource.code()); + Server.addDocument(BazCpp, BazSource.code()); + + EXPECT_THAT(DiagConsumer.filesWithDiags(), + UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true), + Pair(BazCpp, false))); + + auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point()); + EXPECT_TRUE(bool(Locations)); + EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp}, + FooSource.range("one")})); + + // Undefine MACRO, close baz.cpp. + CDB.ExtraClangFlags.clear(); + DiagConsumer.clear(); + Server.removeDocument(BazCpp); + Server.reparseOpenedFiles(); + + EXPECT_THAT(DiagConsumer.filesWithDiags(), + UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false))); + + Locations = runFindDefinitions(Server, FooCpp, FooSource.point()); + EXPECT_TRUE(bool(Locations)); + EXPECT_THAT(Locations->Value, ElementsAre(Location{URIForFile{FooCpp}, + FooSource.range("two")})); +} + TEST_F(ClangdVFSTest, MemoryUsage) { MockFSProvider FS; ErrorCheckingDiagConsumer DiagConsumer; Index: unittests/clangd/TUSchedulerTests.cpp =================================================================== --- unittests/clangd/TUSchedulerTests.cpp +++ 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,58 @@ S.remove(Added); } +TEST_F(TUSchedulerTests, WantDiagnostics) { + std::atomic CallbackCount(0); + { + // To avoid a racy test, don't allow tasks to actualy run on the worker + // thread until we've scheduled them all. + Notification Ready; + TUScheduler S(getDefaultAsyncThreadsCount(), + /*StorePreamblesInMemory=*/true, + /*ASTParsedCallback=*/nullptr); + auto Path = testPath("foo.cpp"); + 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, Debounce) { + std::atomic CallbackCount(0); + { + TUScheduler S(getDefaultAsyncThreadsCount(), + /*StorePreamblesInMemory=*/true, + /*ASTParsedCallback=*/nullptr, + /*UpdateDebounce=*/std::chrono::milliseconds(50)); + auto Path = testPath("foo.cpp"); + S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto, + [&](std::vector Diags) { + ADD_FAILURE() << "auto should have been debounced and canceled"; + }); + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto, + [&](std::vector Diags) { ++CallbackCount; }); + std::this_thread::sleep_for(std::chrono::milliseconds(60)); + S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto, + [&](std::vector Diags) { ++CallbackCount; }); + } + EXPECT_EQ(2, CallbackCount); +} + TEST_F(TUSchedulerTests, ManyUpdates) { const int FilesCount = 3; const int UpdatesPerFile = 10; @@ -132,7 +184,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),