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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry about the delay on this, I said I'd prioritize it... had a slightly odd day.

Haven't reviewed the unit test yet (mostly a reminder to myself).
Generally looks good...



================
Comment at: clang-tools-extra/clangd/ClangdServer.h:153
 
+    /// Whether to run PreamblePeer asynchronously.
+    bool AsyncPreambleBuilds = false;
----------------
Group with StorePreamblesInMemory.

This comment doesn't make much sense either - PreamblePeer describe's the thread's relation to the main ASTWorker thread, but readers here won't know what it is. What about:
Reuse even stale preambles, and rebuild them in the background.
This improves latency at the cost of accuracy.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:244
+      // If NextReq was requested with WantDiagnostics::Yes we cannot just drop
+      // that on the floor. We block the request trying to override the update
+      // instead of the caller forcing diagnostics to don't block in cases where
----------------
First sentence makes sense, code makes sense, I can't parse the second sentence at all.
Just "block until it's built"?

Any maybe something about why we won't deadlock, but tests are enough I guess.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:292
     dlog("Preamble worker for {0} stopped", FileName);
+    BuiltFirst.notify();
   }
----------------
add comment // will never build one, or so


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:551
                   Semaphore &Barrier, DebouncePolicy UpdateDebounce,
-                  bool StorePreamblesInMemory, ParsingCallbacks &Callbacks) {
-  std::shared_ptr<ASTWorker> Worker(
-      new ASTWorker(FileName, CDB, IdleASTs, Barrier, /*RunSync=*/!Tasks,
-                    UpdateDebounce, StorePreamblesInMemory, Callbacks));
+                  bool StorePreamblesInMemory, bool AsyncPreambleBuilds,
+                  ParsingCallbacks &Callbacks) {
----------------
we're passing around enough loose booleans we should consider having TUScheduler hold its Options struct, and pass it here by refrerence...


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:660
                         std::move(CompilerInvocationDiags), WantDiags);
+    // Block until first preamble is ready. As patching an empty preamble would
+    // imply rebuilding it from scratch.
----------------
"ready, as patching..."


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


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1133
 
 bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
   std::unique_lock<std::mutex> Lock(Mutex);
----------------
I think this would be clearer as a loop... and maybe more correct.


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


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:195
+
+    /// Whether to run PreamblePeer asynchronously. No-op if AsyncThreadsCount
+    /// is 0.
----------------
nit: break before second sentence?


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:421
+opt<bool> AsyncPreambleBuilds{
+    "async-preamble-builds",
+    cat(Misc),
----------------
I'd drop "builds" at least from the flag name, possibly througout


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:423
+    cat(Misc),
+    desc("Enable async preamble builds."),
+    init(false),
----------------
This isn't very descriptive. I'd use some variation of the comment suggested for the ClangdServer::Options


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80293