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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clangd/ASTWorker.cpp:1
+//===--- ASTWorker.cpp --------------------------------------------*-C++-*-===//
+//
----------------
sammccall wrote:
> This file could really use some high-level comments about the scheduling strategy, throughout
I've added the docs to TUScheduler.cpp


================
Comment at: clangd/ASTWorker.cpp:102
+      // not waste time on it.
+      LastUpdateCF->cancel();
+    }
----------------
sammccall wrote:
> 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?
Trying to maximize the compatibility with existing code. There are certainly other strategies to schedule updates.
I'm perfectly happy to explore those, but would try to keep it out of this patch, it's touching quite a lot of things already. And we should probably remove the callback from update in order to make implementing the timeouts simpler


================
Comment at: clangd/ASTWorker.h:45
+
+  std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
+  std::size_t getUsedBytes() const;
----------------
sammccall wrote:
> 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()
I used a different grouping:
```
//lifecycle
run();
setDone();

//async write
update()
// async read
runWithAST()

// sync reads
getPossiblyStalePreamble()
getUsedBytes()
```
Does that make sense? Or do you feel read/write vs sync/async is a better distinction?


================
Comment at: clangd/ASTWorker.h:54
+private:
+  using RequestWithCtx = std::pair<UniqueFunction<void()>, Context>;
+
----------------
sammccall wrote:
> 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.
> 
I'd rather keep it as is. I specifically came up with this typedef because I got annoyed of typing `pair<Request, Context>`.
Happy to change it if you feel that's totally unreadable, of course.


================
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;
----------------
sammccall wrote:
> nit: "combine ... into one class"?
> 
> I'd hope we won't end up with ASTWorker, CppFile, FileInputs, *and* another thing?
Removed the FIXME


================
Comment at: clangd/TUScheduler.cpp:38
+  // Wait for all in-flight tasks to finish.
+  Tasks.waitForAll();
+}
----------------
sammccall wrote:
> destructor will do this
It's much safer to call it explicitly to not depend no the order of fields in the class.


================
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);
----------------
sammccall wrote:
> This would be superseded by ASTWorkerHandle, right?
It's still there, stores inputs and `ASTWorkerHandle`.

Inputs in the `ASTWorker` correspond to the latest **processed** update, we don't expose them in the API. 
These inputs correspond to the latest inputs of the file (they are used by `runWithPreamble` to provide proper inputs).


================
Comment at: clangd/Threading.h:67
+
+  void waitForAll();
+  void runAsync(UniqueFunction<void()> Action);
----------------
sammccall wrote:
> any need to expose this separately from the destructor?
It's a useful helper to have.
We can use it to rewrite tests when removing `future` returned from `addDocument`/`removeDocument`.
Also see my other comment about calling it in destructor of `TUScheduler`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42573