[PATCH] D42573: [clangd] The new threading implementation

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Really coming together!



================
Comment at: clangd/ASTWorker.cpp:1
+//===--- ASTWorker.cpp --------------------------------------------*-C++-*-===//
+//
----------------
This file could really use some high-level comments about the scheduling strategy, throughout


================
Comment at: clangd/ASTWorker.cpp:20
+      StartedRunning(false), Done(false) {
+  if (RunSync)
+    return;
----------------
?!


================
Comment at: clangd/ASTWorker.cpp:102
+      // not waste time on it.
+      LastUpdateCF->cancel();
+    }
----------------
This strategy has some upsides:
 - we eventually get diagnostics for the latest version
 - we don't begin building an AST that we know can never be read
 - we don't "wait" when there's something useful to do
 - it's simple to implement (apart from cancellation)
And downsides:
 - diagnostics are built on the first keystroke
 - indexing blocks interactive actions, because we don't (and can't reasonably) respect cancellation before calling OnUpdated
 - it requires CancellationFlag even though our actions aren't cancelable

What's the goal here - is this a strategy to keep? Or a placeholder? Or trying to maximize compatibility with the previous code?


================
Comment at: clangd/ASTWorker.h:1
+//===--- ASTWorker.h ----------------------------------------------*-C++-*-===//
+//
----------------
ASTWorker is an implementation detail of TUScheduler. Can we move it to TUScheduler.cpp, instead of exposing it, and remove this header?


================
Comment at: clangd/ASTWorker.h:19
+namespace clangd {
+struct InputsAndAST {
+  const ParseInputs &Inputs;
----------------
This seems like an "implementation header" - nobody should depend on ASTWorker AIUI.

So InputsAndAST shouldn't really go here, as it's part of the TUScheduler public interface.


================
Comment at: clangd/ASTWorker.h:24
+
+struct InputsAndPreamble {
+  const ParseInputs &Inputs;
----------------
(InputsAndPreamble is entirely unused here, I think)


================
Comment at: clangd/ASTWorker.h:29
+
+/// Owns one instance of the AST, schedules updated and reads of it.
+/// Also responsible for building and providing access to the preamble.
----------------
updated -> updates


================
Comment at: clangd/ASTWorker.h:38
+
+  // Must be called exactly once on processing thread. Will return after
+  // setDone() is called on a separate thread and all pending requests are
----------------
As discussed offline, this lifecycle is a bit complicated :-)
ASTWorker basically manages a thread, and it'd be nice if we could align the object lifetime and thread lifetime more closely.

The difficulty seems to be that we want to TUScheduler to be able to discard ASTWorkers instantly, even though we can't interrupt them.
After offline brainstorming, we came up with some sort of "ASTWorkerHandle" which:
 - owns the thread that run() is called on
 - detaches that thread on destruction
 - has a weak_ptr or shared_ptr to the worker itself, which it exposes

We still have run() and setDone(), but they're private details.


================
Comment at: clangd/ASTWorker.h:43
+  /// Signal that run() should finish processing pending requests and exit.
+  void setDone();
+
----------------
stop()? (or stopSoon/requestStop to be more explicit)


================
Comment at: clangd/ASTWorker.h:45
+
+  std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
+  std::size_t getUsedBytes() const;
----------------
can we reorder/group these by purpose/sequence?
e.g (no need for the comments, just illustrative).

  //lifecycle
  run();
  setDone();

  //write
  update()

  //read
  runWithAST()
  getPossiblyStalePreamble()

  //misc
  getUsedBytes()


================
Comment at: clangd/ASTWorker.h:54
+private:
+  using RequestWithCtx = std::pair<UniqueFunction<void()>, Context>;
+
----------------
I think this might actually be easier to read with just  `using Request = UniqueFunction<void()>` and then spelling out `pair<Request, Context>`. up to you though.



================
Comment at: clangd/ASTWorker.h:59
+  // File and FileInputs are only accessed on the processing thread from run().
+  // FIXME(ibiryukov): group CppFile and FileInputs into a separate class.
+  const std::shared_ptr<CppFile> File;
----------------
nit: "combine ... into one class"?

I'd hope we won't end up with ASTWorker, CppFile, FileInputs, *and* another thing?


================
Comment at: clangd/ASTWorker.h:69
+  std::queue<RequestWithCtx> Requests; /* GUARDED_BY(Mutex) */
+  // Only set when last request is an update.
+  llvm::Optional<CancellationFlag> LastUpdateCF; /* GUARDED_BY(Mutex) */
----------------
Why?
e.g. `This allows us to cancel an update that was never read, if a subsequent update comes in`


================
Comment at: clangd/TUScheduler.cpp:38
+  // Wait for all in-flight tasks to finish.
+  Tasks.waitForAll();
+}
----------------
destructor will do this


================
Comment at: clangd/TUScheduler.h:77
+  /// In destructor, FileData signals to ASTWorker for file that it can exit.
+  struct FileData {
+    FileData(ParseInputs Inputs, std::shared_ptr<ASTWorker> Worker);
----------------
This would be superseded by ASTWorkerHandle, right?


================
Comment at: clangd/TUScheduler.h:90
+
+  struct AuxData {
+    bool StorePreamblesInMemory;
----------------
inline this? or come find a more descriptive name :-)


================
Comment at: clangd/Threading.h:62
+/// Allows to run tasks on separate (detached) threads and wait for all tasks to
+/// finish.
+class AsyncTaskRunner {
----------------
nit: "allows to run tasks" --> "Runs tasks"

A little more motivation would be useful here: this describes what is done, but not really why.


e.g. `Objects that need to spawn threads can own an AsyncTaskRunner to ensure they all complete on destruction.`


================
Comment at: clangd/Threading.h:65
+public:
+  ~AsyncTaskRunner();
+
----------------
comment


================
Comment at: clangd/Threading.h:67
+
+  void waitForAll();
+  void runAsync(UniqueFunction<void()> Action);
----------------
any need to expose this separately from the destructor?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42573