sammccall marked 2 inline comments as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:457
+ // Give up on this request, releasing resources if any.
+ void abandon() {
+ std::lock_guard<std::mutex> Lock(Mu);
----------------
kadircet wrote:
> in the scenario of a shutdown (or destruction of a preamble thread, because
> file is closed), i think we also want to call cancellation of the acquire
> release here. as things stand unless we're destroying the throttler itself i
> think these requests might stick around.
(this is obsolete, but...)
I don't think this leak is possible, after we get Cancel (from
Throttler->acquire) then we wait until either:
- request is satisfied (TState->ready() on line 500) in which case no cancel
is needed, or
- we call Cancel immediately on line 501.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:483
// Wait until stop is called or there is a request.
- ReqCV.wait(Lock, [this] { return NextReq || Done; });
+ ReqCV.wait(Lock, [&] { return NextReq || Done; });
if (Done)
----------------
kadircet wrote:
> any reason for capturing locals here now ?
Just for consistency with the second `wait` below - I usually don't think it's
interesting to list captures when no lifetimes are involved (because the lambda
is invoked synchronously)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:500
+
+ ReqCV.wait(Lock, [&] { return TState->ready() || Done; });
+ if (Done) {
----------------
kadircet wrote:
> kadircet wrote:
> > we can move the wait into the `if block` above, i think.
> so this implies preamblerequest we issued an acquire for, and the preamble
> request we'll be building below might be different. this is not a concern
> initially but i think it would be nice to not get cornered when we want to
> notify the throttler about such changes.
>
> i guess `PreambleThread::update` would be the natural place to communicate
> such changes, if we're overwriting a preamblerequest and there's a pending
> acquire we can either cancel and make preamblethread re-issue acquire, or we
> can have a new API on throttler to update the request signals. i think, both
> of these require having throttlestate as a member. maybe we should do that
> directly to remind ourselves of this in the future?
> we can move the wait into the if block
that works, but it doesn't really seem clearer to me
`wait (..., cond)` is just `while (!cond) { ... }`, so the outer if is
redundant.
On the other hand the if is important to the code it currently guards (for
reasons explained in the comment), so it seems confusing to split its purpose
in this way.
The only way it makes sense to me is as a microoptimization, but it doesn't
seem like a useful one.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:500
+
+ ReqCV.wait(Lock, [&] { return TState->ready() || Done; });
+ if (Done) {
----------------
sammccall wrote:
> kadircet wrote:
> > kadircet wrote:
> > > we can move the wait into the `if block` above, i think.
> > so this implies preamblerequest we issued an acquire for, and the preamble
> > request we'll be building below might be different. this is not a concern
> > initially but i think it would be nice to not get cornered when we want to
> > notify the throttler about such changes.
> >
> > i guess `PreambleThread::update` would be the natural place to communicate
> > such changes, if we're overwriting a preamblerequest and there's a pending
> > acquire we can either cancel and make preamblethread re-issue acquire, or
> > we can have a new API on throttler to update the request signals. i think,
> > both of these require having throttlestate as a member. maybe we should do
> > that directly to remind ourselves of this in the future?
> > we can move the wait into the if block
>
> that works, but it doesn't really seem clearer to me
>
> `wait (..., cond)` is just `while (!cond) { ... }`, so the outer if is
> redundant.
> On the other hand the if is important to the code it currently guards (for
> reasons explained in the comment), so it seems confusing to split its purpose
> in this way.
>
> The only way it makes sense to me is as a microoptimization, but it doesn't
> seem like a useful one.
> the preamble request we'll be building below might be different
The request in general is (filename, signals). Filename clearly can't change.
Signals can, but...
> i guess PreambleThread::update would be the natural place to communicate such
> changes
I don't think so, I think signals naturally come from elsewhere in the system
as `(filename, signal)` pairs and are best thought of as "sticky", where events
affect both current and future throttling requests.
As such I think we might have an API like `Throttler->updateSignals(Filename,
...)` and let the throttler join on filename, rather than require this joining
to be done artificially somewhere else in clangd.
One nice thing about this is if there's no throttler, we don't pay for keeping
track of the signals.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129100/new/
https://reviews.llvm.org/D129100
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
