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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

we've already discussed most of these so no action needed, but here are some of them before they become completely irrelevant :P



================
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);
----------------
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.


================
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)
----------------
any reason for capturing locals here now ?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:500
+
+          ReqCV.wait(Lock, [&] { return TState->ready() || Done; });
+          if (Done) {
----------------
we can move the wait into the `if block` above, i think.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:500
+
+          ReqCV.wait(Lock, [&] { return TState->ready() || Done; });
+          if (Done) {
----------------
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?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1573
+  case PreambleAction::Queued:
+    Result.push_back("headers are queued");
+    break;
----------------
let's use "includes" here, similar to above.


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:114
+  // If it fails (e.g. races), resources are acquired and must be released.
+  virtual CancelFn acquire(llvm::StringRef Filename, AcquireCallback) = 0;
+
----------------
can we make the first parameter a struct instead? i feel like we might end up adding more parameters describing the reason for the request here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129100