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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
