sammccall added a comment. 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? 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). 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). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76725/new/ https://reviews.llvm.org/D76725 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
