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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 22 inline comments as done.
kadircet added inline comments.


================
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();
----------------
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.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1133
 
 bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
   std::unique_lock<std::mutex> Lock(Mutex);
----------------
sammccall wrote:
> I think this would be clearer as a loop... and maybe more correct.
as discussed offline, loop version isn't more clear keeping the straight version.


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:466
           updateWithDiags(
-              S, File, Inputs, WantDiagnostics::Auto,
+              S, File, Inputs, WantDiagnostics::Yes,
               [File, Nonce, &Mut, &TotalUpdates](std::vector<Diag>) {
----------------
sammccall wrote:
> As you've pointed out, Yes isn't used in production. So if the behaviour of Auto has changed we should test that rather than stop using it.
> 
> My understanding of the change:
> 1. we've asserted that every update yields diagnostics
> 2. this was true because we were calling runWithAST, this forced an AST build at each snapshot, and we'd publish the diagnostics
> 3. now we only publish diagnostics once the version has been through the preamble thread and back
> 4. these requests get coalesced in the preamble thread if the AST worker is pushing them faster than the preamble worker is servicing them
> 5. so we no longer publish diagnostics for every version even if we have a suitable AST
> 
> Is this right?
> If so, I'd assert that TotalUpdates is at least FilesCount and at most FilesCount * UpdatesPerFile, and also that the latest UpdateI for diags (for any file) is equal to UpdatesPerFile - 1.
> And maybe add a brief explanation (we drop diagnostics for some snapshots as they get coalesced in the preamble worker).
> Is this right?

yes. in addition to those, previously `Auto` promised a diagnostics release if it was followed by a read(that's how this test used to work). we no longer guarantee that for the reasons you've mentioned.


What about keeping `WantDiagnostics::Yes` at all? I believe it should be a separate patch none the less, but I would be happy with silently converting Yes to Auto. I would expect this to only break editors not using `Auto` at all and always forcing diags or not requesting them at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80293