[PATCH] D54796: [clangd] C++ API for emitting file status

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/TUScheduler.cpp:268
+  /// Status of the TU.
+  TUStatus Status; /* GUARDED_BY(DiagMu) */
 };
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Is `Status` actually ever read from multiple threads?
> > > I assume it's only accessed on the worker thread, right? I think we can leave out the locks around it and put beside other fields only accessed by the worker thread, i.e. `DiagsWereReported`, etc.
> > > 
> > > PS Sorry go going back and forth, I didn't think about it in the previous review iteration.
> > Unfortunately not, most statuses are emitted via worker thread, but we emit `Queued` status in the main thread...
> Hm, that sounds surprising.
> What if the rebuild is in progress on the worker thread and we emit the queued status on the main thread at the same time? We might get weird sequences of statuses, right? 
> E.g. `Queued → BuildingPreamble → [new update on the main thread] Queued → [processing request on a worker thread] RunningAction('XRefs')`
> 
> The `Queued` request status between `BuildingPreamble` and `RunningAction` looks **really** weird.
> Maybe we could avoid that by setting emitting the `Queued` status on the worker thread whenever we're about to sleep on the debounce timeout?
This sounds fair enough, and can simplify the code.

> Maybe we could avoid that by setting emitting the Queued status on the worker thread whenever we're about to sleep on the debounce timeout?

The `Queued` state is originally designed for the state that the worker thread is blocked by the max-concurrent-threads semaphore.
Emitting it when we're about to sleep on the debounce timeout doesn't seem a right place to me. I think a reasonable place is before requiring the `Barrier`.



================
Comment at: clangd/TUScheduler.cpp:636
+      if (Requests.empty())
+        emitTUStatus({TUAction::Idle, /*Name*/ ""});
     }
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Maybe do it outside the lock? The less dependencies between the locks we have, the better and this one seems unnecessary.
> > Is it safe to read the `Requests` out of the lock here?
> No, but we could store the value of `.empty()` in a local var under the lock. The code looks a bit weirder, but that's a small price to pay for less locks being held at the same time.
SG.


================
Comment at: clangd/TUScheduler.h:60
+    BuildingPreamble, // The preamble of the TU is being built.
+    BuildingFile,     // The TU is being built.
+    Idle, // Indicates the worker thread is idle, and ready to run any upcoming
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > hokein wrote:
> > > > ilya-biryukov wrote:
> > > > > What's the fundamental difference between `BuildingFile` and `RunningAction`?
> > > > > We will often rebuild ASTs while running various actions that read the preamble.
> > > > > 
> > > > > Maybe we could squash those two together? One can view diagnostics as an action on the AST, similar to a direct LSP request like findReferences.
> > > > They are two different states, you can think `RunningAction` means the AST worker starts processing a task (for example `Update`, `GoToDefinition`) and `BuildingFile` means building the AST (which is one possible step when processing the task).  
> > > > 
> > > > In the current implementation, `BuildingPreamble` and `BuildingFile` are only emitted when AST worker processes the `Update` task (as we are mostly interested in TU states of `ASTWorker::update`); for other AST tasks, we just emit `RunningAction` which should be enough.
> > > > 
> > > > Given the following requests in the worker queue:
> > > > `[Update]`
> > > > `[GoToDef]`
> > > > `[Update]`
> > > > `[CodeComplete]`
> > > > 
> > > > statuses we emitted are
> > > > 
> > > > `RunningAction(Update) BuildingPreamble BuildingFile`
> > > > `RunningAction(GoToDef)`
> > > > `RunningAction(Update)  BuildingPreamble BuildingFile`
> > > > `RunningAction(GetCurrentPreamble)`
> > > > `Idle`
> > > > 
> > > That's the confusing part: clangd might be building the ASTs inside `RunningAction` too, but we only choose to report `BuildingFile` during updates.
> > > I would get rid of `BuildingFile` and change it to `RunningAction(Diagnostics)` instead, it feels like that would allow avoiding some confusion in the future.
> > > 
> > `BuildingFile` might be more natural (understandable) than `Diagnostics`. Two possible ways here:
> > 
> > - we can also report `BuildingFile` for other AST tasks, does it reduce the confusing part? 
> > - or if you think `Diagnostic` is an important state of `Update`, how about adding a new state `RunningDiagnostic`? `RunningAction` is a higher level information IMO, clangd is running diagnostic inside `RunningAction(Update).`
> > 
> > WDYT?
> If you look at the `TUScheduler` as a black box, diagnostics are not that much different from other actions reading the AST. 
> I'd err on the side of not sending updates unless something interesting is actually going on.
> What we're currently doing  makes sense (sending a single update per action), I think it doesn't really matter if we call it `BuildingFile` or `RunningAction("Diagnostics")` or `RunningDiagnostics`. Pick the one you like.
> 
> Let's put a comment on `BuildingFile` that it's only emitted when building the AST for diagnostics and the read actions emit `RunningAction` instead
> 
Done. added comment.


================
Comment at: clangd/TUScheduler.h:57
+  enum State {
+    Unknown,
+    Queued,           // The TU is pending in the thread task queue to be built.
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Maybe remove this value and leave out the default constructor too?
> > > That would ensure we never fill have `State` field with undefined value without having this enumerator.
> > > And FWIW, putting **any** value as the default seems fine even if we want to keep the default ctor, all the users have to set the State anyway. 
> > We need the default constructor... When ASTWorker is constructed, we don't know the any state, I'm nervous about putting **any value** as default value (it will hurt the code readability, future readers might be confused -- why the status is `Queued` when constructing).
> > 
> > Adding an `Unknown` seems ok to me, it is a common practice...
> `Idle` would be the valid state for an `ASTWorker` with an empty queue. Why not use it instead?
> 
> Can't disagree using `Unknown` is a common practice, but is it a good one? It forces every caller to handle this value, even though it's semantics are always unclear.
> Why not use `Optional<EnumWithoutUnkown>` if I really want to indicate the lack of value in those cases? Otherwise every `enum` type implements its own version of `Optional`.
`Idle` seems a compromising state for initialization.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54796