[PATCH] D129100: [clangd] Support external throttler for preamble builds

[Sam McCall via Phabricator via cfe-commits]

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