[clangd][NFC] Explode ReceivedPreamble into a CV

[kadircet]

Summary

Instead of a notification, we make use of a CV and store the boolean on
LatestPreamble by converting it into an optional.

Depends on D80293.

[sammccall]

clang-tools-extra/clangd/TUScheduler.cpp

664

Does LatestPreamble signal RequestsCV or just PreambleCV?

Seems like it might be less error-prone to have just one CV, signalled when preamble requests are scheduled, latest preamble becomes available, and on shutdown. The spurious wakeups shouldn't be a real problem, right?

[kadircet]

clang-tools-extra/clangd/TUScheduler.cpp

664

Does LatestPreamble signal RequestsCV or just PreambleCV?

it only signals PreambleCV. But it makes no sense for this code path to block on it, as it is signalled by the same thread. So in theory we should see LatestPreamble and not start blocking in such case.

Seems like it might be less error-prone to have just one CV, signalled when preamble requests are scheduled, latest preamble becomes available, and on shutdown. The spurious wakeups shouldn't be a real problem, right?

Yeah spurious wake-ups aren't really a problem. I thought having separate CVs sounded more clear, as predicates would still look the same even if we only had one CV. So it would enforce us to signal/wait on the right condition variable.

Happy to have only a single one too, do you have any suggestions for its name?

[sammccall]

clang-tools-extra/clangd/TUScheduler.cpp

664

Yeah, I think that having the CV not cover all the conditions, with correctness due to what threads the conditions are set on, is a bit too subtle.
I'd prefer a single CV named PreambleCV with an appropriate comment seems clear, WDYT?

[kadircet]

• Make distinction between PreambleCV and RequestsCV clearer.

clang-tools-extra/clangd/TUScheduler.cpp

 #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 <algorithm>
 #include <chrono>
+#include <condition_variable>
 #include <functional>
 #include <memory>
 #include <mutex>
 #include <queue>
 #include <string>
 #include <thread>
 #include <type_traits>
 #include <utility>
private:
   const bool RunSync;
   /// Time to wait after an update to see whether another update obsoletes it.
   const DebouncePolicy UpdateDebounce;
   /// File that ASTWorker is responsible for.
   const Path FileName;
   const GlobalCompilationDatabase &CDB;
   /// Callback invoked when preamble or main file AST is built.
   ParsingCallbacks &Callbacks;
-  /// Latest build preamble for current TU.
-  std::shared_ptr<const PreambleData> LatestPreamble;
-  Notification BuiltFirstPreamble;
 
   Semaphore &Barrier;
   /// Whether the 'onMainAST' callback ran for the current FileInputs.
   bool RanASTCallback = false;
   /// Guards members used by both TUScheduler and the worker thread.
   mutable std::mutex Mutex;
   /// File inputs, currently being used by the worker.
   /// Writes and reads from unknown threads are locked. Reads from the worker
   /// thread are not locked, as it's the only writer.
   ParseInputs FileInputs; /* GUARDED_BY(Mutex) */
   /// Times of recent AST rebuilds, used for UpdateDebounce computation.
   llvm::SmallVector<DebouncePolicy::clock::duration, 8>
       RebuildTimes; /* GUARDED_BY(Mutex) */
   /// Set to true to signal run() to finish processing.
   bool Done;                              /* GUARDED_BY(Mutex) */
   std::deque<Request> Requests;           /* GUARDED_BY(Mutex) */
-  std::queue<Request> PreambleRequests;   /* GUARDED_BY(Mutex) */
   llvm::Optional<Request> CurrentRequest; /* GUARDED_BY(Mutex) */
+  /// Signalled whenever a new request has been scheduled or processing of a
+  /// request has completed.
   mutable std::condition_variable RequestsCV;
-  Notification ReceivedPreamble;
+  /// Latest build preamble for current TU.
+  /// None means no builds yet, null means there was an error while building.
+  /// Only written by ASTWorker's thread.
+  llvm::Optional<std::shared_ptr<const PreambleData>> LatestPreamble;
+  std::queue<Request> PreambleRequests; /* GUARDED_BY(Mutex) */
+  /// Signaled whenever LatestPreamble changes state or there's a new
+  /// PreambleRequest.
+  mutable std::condition_variable PreambleCV;
   /// Guards the callback that publishes results of AST-related computations
   /// (diagnostics, highlightings) and file statuses.
   std::mutex PublishMu;
   // Used to prevent remove document + add document races that lead to
   // out-of-order callbacks for publishing results of onMainAST callback.
   //
   // The lifetime of the old/new ASTWorkers will overlap, but their handles
   // don't. When the old handle is destroyed, the old worker will stop reporting
if (!Invocation) {
                             [&](llvm::function_ref<void()> 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<std::mutex> Lock(PublishMu);
                               if (CanPublishResults)
                                 Publish();
                             });
+      // Note that this might throw away a stale preamble that might still be
+      // useful, but this is how we communicate a build error.
+      LatestPreamble.emplace();
       // Make sure anyone waiting for the preamble gets notified it could not be
       // built.
-      BuiltFirstPreamble.notify();
+      PreambleCV.notify_all();
       return;
     }
 
     PreamblePeer.update(std::move(Invocation), std::move(Inputs),
                         std::move(CompilerInvocationDiags), WantDiags);
-    // Block until first preamble is ready, as patching an empty preamble would
-    // imply rebuilding it from scratch.
-    // This isn't the natural place to block, rather where the preamble would be
-    // consumed. But that's too late, we'd be running on the worker thread with
-    // the PreambleTask scheduled and so we'd deadlock.
-    ReceivedPreamble.wait();
+    std::unique_lock<std::mutex> Lock(Mutex);
+    PreambleCV.wait(Lock, [this] {
+      // Block until we reiceve a preamble request, unless a preamble already
+      // exists, as patching an empty preamble would imply rebuilding it from
+      // scratch.
+      // We block here instead of the consumer to prevent any deadlocks. Since
+      // LatestPreamble is only populated by ASTWorker thread.
+      return LatestPreamble || !PreambleRequests.empty() || Done;
+    });
     return;
   };
   startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation);
 }
 
 void ASTWorker::runWithAST(
     llvm::StringRef Name,
     llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
void ASTWorker::updatePreamble(std::unique_ptr<CompilerInvocation> CI,
   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) {
+    if (!LatestPreamble || Preamble != *LatestPreamble) {
       ++PreambleBuildCount;
       // Cached AST is no longer valid.
       IdleASTs.take(this);
       RanASTCallback = false;
       std::lock_guard<std::mutex> Lock(Mutex);
       // LatestPreamble might be the last reference to old preamble, do not
       // trigger destructor while holding the lock.
-      std::swap(LatestPreamble, Preamble);
+      if (LatestPreamble)
+        std::swap(*LatestPreamble, Preamble);
+      else
+        LatestPreamble = std::move(Preamble);
     }
+    // Notify anyone waiting for a preamble.
+    PreambleCV.notify_all();
     // 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();
-    ReceivedPreamble.notify();
     return;
   }
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     PreambleRequests.push({std::move(Task), std::move(TaskName),
                            steady_clock::now(), Context::current().clone(),
                            llvm::None, TUScheduler::NoInvalidation, nullptr});
   }
-  ReceivedPreamble.notify();
+  PreambleCV.notify_all();
   RequestsCV.notify_all();
 }
 
 void ASTWorker::generateDiagnostics(
     std::unique_ptr<CompilerInvocation> Invocation, ParseInputs Inputs,
     std::vector<Diag> CIDiags) {
   // Tracks ast cache accesses for publishing diags.
   static constexpr trace::Metric ASTAccessForDiag(
       "ast_access_diag", trace::Metric::Counter, "result");
   assert(Invocation);
+  assert(LatestPreamble);
   // No need to rebuild the AST if we won't send the diagnostics.
   {
     std::lock_guard<std::mutex> Lock(PublishMu);
     if (!CanPublishResults)
       return;
   }
   // Used to check whether we can update AST cache.
   bool InputsAreLatest =
void ASTWorker::generateDiagnostics(
   // 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<std::unique_ptr<ParsedAST>> AST =
       IdleASTs.take(this, &ASTAccessForDiag);
   if (!AST || !InputsAreLatest) {
     auto RebuildStartTime = DebouncePolicy::clock::now();
     llvm::Optional<ParsedAST> NewAST = ParsedAST::build(
-        FileName, Inputs, std::move(Invocation), CIDiags, LatestPreamble);
+        FileName, Inputs, std::move(Invocation), CIDiags, *LatestPreamble);
     auto RebuildDuration = DebouncePolicy::clock::now() - RebuildStartTime;
     ++ASTBuildCount;
     // 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<std::mutex> Lock(Mutex, std::try_to_lock);
     if (Lock.owns_lock()) {
       // Do not let RebuildTimes grow beyond its small-size (i.e.
       // capacity).
if (InputsAreLatest) {
     RanASTCallback = *AST != nullptr;
     IdleASTs.put(this, std::move(*AST));
   }
 }
 
 std::shared_ptr<const PreambleData>
 ASTWorker::getPossiblyStalePreamble() const {
   std::lock_guard<std::mutex> Lock(Mutex);
-  return LatestPreamble;
+  return LatestPreamble ? *LatestPreamble : nullptr;
 }
 
-void ASTWorker::waitForFirstPreamble() const { BuiltFirstPreamble.wait(); }
+void ASTWorker::waitForFirstPreamble() const {
+  std::unique_lock<std::mutex> Lock(Mutex);
+  PreambleCV.wait(Lock, [this] { return LatestPreamble.hasValue() || Done; });
+}
 
 tooling::CompileCommand ASTWorker::getCurrentCompileCommand() const {
   std::unique_lock<std::mutex> Lock(Mutex);
   return FileInputs.CompileCommand;
 }
 
 TUScheduler::FileStats ASTWorker::stats() const {
   TUScheduler::FileStats Result;
void ASTWorker::stop() {
     std::lock_guard<std::mutex> Lock(PublishMu);
     CanPublishResults = false;
   }
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     assert(!Done && "stop() called twice");
     Done = true;
   }
-  // We are no longer going to build any preambles, let the waiters know that.
-  ReceivedPreamble.notify();
-  BuiltFirstPreamble.notify();
   PreamblePeer.stop();
+  // We are no longer going to build any preambles, let the waiters know that.
+  PreambleCV.notify_all();
   Status.stop();
   RequestsCV.notify_all();
 }
 
 void ASTWorker::startTask(llvm::StringRef Name,
                           llvm::unique_function<void()> Task,
                           llvm::Optional<WantDiagnostics> UpdateType,
                           TUScheduler::ASTActionInvalidation Invalidation) {