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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ASTWorker.cpp:102
+      // not waste time on it.
+      LastUpdateCF->cancel();
+    }
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > 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
> This sounds fine - can you add a little bit of this rationale and things we might want to change? Maybe at the end of the header comment?
> 
> As it stands, it would be very easy for us to land this, move onto other things for a month, and lose the reasoning why the strategy is this way.
Added a section about cancellation to the comments in the `.cpp` file.



================
Comment at: clangd/TUScheduler.cpp:13
+// ASTWorker that manages the AST is owned both by the processing thread and the
+// TUScheduler. Therefore the destructor of ASTWorker to not block, as it is
+// always run after the processing loop is finished. When the TUScheduler gives
----------------
sammccall wrote:
> I can't really follow these two sentences.  I think we may want to give a little motivation (it's not clear why the destructor needs to not block) before introducing ASTWorkerHandle. e.g.
> 
> // The TUScheduler should discard an ASTWorker when remove() is called, but its thread may 
> // be busy and we don't want to block. So the workers are accessed via an ASTWorkerHandle.
> // Destroying the handle signals the worker to exit its run loop, gives up shared ownership of the worker, and detaches the thread.
Added your comment.


================
Comment at: clangd/TUScheduler.cpp:82
+  const std::shared_ptr<CppFile> File;
+  // Inputs, corresponding to the current state of File.
+  ParseInputs FileInputs;
----------------
sammccall wrote:
> ah, this is my confusion - i mixed up "current state of File" with "current state of the file", which are almost opposites!
> Do you think it's confusing to call the `File` member `AST`? All the callsites actually seem to make sense with that name.
SG, renamed.


================
Comment at: clangd/TUScheduler.cpp:186
+    auto AST = File->getAST().get();
+    AST->runUnderLock([&](ParsedAST *AST) {
+      if (!AST) {
----------------
sammccall wrote:
> why under lock? I thought this is the only thread that can access?
> 
> (if this locking is a no-op pending cleanup, fixme)
That's the only way to access the AST with the current `CppFile` interface, I'll do a cleanup with the follow-up patch.
Added a comment


================
Comment at: clangd/TUScheduler.cpp:217
+std::size_t ASTWorker::getUsedBytes() const {
+  std::lock_guard<std::mutex> Lock(Mutex);
+  return File->getUsedBytes();
----------------
sammccall wrote:
> I don't think this works - other accesses of File aren't guarded by the mutex?
> (And File is documented as only being used by the worker thread)
> 
> One option is to cache this in a member after each rebuild(), and put that member behind the mutex. It won't catch ASTs that grow over time, though :-(
> A fundamental question is what this function should do if called while the AST is being rebuilt on the worker thread.
It work because `CppFile` is thread-safe at the moment. We don't even need the lock.
I'll do the associated cleanup with the follow-up patch.

> One option is to cache this in a member after each rebuild(), and put that member behind the mutex. It won't catch ASTs that grow over time, though :-(
That was the plan for the cleanup of `CppFile`.
We could update this member after each read, too. This would give us a good estimate after AST grows.

>A fundamental question is what this function should do if called while the AST is being rebuilt on the worker thread.
We only report estimated memory usage and it's only accurate when there are no active requests.
So the answer is: we report memory used by the previous AST while doing the rebuild and memory used by the new AST after rebuild is finished.
This seems good enough, we don't need the perfect estimates AFAIK.


================
Comment at: clangd/TUScheduler.cpp:285
+struct TUScheduler::FileData {
+  FileData(ParseInputs Inputs, ASTWorkerHandle Worker)
+      : Inputs(std::move(Inputs)), Worker(std::move(Worker)) {}
----------------
sammccall wrote:
> any need for this constructor?
It allowed us to use `llvm::make_unique`.
Remove it and replaced with `unique_ptr<FileData>(new FileData{})`.


================
Comment at: clangd/TUScheduler.cpp:288
+
+  ParseInputs Inputs;
+  ASTWorkerHandle Worker;
----------------
sammccall wrote:
> This could use a comment and/or a more descriptive variable name to distinguish it from Worker.FileInputs.
Added a comment. Worker.FileInputs are not exposed in the public API (for that specific reason), so hopefully shouldn't cause any confusion


================
Comment at: clangd/TUScheduler.cpp:312
         OnUpdated) {
-  CachedInputs[File] = Inputs;
-
-  auto Resources = Files.getOrCreateFile(File);
-  auto DeferredRebuild = Resources->deferRebuild(std::move(Inputs));
+  auto It = Files.find(File);
+  if (It == Files.end()) {
----------------
sammccall wrote:
> Maybe clearer than STL it->second stuff:
> 
>   FileData &FD = Files[File]
>   if (!FD.Worker)
>     FD.Worker = Create(...);
>   FD.Inputs = move(Inputs)
Thanks. Looks much better.


================
Comment at: clangd/TUScheduler.cpp:365
 
-  const ParseInputs &Inputs = getInputs(File);
+  ParseInputs InputsCopy = It->second->Inputs;
   std::shared_ptr<const PreambleData> Preamble =
----------------
sammccall wrote:
> why do we want to make this copy rather than *only* getting the preamble within the task?
> Is it to do with preferring preambles from the past, but accepting those from the future?
> If I'm understanding right, the general case is these versions:
> 
>   - V1: this preamble has finished building before `runWithPreamble` is called
>   - V2: this update() came before `runWithPreamble`, but wasn't built yet. It finished building before the read task ran.
>   - V3: this update() came before `runWithPreamble`, but wasn't built by the time the read task ran.
>   - V4: this update() came after `runWithPreamble` was called, but finished before the read task ran (semaphore race?)
>   - V5: this came after `runWithPreamble` and finished after the read task
>  
> Ideally I guess we'd prefer V2, falling back to V1 -> V4 -> null in that order.
> Just calling `getPossiblyStalePreamble()` in the task would return V4 -> V2 -> V1 -> null.
> The current code will return V1 -> V4 -> V2.
> 
> But we have to get really unlucky for V4 to exist, whereas V2 is quite likely to exist I think. This means that only calling `getPossiblyStalePreamble()` once, in the task, seems more likely to get this right to me.
Good point.
The original intention was to pass both preambles to the actions and make them choose which one matches for their needs.
Given that we now cheat and use preambles anyway without checking if they match, not storing the copy seems the way to go.


================
Comment at: clangd/TUScheduler.cpp:38
+  // Wait for all in-flight tasks to finish.
+  Tasks.waitForAll();
+}
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > 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.
> Hmm, not sure it's much safer than putting a comment on a member, but up to you.
> 
> If we're not using the destructor-blocking behavior, remove the destructor or replace with an assert?
I had trouble figuring out what went wrong more than once when hitting similar errors.
The worst part is that it usually pops up when editing the code (and missing the comment), not in the initial patch.
I'd leave as is.

I also think that an extra `waitForAll()` in destructor doesn't hurt and makes the interface of the class saner, i.e. makes it easier to use such a class as a local variable.
There is a new test that checks the destructor waits, too. Hope that's alright.


================
Comment at: clangd/Threading.h:67
+
+  void waitForAll();
+  void runAsync(UniqueFunction<void()> Action);
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > 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`.
> Yep, the testing use case makes sense. Let's replace the dtor with an assert though, if we don't think owning this object is a safe way to guard.
I don't think owning the object is a safe way to guard, but waiting in destructor still seems like a nicer interface than assert (see the other comment).
Added a test for that.


================
Comment at: clangd/Threading.h:27
+/// Once cancelled, cannot be returned to the previous state.
+/// FIXME: We should split this class it into consumers and producers of the
+/// cancellation flags.
----------------
sammccall wrote:
> hmm, I'm not sure there's enough value in this FIXME to make it ever worth doing. I think it will live forever and we should just drop it :-)
Oh, I think there is if we ever want to use it as a more general cancellation mechanism.
Splitting the responsibilities of "checking for cancellation" and "cancelling the request" seems like a very useful thing to do.

Removed the FIXME, we can discuss that in more detail if CancellationFlag gets more widely used.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42573