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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:155
 namespace {
+/// Responsible for building and providing access to the preamble of a TU. This
+/// worker doesn't guarantee that each preamble request will be built, in case
----------------
This doc comment doesn't mention "thread" once :-)


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:156
+/// Responsible for building and providing access to the preamble of a TU. This
+/// worker doesn't guarantee that each preamble request will be built, in case
+/// of multiple preamble requests, it will only build the last one. Once stop is
----------------
move documentation of requestBuild() to that function?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:157
+/// worker doesn't guarantee that each preamble request will be built, in case
+/// of multiple preamble requests, it will only build the last one. Once stop is
+/// called it will build any remaining request and will exit the run loop.
----------------
Move documentation of stop() semantics to stop()?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:159
+/// called it will build any remaining request and will exit the run loop.
+class PreambleWorker {
+public:
----------------
we may want to call this PreambleThread? Current name emphasizes peer relationship with ASTWorker, but current code has parent/child relationship.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:171
+
+  void requestBuild(CompilerInvocation *CI, ParseInputs PI) {
+    // If compiler invocation was broken, just fail out early.
----------------
or just update()?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:201
+  /// will unblock even after an unsuccessful build.
+  void waitForFirstPreamble() const { BuiltFirstPreamble.wait(); }
+
----------------
I think all the members in this class with "preamble" in the name can drop that word without any loss, e.g. `PreambleWorker::waitForFirst()`


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:205
+  /// built or latest attempt resulted in a failure.
+  std::shared_ptr<const PreambleData> getLatestBuiltPreamble() const {
+    std::lock_guard<std::mutex> Lock(Mutex);
----------------
latest()?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:218
+        PreambleRequested.wait(Lock, [this] { return PreambleReq || Done; });
+        // No request means we are done.
+        if (!PreambleReq)
----------------
Why do we rebuild the preamble if stop is requested?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:265
+  /// Updates the TUStatus and emits it. Only called in the worker thread.
+  void emitTUStatus(TUAction Action,
+                    const TUStatus::BuildDetails *Details = nullptr) {
----------------
as discussed a bit in chat, I think this part needs some more thought. Currently the ASTWorker and PreambleWorker emit data for the same file with some interleaving that's hard to reason about. The state needs an owner.

(e.g. consider a class that holds a TUStatus and exposes an Event<TUStatus>, with threadsafe setPreambleAction and setWorkerAction methods, or so)


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:291
+  /// valid for Req.
+  std::shared_ptr<const PreambleData> buildPreamble(Request Req) {
+    assert(Req.CI && "Got preamble request with null compiler invocation");
----------------
just build(), to avoid confusion clangd::buildPreamble?

Also consider merging with updateLatestBuiltPreamble, both are small and private and they always go together.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:295
+    std::shared_ptr<const PreambleData> OldPreamble =
+        Inputs.ForceRebuild ? std::shared_ptr<const PreambleData>()
+                            : getLatestBuiltPreamble();
----------------
(nullptr should work here?)


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:312
+
+  mutable std::mutex Mutex;
+  bool Done = false;
----------------
Yikes, lots of state :-)
I'd consider merging the two CVs - I find keeping the events separate and reasoning when you need notify_one vs _all doesn't seem to pay off, may just be the way I think about queues. Up to you.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:322
+  mutable std::condition_variable PreambleBuilt;
+  std::shared_ptr<const PreambleData> LastBuiltPreamble;
+
----------------
nit: last vs latest, be consistent


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:325
+  Notification BuiltFirstPreamble;
+  const PathRef FileName;
+  ParsingCallbacks &Callbacks;
----------------
please make this owning for simplicity, this isn't a lightweight object anyway


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:600
         Inputs, CompilerInvocationDiagConsumer, &CC1Args);
+    auto OldPreamble = PW.getLatestBuiltPreamble();
+    PW.requestBuild(Invocation.get(), Inputs);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76125