[PATCH] D76125: [clangd] Decouple preambleworker from astworker, NFCI

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This basically looks good, just nits (mostly about comments, sorry I can't help myself).

I think the only real outstanding thing is TUStatus, which we're still discussing offline. Since this patch is intended to be NFC, I see two paths consistent with the spirit of that:

- work out how to make this emit the same sequence of events (probably in a fairly brittle way), document that, and sort it out in the next patch
- make TUStatus reflect the new model, without changing its essential design or scope. Currently TUStatus is currently (stateful bits about TU, worker thread activity), so the extension would be (stateful bits about TU, worker thread activity, preamble thread activity).



================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:218
+        PreambleRequested.wait(Lock, [this] { return PreambleReq || Done; });
+        // No request means we are done.
+        if (!PreambleReq)
----------------
kadircet wrote:
> sammccall wrote:
> > Why do we rebuild the preamble if stop is requested?
> it was to be consistent with astworker, but i suppose it isn't necessary as ASTWorker does that to make sure each LSP request gets a reply, and it can be done with stale preambles.
> 
> Are there any other reasons for ASTWorker to empty the queue after stop?
Yeah, I think that's the only reason and the asymmetry is worthwhile here (esp because one preamble is way more expensive than one AST).

If that's not clear in the ASTworker piece, please do add a comment!


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:600
         Inputs, CompilerInvocationDiagConsumer, &CC1Args);
+    auto OldPreamble = PW.getLatestBuiltPreamble();
+    PW.requestBuild(Invocation.get(), Inputs);
----------------
kadircet wrote:
> sammccall wrote:
> > this doesn't seem correct (maybe ok in this patch because of the blocking, but not in general). You're assuming the last available preamble is the one that the last AST was built with.
> > 
> > I suppose you can't check the preamble of the current ParsedAST because it might not be cached, and you nevertheless want to skip rebuild if the diagnostics are going to be the same. I can't think of anything better than continuing to hold the shared_ptr for PreambleForLastBuiltAST or something like that.
> right, this is just a "hack" to keep this change NFC.
> 
> in the follow-up patches i am planning to signal whether latest built preamble is reusable for a given `ParseInputs`, and also signal what the AST should be patched with.
> 
> diagnostics(ast) will only be built if preamble is re-usable.
> right, this is just a "hack" to keep this change NFC.

can you add a comment about this? (In particular, why it's correct?)

> in the follow-up patches i am planning to signal whether latest built preamble is reusable for a given ParseInputs

Does "reusable" mean "completely valid" (current semantics), or usable with some tweaks (e.g. added headers)? It would be good to define some precise terminology around this.

> diagnostics(ast) will only be built if preamble is re-usable.

SG


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:156
+/// Responsible for building and providing access to the preamble of a TU.
+/// Processes async build requests on a dedicated thread executing run method.
+/// Unless RunSync is provided, which will build preambles on the requesting
----------------
This sentence is hard to parse (processes, build, requests, and run are all ambiguous between nouns and verbs, and the pronouns, articles etc are missing).

Suggestion: "It processes updates asynchronously, building preambles on a dedicated thread. (This thread should be spawned externally and call the run() method)."


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:156
+/// Responsible for building and providing access to the preamble of a TU.
+/// Processes async build requests on a dedicated thread executing run method.
+/// Unless RunSync is provided, which will build preambles on the requesting
----------------
sammccall wrote:
> This sentence is hard to parse (processes, build, requests, and run are all ambiguous between nouns and verbs, and the pronouns, articles etc are missing).
> 
> Suggestion: "It processes updates asynchronously, building preambles on a dedicated thread. (This thread should be spawned externally and call the run() method)."
Actually, is "processing requests" really the clearest way to describe the model here?

What about "Whenever the thread is idle and the preamble is outdated, it starts to build a fresh preamble from the latest inputs."


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:157
+/// Processes async build requests on a dedicated thread executing run method.
+/// Unless RunSync is provided, which will build preambles on the requesting
+/// thread instead.
----------------
nit: "which" appears to refer to `RunSync` here, and "provided" doesn't exactly mean "true".

Suggestion: If RunSync is true, preambles are built synchronously in update() instead.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:171
+
+  /// It isn't guaranteed that each request will be built. If there are multiple
+  /// update requests while building a preamble, only the last one will be
----------------
request -> requested version


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:224
+        CurrentReq = std::move(*NextReq);
+        // Reset it here, before we build the request to not overwrite any new
+        // request that might arrive while we are still building this one.
----------------
I think the comment adds more confusion than it resolves: you seem to be describing why an alternative (reacquiring the lock after building, and then calling reset?) doesn't work, but never state what that alternative is.

Moreover, clearing NextReq after moving the value into CurrentReq seems natural, I'm not sure a comment is necessary (except maybe to complain that std::optional's move constructor doesn't do this!)


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:243
+    }
+    // Let the worker thread know we've been stopped.
+    ReqCV.notify_all();
----------------
we've been stopped -> it should stop?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:300
+      std::lock_guard<std::mutex> Lock(Mutex);
+      LatestBuild = std::move(Preamble);
+      CurrentReq.reset();
----------------
If LatestBuild is the last shared_ptr to the old preamble, we're destroying it here with the lock held, which probably does IO.

Instead, std::swap(Preamble, LatestBuild) so that the destructor runs when the function exits.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:310
+  llvm::Optional<Request> NextReq;
+  // Written by only worker thread, might be read by multiple threads through
+  // BlockUntilIdle.
----------------
(not sure the "written by" comment is needed since this is just a regular guarded-by-mutex?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76125