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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1138
+  if (!wait(Lock, RequestsCV, Timeout,
+            [&] { return Requests.empty() && !CurrentRequest; }))
+    return false;
----------------
I'd consider pulling out an IsIdle lambda (checking for all 3 conditions) and using it both here and at the bottom.

PreambleRequests will never be (solely) full here, and Requests will always be empty at the bottom, but it's harmless and I think easier to read.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1142
+  // Now that ASTWorker processed all requests, ensure PreamblePeer has served
+  // all update requests.
+  if (!PreamblePeer.blockUntilIdle(Timeout))
----------------
Maybe explicitly: "This might create new PreambleRequests for the ASTWorker."


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1146
+  Lock.lock();
+  assert(Requests.empty() && "Received new requests during blockUntilIdle");
+  // Finally make sure ASTWorker has processed all of the preamble updates.
----------------
sammccall wrote:
> um... isn't that allowed?
> If new traffic means the queue doesn't go idle, I think we're supposed to return false, rather than crash.
OK, I somehowe forgot that ASTWorker isn't threadsafe so we only have the current thread and the worker thread to worry about.

Suggest a slightly expanded message like: No new normal tasks can be scheduled concurrently with blockUntilIdle(): ASTWorker isn't threadsafe


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:97
+  static std::unique_ptr<ParsingCallbacks>
+  capturePreamble(PreambleCallback CB) {
+    class CapturePreamble : public ParsingCallbacks {
----------------
Consider inlining this class until we have a second user.
It's not clear that adapting between lambda and an interface clarifies the test enough to justify the indirection.


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:97
+  static std::unique_ptr<ParsingCallbacks>
+  capturePreamble(PreambleCallback CB) {
+    class CapturePreamble : public ParsingCallbacks {
----------------
sammccall wrote:
> Consider inlining this class until we have a second user.
> It's not clear that adapting between lambda and an interface clarifies the test enough to justify the indirection.
capturePreamble isn't the right name for this function, unlike captureDiags it doesn't actually send the preamble anywhere.


================
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>) {
----------------
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).


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:999
+  TUScheduler S(CDB, optsForTest(), capturePreamble([&] {
+                  if (PreambleBuildCount > 0)
+                    Ready.wait();
----------------
This seems like a dubious use of atomic (lack of synchronization between accesses) - there's actually only one thread here, right?

It would be much clearer IMO to just test the passed-in `Version` directly.


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1020
+    ASSERT_TRUE(bool(AST));
+    EXPECT_THAT(AST->Inputs.Version, PI.Version);
+    RunASTAction.notify();
----------------
nit: spell out "blocking" explicitly


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1020
+    ASSERT_TRUE(bool(AST));
+    EXPECT_THAT(AST->Inputs.Version, PI.Version);
+    RunASTAction.notify();
----------------
sammccall wrote:
> nit: spell out "blocking" explicitly
if you expose preambleVersion() from ParsedAST, you could assert on it directly. I think this would be much clearer than the stuff with PreambleBuildCount.

(I don't see any reason not to expose preamble itself, but this probably isn't a good enough reason to do it)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80293