[PATCH] D80293: [clangd] Run PreambleThread in async mode behind a flag

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:724
                            std::move(Req.WantDiags));
+    // Set it after notifying ASTPeer about the preamble to prevent any races.
+    BuiltFirst.notify();
----------------
kadircet wrote:
> sammccall wrote:
> > hmm, we're notifying the ASTPeer, and then setting builtfirst which is being waited on... by astpeer.
> > This suggests to me the AST thread should own the notification, not the preamble thread.
> > And in fact it already has a notification for the first preamble being built! why can't we use that one?
> umm, it is a little bit more complicated.
> 
> The above call only inserts the "update request" onto ASTPeer's queue and doesn't update the preamble inside ASTPeer. This enables ASTPeer to access `LatestPreamble` without holding the lock, as it is the only writer. We can change that, update the `LatestPreamble` while queueing the request.
As discussed, the relationship between the two is complicated.
At least we should move the second into ASTPeer to reduce the surface area between the two.

As a followup, we can "explode" the existing notification into CV + Mutex + bool.
The bool can naturally become the "present" bit on LatestPreamble if it turns into an Optional<shared_ptr<>> - we must document none vs null!
The Mutex already exists.

Then we don't need the second notification: we just wait on `!preambletasks.empty() || LatestPreamble.hasValue()`




================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:658
                         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.
This needs a comment :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80293/new/

https://reviews.llvm.org/D80293