diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h --- a/clang-tools-extra/clangd/Preamble.h +++ b/clang-tools-extra/clangd/Preamble.h @@ -27,7 +27,9 @@ #include "Diagnostics.h" #include "FS.h" #include "Headers.h" +#include "Path.h" #include "index/CanonicalIncludes.h" +#include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/PrecompiledPreamble.h" #include "clang/Tooling/CompilationDatabase.h" @@ -72,17 +74,20 @@ const CanonicalIncludes &)>; /// Build a preamble for the new inputs unless an old one can be reused. -/// If \p OldPreamble can be reused, it is returned unchanged. -/// If \p OldPreamble is null, always builds the preamble. /// If \p PreambleCallback is set, it will be run on top of the AST while -/// building the preamble. Note that if the old preamble was reused, no AST is -/// built and, therefore, the callback will not be executed. +/// building the preamble. std::shared_ptr buildPreamble(PathRef FileName, CompilerInvocation CI, - std::shared_ptr OldPreamble, const ParseInputs &Inputs, bool StoreInMemory, PreambleParsedCallback PreambleCallback); +/// Returns true if \p Preamble is reusable for \p Inputs. Note that it will +/// return true when some missing headers are now available. +/// FIXME: Should return more information about the delta between \p Preamble +/// and \p Inputs, e.g. new headers. +bool isPreambleCompatible(const PreambleData &Preamble, + const ParseInputs &Inputs, PathRef FileName, + const CompilerInvocation &CI); } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -90,7 +90,6 @@ std::shared_ptr buildPreamble(PathRef FileName, CompilerInvocation CI, - std::shared_ptr OldPreamble, const ParseInputs &Inputs, bool StoreInMemory, PreambleParsedCallback PreambleCallback) { // Note that we don't need to copy the input contents, preamble can live @@ -100,23 +99,6 @@ auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0); - if (OldPreamble && - compileCommandsAreEqual(Inputs.CompileCommand, - OldPreamble->CompileCommand) && - OldPreamble->Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds, - Inputs.FS.get())) { - vlog("Reusing preamble version {0} for version {1} of {2}", - OldPreamble->Version, Inputs.Version, FileName); - return OldPreamble; - } - if (OldPreamble) - vlog("Rebuilding invalidated preamble for {0} version {1} " - "(previous was version {2})", - FileName, Inputs.Version, OldPreamble->Version); - else - vlog("Building first preamble for {0} version {1}", FileName, - Inputs.Version); - trace::Span Tracer("BuildPreamble"); SPAN_ATTACH(Tracer, "File", FileName); StoreDiags PreambleDiagnostics; @@ -172,5 +154,17 @@ } } +bool isPreambleCompatible(const PreambleData &Preamble, + const ParseInputs &Inputs, PathRef FileName, + const CompilerInvocation &CI) { + auto ContentsBuffer = + llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName); + auto Bounds = + ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0); + return compileCommandsAreEqual(Inputs.CompileCommand, + Preamble.CompileCommand) && + Preamble.Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds, + Inputs.FS.get()); +} } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -5,41 +5,46 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // //===----------------------------------------------------------------------===// -// For each file, managed by TUScheduler, we create a single ASTWorker that -// manages an AST for that file. All operations that modify or read the AST are -// run on a separate dedicated thread asynchronously in FIFO order. +// TUScheduler manages a worker per active file. This ASTWorker processes +// updates (modifications to file contents) and reads (actions performed on +// preamble/AST) to the file. // -// We start processing each update immediately after we receive it. If two or -// more updates come subsequently without reads in-between, we attempt to drop -// an older one to not waste time building the ASTs we don't need. +// Each ASTWorker owns a dedicated thread to process updates and reads to the +// relevant file. Any request gets queued in FIFO order to be processed by that +// thread. // -// The processing thread of the ASTWorker is also responsible for building the -// preamble. However, unlike AST, the same preamble can be read concurrently, so -// we run each of async preamble reads on its own thread. +// An update request replaces current praser inputs to ensure any subsequent +// read sees the version of the file they were requested. It will also issue a +// build for new inputs. // -// To limit the concurrent load that clangd produces we maintain a semaphore -// that keeps more than a fixed number of threads from running concurrently. +// ASTWorker processes the file in two parts, a preamble and a main-file +// section. A preamble can be reused between multiple versions of the file until +// invalidated by a modification to a header, compile commands or modification +// to relevant part of the current file. Such a preamble is called compatible. +// An update is considered dead if no read was issued for that version and +// diagnostics weren't requested by client or could be generated for a later +// version of the file. ASTWorker eliminates such requests as they are +// redundant. // -// Rationale for cancelling updates. -// LSP clients can send updates to clangd on each keystroke. Some files take -// significant time to parse (e.g. a few seconds) and clangd can get starved by -// the updates to those files. Therefore we try to process only the last update, -// if possible. -// Our current strategy to do that is the following: -// - For each update we immediately schedule rebuild of the AST. -// - Rebuild of the AST checks if it was cancelled before doing any actual work. -// If it was, it does not do an actual rebuild, only reports llvm::None to the -// callback -// - When adding an update, we cancel the last update in the queue if it didn't -// have any reads. -// There is probably a optimal ways to do that. One approach we might take is -// the following: -// - For each update we remember the pending inputs, but delay rebuild of the -// AST for some timeout. -// - If subsequent updates come before rebuild was started, we replace the -// pending inputs and reset the timer. -// - If any reads of the AST are scheduled, we start building the AST -// immediately. +// In the presence of stale (non-compatible) preambles, ASTWorker won't publish +// diagnostics for update requests. Read requests will be served with ASTs build +// with stale preambles, unless the read is picky and requires a compatible +// preamble. In such cases it will block until new preamble is built. +// +// ASTWorker owns a PreambleThread for building preambles. If the preamble gets +// invalidated by an update request, a new build will be requested on +// PreambleThread. Since PreambleThread only receives requests for newer +// versions of the file, in case of multiple requests it will only build the +// last one and skip requests in between. Unless client force requested +// diagnostics(WantDiagnostics::Yes). +// +// When a new preamble is built, a "golden" AST is immediately built from that +// version of the file. This ensures diagnostics get updated even if the queue +// is full. +// +// Some read requests might just need preamble. Since preambles can be read +// concurrently, ASTWorker runs these requests on their own thread. These +// requests will receive latest build preamble, which might possibly be stale. #include "TUScheduler.h" #include "Cancellation.h" @@ -56,6 +61,8 @@ #include "index/CanonicalIncludes.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Tooling/CompilationDatabase.h" +#include "llvm/ADT/FunctionExtras.h" +#include "llvm/ADT/None.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/ScopeExit.h" @@ -64,14 +71,20 @@ #include "llvm/ADT/StringRef.h" #include "llvm/Support/Errc.h" #include "llvm/Support/ErrorHandling.h" +#include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Path.h" #include "llvm/Support/Threading.h" #include +#include +#include #include #include #include #include #include +#include +#include +#include namespace clang { namespace clangd { @@ -190,37 +203,26 @@ ParsingCallbacks &Callbacks; }; -/// Responsible for building and providing access to the preamble of a TU. -/// Whenever the thread is idle and the preamble is outdated, it starts to build -/// a fresh preamble from the latest inputs. If RunSync is true, preambles are -/// built synchronously in update() instead. +/// Responsible for building preambles. Whenever the thread is idle and the +/// preamble is outdated, it starts to build a fresh preamble from the latest +/// inputs. If RunSync is true, preambles are built synchronously in update() +/// instead. class PreambleThread { public: PreambleThread(llvm::StringRef FileName, ParsingCallbacks &Callbacks, bool StorePreambleInMemory, bool RunSync, - SynchronizedTUStatus &Status) + SynchronizedTUStatus &Status, ASTWorker &AW) : FileName(FileName), Callbacks(Callbacks), - StoreInMemory(StorePreambleInMemory), RunSync(RunSync), Status(Status) { - } - - size_t getUsedBytes() const { - auto Preamble = latest(); - return Preamble ? Preamble->Preamble.getSize() : 0; - } + StoreInMemory(StorePreambleInMemory), RunSync(RunSync), Status(Status), + ASTPeer(AW) {} /// It isn't guaranteed that each requested version will be built. If there /// are multiple update requests while building a preamble, only the last one /// will be built. - void update(CompilerInvocation *CI, ParseInputs PI) { - // If compiler invocation was broken, just fail out early. - if (!CI) { - // Make sure anyone waiting for the preamble gets notified it could not be - // built. - BuiltFirst.notify(); - return; - } - // Make possibly expensive copy while not holding the lock. - Request Req = {std::make_unique(*CI), std::move(PI)}; + void update(std::unique_ptr CI, ParseInputs PI, + std::vector CIDiags, WantDiagnostics WantDiags) { + Request Req = {std::move(CI), std::move(PI), std::move(CIDiags), WantDiags, + Context::current().clone()}; if (RunSync) { build(std::move(Req)); Status.update([](TUStatus &Status) { @@ -230,7 +232,9 @@ } { std::lock_guard Lock(Mutex); - assert(!Done && "Build request to PreambleWorker after stop"); + // If shutdown is issued, don't bother building. + if (Done) + return; NextReq = std::move(Req); } // Let the worker thread know there's a request, notify_one is safe as there @@ -238,19 +242,7 @@ ReqCV.notify_all(); } - /// Blocks until at least a single request has been processed. Note that it - /// will unblock even after an unsuccessful build. - void waitForFirst() const { BuiltFirst.wait(); } - - /// Returns the latest built preamble, might be null if no preamble has been - /// built or latest attempt resulted in a failure. - std::shared_ptr latest() const { - std::lock_guard Lock(Mutex); - return LatestBuild; - } - void run() { - dlog("Starting preamble worker for {0}", FileName); while (true) { { std::unique_lock Lock(Mutex); @@ -262,6 +254,8 @@ CurrentReq = std::move(*NextReq); NextReq.reset(); } + + WithContext Guard(std::move(CurrentReq->Ctx)); // Build the preamble and let the waiters know about it. build(std::move(*CurrentReq)); bool IsEmpty = false; @@ -280,17 +274,16 @@ } ReqCV.notify_all(); } - // We are no longer going to build any preambles, let the waiters know that. - BuiltFirst.notify(); - dlog("Preamble worker for {0} finished", FileName); + dlog("Preamble worker for {0} stopped", FileName); } /// Signals the run loop to exit. void stop() { - dlog("Stopping preamble worker for {0}", FileName); + dlog("Preamble worker for {0} received stop", FileName); { std::lock_guard Lock(Mutex); Done = true; + NextReq.reset(); } // Let the worker thread know that it should stop. ReqCV.notify_all(); @@ -307,6 +300,9 @@ struct Request { std::unique_ptr CI; ParseInputs Inputs; + std::vector CIDiags; + WantDiagnostics WantDiags; + Context Ctx; }; bool isDone() { @@ -314,36 +310,9 @@ return Done; } - /// Builds a preamble for Req and caches it. Might re-use the latest built - /// preamble if it is valid for Req. Also signals waiters about the build. - /// FIXME: We shouldn't cache failed preambles, if we've got a successful - /// build before. - void build(Request Req) { - assert(Req.CI && "Got preamble request with null compiler invocation"); - const ParseInputs &Inputs = Req.Inputs; - std::shared_ptr OldPreamble = - Inputs.ForceRebuild ? nullptr : latest(); - - Status.update([&](TUStatus &Status) { - Status.PreambleActivity = PreambleAction::Building; - }); - - auto Preamble = clang::clangd::buildPreamble( - FileName, std::move(*Req.CI), OldPreamble, Inputs, StoreInMemory, - [this, Version(Inputs.Version)]( - ASTContext &Ctx, std::shared_ptr PP, - const CanonicalIncludes &CanonIncludes) { - Callbacks.onPreambleAST(FileName, Version, Ctx, std::move(PP), - CanonIncludes); - }); - { - std::lock_guard Lock(Mutex); - // LatestBuild might be the last reference to old preamble, do not trigger - // destructor while holding the lock. - std::swap(LatestBuild, Preamble); - } - BuiltFirst.notify(); - } + /// Builds a preamble for \p Req, might reuse LatestBuild if possible. + /// Notifies ASTWorker after build finishes. + void build(Request Req); mutable std::mutex Mutex; bool Done = false; /* GUARDED_BY(Mutex) */ @@ -351,16 +320,17 @@ llvm::Optional CurrentReq; /* GUARDED_BY(Mutex) */ // Signaled whenever a thread populates NextReq or worker thread builds a // Preamble. - mutable std::condition_variable ReqCV; /* GUARDED_BY(Mutex) */ - std::shared_ptr LatestBuild; /* GUARDED_BY(Mutex) */ + mutable std::condition_variable ReqCV; /* GUARDED_BY(Mutex) */ + // Accessed only by preamble thread. + std::shared_ptr LatestBuild; - Notification BuiltFirst; const Path FileName; ParsingCallbacks &Callbacks; const bool StoreInMemory; const bool RunSync; SynchronizedTUStatus &Status; + ASTWorker &ASTPeer; }; class ASTWorkerHandle; @@ -404,6 +374,14 @@ std::shared_ptr getPossiblyStalePreamble() const; + /// Used to inform ASTWorker about a new preamble build by PreambleThread. + /// Diagnostics are only published through this callback. This ensures they + /// are always for newer versions of the file, as the callback gets called in + /// the same order as update requests. + void updatePreamble(std::unique_ptr CI, ParseInputs PI, + std::shared_ptr Preamble, + std::vector CIDiags, WantDiagnostics WantDiags); + /// Obtain a preamble reflecting all updates so far. Threadsafe. /// It may be delivered immediately, or later on the worker thread. void getCurrentPreamble( @@ -421,6 +399,12 @@ bool isASTCached() const; private: + /// Publishes diagnostics for \p Inputs. It will build an AST or reuse the + /// cached one if applicable. Assumes LatestPreamble is compatible for \p + /// Inputs. + void generateDiagnostics(std::unique_ptr Invocation, + ParseInputs Inputs, std::vector CIDiags); + // Must be called exactly once on processing thread. Will return after // stop() is called on a separate thread and all pending requests are // processed. @@ -464,6 +448,9 @@ const GlobalCompilationDatabase &CDB; /// Callback invoked when preamble or main file AST is built. ParsingCallbacks &Callbacks; + /// Latest build preamble for current TU. + std::shared_ptr LatestPreamble; + Notification BuiltFirstPreamble; Semaphore &Barrier; /// Whether the 'onMainAST' callback ran for the current FileInputs. @@ -480,6 +467,7 @@ /// Set to true to signal run() to finish processing. bool Done; /* GUARDED_BY(Mutex) */ std::deque Requests; /* GUARDED_BY(Mutex) */ + std::queue PreambleRequests; /* GUARDED_BY(Mutex) */ llvm::Optional CurrentRequest; /* GUARDED_BY(Mutex) */ mutable std::condition_variable RequestsCV; /// Guards the callback that publishes results of AST-related computations @@ -494,7 +482,7 @@ bool CanPublishResults = true; /* GUARDED_BY(PublishMu) */ SynchronizedTUStatus Status; - PreambleThread PW; + PreambleThread PreamblePeer; }; /// A smart-pointer-like class that points to an active ASTWorker. @@ -551,7 +539,7 @@ Tasks->runAsync("ASTWorker:" + llvm::sys::path::filename(FileName), [Worker]() { Worker->run(); }); Tasks->runAsync("PreambleWorker:" + llvm::sys::path::filename(FileName), - [Worker]() { Worker->PW.run(); }); + [Worker]() { Worker->PreamblePeer.run(); }); } return ASTWorkerHandle(std::move(Worker)); @@ -564,9 +552,10 @@ : IdleASTs(LRUCache), RunSync(RunSync), UpdateDebounce(UpdateDebounce), FileName(FileName), CDB(CDB), Callbacks(Callbacks), Barrier(Barrier), Done(false), Status(FileName, Callbacks), - // FIXME: Run preambleworker async. - PW(FileName, Callbacks, StorePreamblesInMemory, /*RunSync=*/true, - Status) { + PreamblePeer(FileName, Callbacks, StorePreamblesInMemory, + // FIXME: Run PreamblePeer asynchronously once ast patching + // is available. + /*RunSync=*/true, Status, *this) { auto Inputs = std::make_shared(); // Set a fallback command because compile command can be accessed before // `Inputs` is initialized. Other fields are only used after initialization @@ -589,14 +578,6 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) { std::string TaskName = llvm::formatv("Update ({0})", Inputs.Version); auto Task = [=]() mutable { - auto RunPublish = [&](llvm::function_ref Publish) { - // Ensure we only publish results from the worker if the file was not - // removed, making sure there are not race conditions. - std::lock_guard Lock(PublishMu); - if (CanPublishResults) - Publish(); - }; - // Get the actual command as `Inputs` does not have a command. // FIXME: some build systems like Bazel will take time to preparing // environment to build the file, it would be nice if we could emit a @@ -607,33 +588,31 @@ else // FIXME: consider using old command if it's not a fallback one. Inputs.CompileCommand = CDB.getFallbackCommand(FileName); - auto PrevInputs = getCurrentFileInputs(); - // Will be used to check if we can avoid rebuilding the AST. + bool InputsAreTheSame = - std::tie(PrevInputs->CompileCommand, PrevInputs->Contents) == + std::tie(FileInputs->CompileCommand, FileInputs->Contents) == std::tie(Inputs.CompileCommand, Inputs.Contents); + // Cached AST is invalidated. + if (!InputsAreTheSame) { + IdleASTs.take(this); + RanASTCallback = false; + } - bool RanCallbackForPrevInputs = RanASTCallback; + // Update current inputs so that subsequent reads can see them. { std::lock_guard Lock(Mutex); FileInputs = std::make_shared(Inputs); } - RanASTCallback = false; + log("ASTWorker building file {0} version {1} with command {2}\n[{3}]\n{4}", FileName, Inputs.Version, Inputs.CompileCommand.Heuristic, Inputs.CompileCommand.Directory, llvm::join(Inputs.CompileCommand.CommandLine, " ")); - // Rebuild the preamble and the AST. + StoreDiags CompilerInvocationDiagConsumer; std::vector CC1Args; std::unique_ptr Invocation = buildCompilerInvocation( Inputs, CompilerInvocationDiagConsumer, &CC1Args); - // This is true for now, as we always block until new preamble is build. - // Once we start to block preambles out-of-order we need to make sure - // OldPreamble refers to the preamble that was used to build last AST. - auto OldPreamble = PW.latest(); - PW.update(Invocation.get(), Inputs); - auto NewPreamble = PW.latest(); // Log cc1 args even (especially!) if creating invocation failed. if (!CC1Args.empty()) vlog("Driver produced command: cc1 {0}", llvm::join(CC1Args, " ")); @@ -643,109 +622,27 @@ elog("Could not build CompilerInvocation for file {0}", FileName); // Remove the old AST if it's still in cache. IdleASTs.take(this); + RanASTCallback = false; // Report the diagnostics we collected when parsing the command line. Callbacks.onFailedAST(FileName, Inputs.Version, - std::move(CompilerInvocationDiags), RunPublish); - return; - } - - bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble); - // Before doing the expensive AST reparse, we want to release our reference - // to the old preamble, so it can be freed if there are no other references - // to it. - OldPreamble.reset(); - Status.update([&](TUStatus &Status) { - Status.ASTActivity.K = ASTAction::Building; - Status.ASTActivity.Name = std::move(TaskName); - }); - if (!CanReuseAST) { - IdleASTs.take(this); // Remove the old AST if it's still in cache. - } else { - // We don't need to rebuild the AST, check if we need to run the callback. - if (RanCallbackForPrevInputs) { - RanASTCallback = true; - // Take a shortcut and don't report the diagnostics, since they should - // not changed. All the clients should handle the lack of OnUpdated() - // call anyway to handle empty result from buildAST. - // FIXME(ibiryukov): the AST could actually change if non-preamble - // includes changed, but we choose to ignore it. - // FIXME(ibiryukov): should we refresh the cache in IdleASTs for the - // current file at this point? - log("Skipping rebuild of the AST for {0}, inputs are the same.", - FileName); - - Status.update([](TUStatus &Status) { - Status.Details.ReuseAST = true; - Status.Details.BuildFailed = false; - }); - return; - } - } - - // We only need to build the AST if diagnostics were requested. - if (WantDiags == WantDiagnostics::No) + std::move(CompilerInvocationDiags), + [&](llvm::function_ref Publish) { + // Ensure we only publish results from the worker + // if the file was not removed, making sure there + // are not race conditions. + std::lock_guard Lock(PublishMu); + if (CanPublishResults) + Publish(); + }); + // Make sure anyone waiting for the preamble gets notified it could not be + // built. + BuiltFirstPreamble.notify(); return; - - { - std::lock_guard Lock(PublishMu); - // No need to rebuild the AST if we won't send the diagnostics. However, - // note that we don't prevent preamble rebuilds. - if (!CanPublishResults) - return; } - // Get the AST for diagnostics. - llvm::Optional> AST = IdleASTs.take(this); - auto RebuildStartTime = DebouncePolicy::clock::now(); - if (!AST) { - llvm::Optional NewAST = - buildAST(FileName, std::move(Invocation), CompilerInvocationDiags, - Inputs, NewPreamble); - // buildAST fails. - Status.update([&](TUStatus &Status) { - Status.Details.ReuseAST = false; - Status.Details.BuildFailed = !NewAST.hasValue(); - }); - AST = NewAST ? std::make_unique(std::move(*NewAST)) : nullptr; - } else { - // We are reusing the AST. - Status.update([](TUStatus &Status) { - Status.Details.ReuseAST = true; - Status.Details.BuildFailed = false; - }); - } - - // 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. - // Note *AST can still be null if buildAST fails. - if (*AST) { - { - // Try to record the AST-build time, to inform future update debouncing. - // This is best-effort only: if the lock is held, don't bother. - auto RebuildDuration = DebouncePolicy::clock::now() - RebuildStartTime; - std::unique_lock Lock(Mutex, std::try_to_lock); - if (Lock.owns_lock()) { - // Do not let RebuildTimes grow beyond its small-size (i.e. capacity). - if (RebuildTimes.size() == RebuildTimes.capacity()) - RebuildTimes.erase(RebuildTimes.begin()); - RebuildTimes.push_back(RebuildDuration); - } - } - trace::Span Span("Running main AST callback"); - - Callbacks.onMainAST(FileName, **AST, RunPublish); - RanASTCallback = true; - } else { - // Failed to build the AST, at least report diagnostics from the command - // line if there were any. - // FIXME: we might have got more errors while trying to build the AST, - // surface them too. - Callbacks.onFailedAST(FileName, Inputs.Version, CompilerInvocationDiags, - RunPublish); - } - // Stash the AST in the cache for further use. - IdleASTs.put(this, std::move(*AST)); + PreamblePeer.update(std::move(Invocation), std::move(Inputs), + std::move(CompilerInvocationDiags), WantDiags); + return; }; startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation); } @@ -766,6 +663,9 @@ // Try rebuilding the AST. vlog("ASTWorker rebuilding evicted AST to run {0}: {1} version {2}", Name, FileName, CurrentInputs->Version); + // FIXME: We might need to build a patched ast once preamble thread starts + // running async. Currently getPossiblyStalePreamble below will always + // return a compatible preamble as ASTWorker::update blocks. llvm::Optional NewAST = Invocation ? buildAST(FileName, std::move(Invocation), CompilerInvocationDiagConsumer.take(), @@ -787,9 +687,184 @@ startTask(Name, std::move(Task), /*UpdateType=*/None, Invalidation); } +void PreambleThread::build(Request Req) { + assert(Req.CI && "Got preamble request with null compiler invocation"); + const ParseInputs &Inputs = Req.Inputs; + + Status.update([&](TUStatus &Status) { + Status.PreambleActivity = PreambleAction::Building; + }); + auto _ = llvm::make_scope_exit([this, &Req] { + ASTPeer.updatePreamble(std::move(Req.CI), std::move(Req.Inputs), + LatestBuild, std::move(Req.CIDiags), + std::move(Req.WantDiags)); + }); + + if (!LatestBuild || Inputs.ForceRebuild) { + vlog("Building first preamble for {0} version {1}", FileName, + Inputs.Version); + } else if (isPreambleCompatible(*LatestBuild, Inputs, FileName, *Req.CI)) { + vlog("Reusing preamble version {0} for version {1} of {2}", + LatestBuild->Version, Inputs.Version, FileName); + return; + } else { + vlog("Rebuilding invalidated preamble for {0} version {1} (previous was " + "version {2})", + FileName, Inputs.Version, LatestBuild->Version); + } + + LatestBuild = clang::clangd::buildPreamble( + FileName, *Req.CI, Inputs, StoreInMemory, + [this, Version(Inputs.Version)](ASTContext &Ctx, + std::shared_ptr PP, + const CanonicalIncludes &CanonIncludes) { + Callbacks.onPreambleAST(FileName, Version, Ctx, std::move(PP), + CanonIncludes); + }); +} + +void ASTWorker::updatePreamble(std::unique_ptr CI, + ParseInputs PI, + std::shared_ptr Preamble, + std::vector CIDiags, + WantDiagnostics WantDiags) { + std::string TaskName = llvm::formatv("Build AST for ({0})", PI.Version); + // Store preamble and build diagnostics with new preamble if requested. + auto Task = [this, Preamble = std::move(Preamble), CI = std::move(CI), + PI = std::move(PI), CIDiags = std::move(CIDiags), + WantDiags = std::move(WantDiags)]() mutable { + // Update the preamble inside ASTWorker queue to ensure atomicity. As a task + // running inside ASTWorker assumes internals won't change until it + // finishes. + if (Preamble != LatestPreamble) { + // Cached AST is no longer valid. + IdleASTs.take(this); + RanASTCallback = false; + std::lock_guard Lock(Mutex); + // LatestPreamble might be the last reference to old preamble, do not + // trigger destructor while holding the lock. + std::swap(LatestPreamble, Preamble); + } + // Give up our ownership to old preamble before starting expensive AST + // build. + Preamble.reset(); + BuiltFirstPreamble.notify(); + // We only need to build the AST if diagnostics were requested. + if (WantDiags == WantDiagnostics::No) + return; + // Report diagnostics with the new preamble to ensure progress. Otherwise + // diagnostics might get stale indefinitely if user keeps invalidating the + // preamble. + generateDiagnostics(std::move(CI), std::move(PI), std::move(CIDiags)); + }; + if (RunSync) { + Task(); + return; + } + { + std::lock_guard Lock(Mutex); + PreambleRequests.push({std::move(Task), std::move(TaskName), + steady_clock::now(), Context::current().clone(), + llvm::None, TUScheduler::NoInvalidation, nullptr}); + } + RequestsCV.notify_all(); +} + +void ASTWorker::generateDiagnostics( + std::unique_ptr Invocation, ParseInputs Inputs, + std::vector CIDiags) { + assert(Invocation); + // No need to rebuild the AST if we won't send the diagnostics. + { + std::lock_guard Lock(PublishMu); + if (!CanPublishResults) + return; + } + // Used to check whether we can update AST cache. + bool InputsAreLatest = + std::tie(FileInputs->CompileCommand, FileInputs->Contents) == + std::tie(Inputs.CompileCommand, Inputs.Contents); + // Take a shortcut and don't report the diagnostics, since they should be the + // same. All the clients should handle the lack of OnUpdated() call anyway to + // handle empty result from buildAST. + // FIXME: the AST could actually change if non-preamble includes changed, + // but we choose to ignore it. + if (InputsAreLatest && RanASTCallback) + return; + + // Get the AST for diagnostics, either build it or use the cached one. + std::string TaskName = llvm::formatv("Build AST ({0})", Inputs.Version); + Status.update([&](TUStatus &Status) { + Status.ASTActivity.K = ASTAction::Building; + Status.ASTActivity.Name = std::move(TaskName); + }); + // We might be able to reuse the last we've built for a read request. + // FIXME: It might be better to not reuse this AST. That way queued AST builds + // won't be required for diags. + llvm::Optional> AST = IdleASTs.take(this); + if (!AST) { + auto RebuildStartTime = DebouncePolicy::clock::now(); + llvm::Optional NewAST = buildAST( + FileName, std::move(Invocation), CIDiags, Inputs, LatestPreamble); + auto RebuildDuration = DebouncePolicy::clock::now() - RebuildStartTime; + // Try to record the AST-build time, to inform future update debouncing. + // This is best-effort only: if the lock is held, don't bother. + std::unique_lock Lock(Mutex, std::try_to_lock); + if (Lock.owns_lock()) { + // Do not let RebuildTimes grow beyond its small-size (i.e. + // capacity). + if (RebuildTimes.size() == RebuildTimes.capacity()) + RebuildTimes.erase(RebuildTimes.begin()); + RebuildTimes.push_back(RebuildDuration); + Lock.unlock(); + } + Status.update([&](TUStatus &Status) { + Status.Details.ReuseAST = false; + Status.Details.BuildFailed = !NewAST.hasValue(); + }); + AST = NewAST ? std::make_unique(std::move(*NewAST)) : nullptr; + } else { + assert(InputsAreLatest && !RanASTCallback && + "forgot to invalidate cached ast?"); + log("Skipping rebuild of the AST for {0}, inputs are the same.", FileName); + Status.update([](TUStatus &Status) { + Status.Details.ReuseAST = true; + Status.Details.BuildFailed = false; + }); + } + + // Publish diagnostics. + auto RunPublish = [&](llvm::function_ref Publish) { + // Ensure we only publish results from the worker if the file was not + // removed, making sure there are not race conditions. + std::lock_guard Lock(PublishMu); + if (CanPublishResults) + Publish(); + }; + if (*AST) { + trace::Span Span("Running main AST callback"); + Callbacks.onMainAST(FileName, **AST, RunPublish); + } else { + // Failed to build the AST, at least report diagnostics from the + // command line if there were any. + // FIXME: we might have got more errors while trying to build the + // AST, surface them too. + Callbacks.onFailedAST(FileName, Inputs.Version, CIDiags, RunPublish); + } + + // AST might've been built for an older version of the source, as ASTWorker + // queue raced ahead while we were waiting on the preamble. In that case the + // queue can't reuse the AST. + if (InputsAreLatest) { + RanASTCallback = *AST != nullptr; + IdleASTs.put(this, std::move(*AST)); + } +} + std::shared_ptr ASTWorker::getPossiblyStalePreamble() const { - return PW.latest(); + std::lock_guard Lock(Mutex); + return LatestPreamble; } void ASTWorker::getCurrentPreamble( @@ -822,7 +897,7 @@ RequestsCV.notify_all(); } -void ASTWorker::waitForFirstPreamble() const { PW.waitForFirst(); } +void ASTWorker::waitForFirstPreamble() const { BuiltFirstPreamble.wait(); } std::shared_ptr ASTWorker::getCurrentFileInputs() const { std::unique_lock Lock(Mutex); @@ -856,6 +931,9 @@ assert(!Done && "stop() called twice"); Done = true; } + // We are no longer going to build any preambles, let the waiters know that. + BuiltFirstPreamble.notify(); + PreamblePeer.stop(); Status.stop(); RequestsCV.notify_all(); } @@ -899,13 +977,14 @@ } void ASTWorker::run() { - auto _ = llvm::make_scope_exit([this] { PW.stop(); }); while (true) { { std::unique_lock Lock(Mutex); assert(!CurrentRequest && "A task is already running, multiple workers?"); for (auto Wait = scheduleLocked(); !Wait.expired(); Wait = scheduleLocked()) { + assert(PreambleRequests.empty() && + "Preamble updates should be scheduled immediately"); if (Done) { if (Requests.empty()) return; @@ -934,8 +1013,15 @@ wait(Lock, RequestsCV, Wait); } - CurrentRequest = std::move(Requests.front()); - Requests.pop_front(); + // Any request in ReceivedPreambles is at least as old as the + // Requests.front(), so prefer them first to preserve LSP order. + if (!PreambleRequests.empty()) { + CurrentRequest = std::move(PreambleRequests.front()); + PreambleRequests.pop(); + } else { + CurrentRequest = std::move(Requests.front()); + Requests.pop_front(); + } } // unlock Mutex // It is safe to perform reads to CurrentRequest without holding the lock as @@ -962,7 +1048,7 @@ { std::lock_guard Lock(Mutex); CurrentRequest.reset(); - IsEmpty = Requests.empty(); + IsEmpty = Requests.empty() && PreambleRequests.empty(); } if (IsEmpty) { Status.update([&](TUStatus &Status) { @@ -975,6 +1061,9 @@ } Deadline ASTWorker::scheduleLocked() { + // Process new preambles immediately. + if (!PreambleRequests.empty()) + return Deadline::zero(); if (Requests.empty()) return Deadline::infinity(); // Wait for new requests. // Handle cancelled requests first so the rest of the scheduler doesn't. @@ -1045,8 +1134,9 @@ bool ASTWorker::blockUntilIdle(Deadline Timeout) const { std::unique_lock Lock(Mutex); - return wait(Lock, RequestsCV, Timeout, - [&] { return Requests.empty() && !CurrentRequest; }); + return wait(Lock, RequestsCV, Timeout, [&] { + return PreambleRequests.empty() && Requests.empty() && !CurrentRequest; + }); } // Render a TUAction to a user-facing string representation. @@ -1221,6 +1311,9 @@ // Future is populated if the task needs a specific preamble. std::future> ConsistentPreamble; + // FIXME: Currently this only holds because ASTWorker blocks after issuing a + // preamble build. Get rid of consistent reads or make them build on the + // calling thread instead. if (Consistency == Consistent) { std::promise> Promise; ConsistentPreamble = Promise.get_future(); diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -112,9 +112,8 @@ ADD_FAILURE() << "Couldn't build CompilerInvocation"; return {}; } - auto Preamble = - buildPreamble(testPath(TU.Filename), *CI, /*OldPreamble=*/nullptr, Inputs, - /*InMemory=*/true, /*Callback=*/nullptr); + auto Preamble = buildPreamble(testPath(TU.Filename), *CI, Inputs, + /*InMemory=*/true, /*Callback=*/nullptr); return codeComplete(testPath(TU.Filename), Inputs.CompileCommand, Preamble.get(), TU.Code, Point, Inputs.FS, Opts); } @@ -518,16 +517,16 @@ AllOf(Named("complete_static_member"), Kind(CompletionItemKind::Property)))); - Results = completions( + Results = completions( R"cpp( enum Color { Red }; Color u = ^ )cpp"); - EXPECT_THAT(Results.Completions, - Contains( - AllOf(Named("Red"), Kind(CompletionItemKind::EnumMember)))); + EXPECT_THAT( + Results.Completions, + Contains(AllOf(Named("Red"), Kind(CompletionItemKind::EnumMember)))); } TEST(CompletionTest, NoDuplicates) { @@ -1046,9 +1045,8 @@ ADD_FAILURE() << "Couldn't build CompilerInvocation"; return {}; } - auto Preamble = - buildPreamble(testPath(TU.Filename), *CI, /*OldPreamble=*/nullptr, Inputs, - /*InMemory=*/true, /*Callback=*/nullptr); + auto Preamble = buildPreamble(testPath(TU.Filename), *CI, Inputs, + /*InMemory=*/true, /*Callback=*/nullptr); if (!Preamble) { ADD_FAILURE() << "Couldn't build Preamble"; return {}; @@ -1712,7 +1710,7 @@ CodeCompleteOptions Opts; Opts.IncludeFixIts = true; - const char* Code = + const char *Code = R"cpp( class Auxilary { public: @@ -1746,7 +1744,7 @@ TEST(CompletionTest, FixItForDotToArrow) { CodeCompleteOptions Opts; Opts.IncludeFixIts = true; - const char* Code = + const char *Code = R"cpp( class Auxilary { public: @@ -1850,7 +1848,7 @@ R"cpp( #include "foo/abc/[[fo^o.h"]] )cpp", - }; + }; for (const auto &Text : TestCodes) { Annotations TestCode(Text); TU.Code = TestCode.code().str(); @@ -2222,10 +2220,9 @@ Sym.IncludeHeaders.emplace_back("\"foo.h\"", 2); Sym.IncludeHeaders.emplace_back("\"bar.h\"", 1000); - EXPECT_THAT( - completions(TU, Test.point(), {Sym}).Completions, - UnorderedElementsAre( - AllOf(Named("Func"), HasInclude("\"foo.h\""), Not(InsertInclude())))); + EXPECT_THAT(completions(TU, Test.point(), {Sym}).Completions, + UnorderedElementsAre(AllOf(Named("Func"), HasInclude("\"foo.h\""), + Not(InsertInclude())))); } TEST(CompletionTest, MergeMacrosFromIndexAndSema) { diff --git a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp --- a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -286,7 +286,7 @@ FileIndex Index; bool IndexUpdated = false; - buildPreamble(FooCpp, *CI, /*OldPreamble=*/nullptr, PI, + buildPreamble(FooCpp, *CI, PI, /*StoreInMemory=*/true, [&](ASTContext &Ctx, std::shared_ptr PP, const CanonicalIncludes &CanonIncludes) { @@ -424,7 +424,7 @@ } TEST(FileIndexTest, MergeMainFileSymbols) { - const char* CommonHeader = "void foo();"; + const char *CommonHeader = "void foo();"; TestTU Header = TestTU::withCode(CommonHeader); TestTU Cpp = TestTU::withCode("void foo() {}"); Cpp.Filename = "foo.cpp"; diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -24,6 +24,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include +#include #include #include #include @@ -283,6 +284,7 @@ S.runWithPreamble("StaleRead", Path, TUScheduler::Stale, [&](Expected Pre) { ASSERT_TRUE(bool(Pre)); + ASSERT_TRUE(Pre->Preamble); EXPECT_EQ(Pre->Preamble->Version, "A"); EXPECT_THAT(includes(Pre->Preamble), ElementsAre("")); @@ -292,11 +294,13 @@ S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent, [&](Expected Pre) { ASSERT_TRUE(bool(Pre)); + ASSERT_TRUE(Pre->Preamble); EXPECT_EQ(Pre->Preamble->Version, "B"); EXPECT_THAT(includes(Pre->Preamble), ElementsAre("")); ++CallbackCount; }); + S.blockUntilIdle(timeoutSeconds(10)); } EXPECT_EQ(2, CallbackCount); } @@ -731,11 +735,14 @@ )cpp"; ParseInputs Inputs = getInputs(Source, SourceContents); + std::atomic DiagCount(0); // Update the source contents, which should trigger an initial build with // the header file missing. updateWithDiags( - S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) { + S, Source, Inputs, WantDiagnostics::Yes, + [&DiagCount](std::vector Diags) { + ++DiagCount; EXPECT_THAT(Diags, ElementsAre(Field(&Diag::Message, "'foo.h' file not found"), Field(&Diag::Message, @@ -751,18 +758,23 @@ // The addition of the missing header file shouldn't trigger a rebuild since // we don't track missing files. updateWithDiags( - S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) { + S, Source, Inputs, WantDiagnostics::Yes, + [&DiagCount](std::vector Diags) { + ++DiagCount; ADD_FAILURE() << "Did not expect diagnostics for missing header update"; }); // Forcing the reload should should cause a rebuild which no longer has any // errors. Inputs.ForceRebuild = true; - updateWithDiags( - S, Source, Inputs, WantDiagnostics::Yes, - [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); }); + updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes, + [&DiagCount](std::vector Diags) { + ++DiagCount; + EXPECT_THAT(Diags, IsEmpty()); + }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); + EXPECT_EQ(DiagCount, 2U); } TEST_F(TUSchedulerTests, NoChangeDiags) { TUScheduler S(CDB, optsForTest(), captureDiags()); @@ -853,15 +865,19 @@ TUState(PreambleAction::Idle, ASTAction::RunningAction), // We build the preamble TUState(PreambleAction::Building, ASTAction::RunningAction), - // Preamble worker goes idle + // We built the preamble, and issued ast built on ASTWorker + // thread. Preambleworker goes idle afterwards. + TUState(PreambleAction::Idle, ASTAction::RunningAction), + // Start task for building the ast, as a result of building + // preamble, on astworker thread. TUState(PreambleAction::Idle, ASTAction::RunningAction), - // We start building the ast + // AST build starts. TUState(PreambleAction::Idle, ASTAction::Building), - // Built finished succesffully + // AST built finished successfully TUState(PreambleAction::Idle, ASTAction::Building), - // Rnning go to def + // Running go to def TUState(PreambleAction::Idle, ASTAction::RunningAction), - // both workers go idle + // ASTWorker goes idle. TUState(PreambleAction::Idle, ASTAction::Idle))); } diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp --- a/clang-tools-extra/clangd/unittests/TestTU.cpp +++ b/clang-tools-extra/clangd/unittests/TestTU.cpp @@ -35,7 +35,7 @@ Files[ImportThunk] = ThunkContents; ParseInputs Inputs; - auto& Argv = Inputs.CompileCommand.CommandLine; + auto &Argv = Inputs.CompileCommand.CommandLine; Argv = {"clang"}; // FIXME: this shouldn't need to be conditional, but it breaks a // GoToDefinition test for some reason (getMacroArgExpandedLocation fails). @@ -71,8 +71,7 @@ auto CI = buildCompilerInvocation(Inputs, Diags); assert(CI && "Failed to build compilation invocation."); auto Preamble = - buildPreamble(testPath(Filename), *CI, - /*OldPreamble=*/nullptr, Inputs, + buildPreamble(testPath(Filename), *CI, Inputs, /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr); auto AST = buildAST(testPath(Filename), std::move(CI), Diags.take(), Inputs, Preamble); @@ -89,7 +88,7 @@ if (llvm::StringRef(Code).contains(Marker) || llvm::StringRef(HeaderCode).contains(Marker)) return true; - for (const auto& KV : this->AdditionalFiles) + for (const auto &KV : this->AdditionalFiles) if (llvm::StringRef(KV.second).contains(Marker)) return true; return false;