[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

Wed Mar 25 11:53:39 PDT 2020

kadircet added a comment.

In D76725#1940282 <https://reviews.llvm.org/D76725#1940282>, @sammccall wrote:

> So I think we need a careful description of the new model somewhere. Not so much on specific changes/constraints parts of the code operate under, but what it's trying to do.
>
> My best understanding is:
>
> - In general, read requests are processed in order by the ASTWorker, and get exactly the version of the file when the request was received. That is, the ASTworker queue interleaves updates and reads in the same order as LSP.
> - Preambles are built on the PreambleWorker for a certain version of the file, and may be compatible or incompatible with subsequent versions. If a read is "picky" it may block the ASTWorker queue waiting for a compatible preamble, otherwise it will "patch up" an incompatible one.
> - Diagnostics and indexes are updated opportunistically using onMainAST when an AST is built, but only if the used preamble is compatible. Publishing diagnostics from patched preambles is not allowed, but we also do not block waiting for an up-to-date preamble in order to generate publishable diagnostics. (Exception: if WantDiagnostics=Yes, we block).
> - To ensure forward progress with diagnostics if the queue is full, when preamble is built we immediately build a "golden" AST from that version and publish diagnostics. To ensure diagnostics do not "go backwards", opportunistic diagnostics are suppressed if they don't use the latest preamble (the one with the last golden AST). This means the sequence of ASTs producing diagnostics is neatly partitioned into "epochs" of the preambles, the first in the epoch is always the golden version. (Exception: if WantDiagnostics=no, diagnostics are not emitted for the golden AST). Because these versions are built out-of-order (not interleaved with reads per LSP), the golden AST is not cached and reused for reads.
>
>   Is this about right?

Yes that's most of what this patch does. Tried to explain/sum up those in file comments.

> Based on this, I do think the model would be much easier to reason about and verify if the golden AST was built on the ASTWorker thread in a queued task, rather than on the PreambleWorker thread in a callback. This means we understand allowed sequencing by looking at the queue rather than reasoning about mutexes/threads with multiple possible interleavings. By pushing the golden AST task to the front of the queue, it's more explicit that this is a priorities/scheduling decision. Following what happened in logs is probably easier due to less interleaving.
> 
>   (The callback could enqueue the task, or the PreambleWorker could just enqueue the task directly at that point - not sure the callback indirection buys anything).

Done. It is still the callback that enqueues the task, but moved storage of latest build preamble from PreambleThread to ASTWorker to prevent a possible race between currently running update in astworker and build of preamble in preamblethread, as discussed offline.
This also enabled us to cache golden ASTs in case ASTWorker hasn't received any updates in between which gets rid of the additional AST build cost.

> One thing that seems complicated in the model is that the AST build needs to decide whether to block on a compatible preamble, but it's the following read that determines whether it needs to. In the worst case, the picky read hasn't even been scheduled yet. I think picky reads probably need to block on the target preamble and build their AST themselves if the cached one isn't suitable. If they "miss" their preamble (i.e. preambleVersion >= readVersion but the preamble isn't compatible) then I think we should fail the request (i.e. picky requests may be invalidated by subsequent preamble edits).

Agreed, but this is not in the scope of this patch, will address that in the upcoming patches after dropping the synchronization between preamble and ast thread. In addition to that i am planning to ensure updates with `WantDiagnostics::Yes` gets build by making them non-overwritable in PreambleThread. That is any subsequent build requests will block (astworker thread) until update with `WantDiags::Yes` starts building.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76725