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

sammccall added inline comments. Comment at: clang-tools-extra/clangd/TUScheduler.cpp:499 // Build the preamble and let the waiters know about it. build(std::move(*CurrentReq)); } kadircet wrote: > regarding the flakiness, what about

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

kadircet added inline comments. Comment at: clang-tools-extra/clangd/TUScheduler.cpp:499 // Build the preamble and let the waiters know about it. build(std::move(*CurrentReq)); } regarding the flakiness, what about resetting the

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

sammccall added inline comments. Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1499 + +S.remove(Filenames.back()); +// Now shut down the TU Scheduler. kadircet wrote: > sequencing is hard here but it'd be nice to ensure release is

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

sammccall added a comment. In D129100#3634548 , @vitalybuka wrote: > This looks flaky https://lab.llvm.org/buildbot/#/builders/5/builds/25929 Sorry about that, I believe this is fixed in eb64dbd6e0e617298579d32372fb92e595816d45

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

vitalybuka added a comment. This looks flaky https://lab.llvm.org/buildbot/#/builders/5/builds/25929 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129100/new/ https://reviews.llvm.org/D129100 ___

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

Hi, On 2022-07-06 16:28:14 +, Sam McCall via Phabricator wrote: > sammccall added a comment. > > In D129100#3633073 , @anarazel > wrote: > > > This caused llvm builds to fail for me (using clang-14, debug, debian > > unstable, lld as linker): > >

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

sammccall added a comment. In D129100#3633073 , @anarazel wrote: > This caused llvm builds to fail for me (using clang-14, debug, debian > unstable, lld as linker): Thanks, I have a very similar setup but the build passed locally, I'll never

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

anarazel added a comment. This caused llvm builds to fail for me (using clang-14, debug, debian unstable, lld as linker): FAILED: tools/clang/tools/extra/clangd/unittests/ClangdTests : && /usr/bin/clang++-14 -gz=zlib -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rGed0e20d5e8c5: [clangd] Support external throttler for preamble builds (authored by sammccall). Changed

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

sammccall marked 5 inline comments as done. sammccall added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.h:108 +/// This throttler controls which preambles may be built at a given time. +clangd::PreambleThrottler *PreambleThrottler = nullptr; +

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

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, lgtm! Comment at: clang-tools-extra/clangd/ClangdServer.h:108 +/// This throttler controls which preambles may be built at a given time. +

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

sammccall updated this revision to Diff 442319. sammccall marked an inline comment as done. sammccall added a comment. address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129100/new/ https://reviews.llvm.org/D129100 Files:

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

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 Lock(Mu); kadircet

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

sammccall updated this revision to Diff 442309. sammccall added a comment. remove dead code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129100/new/ https://reviews.llvm.org/D129100 Files: clang-tools-extra/clangd/ClangdServer.cpp

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

sammccall updated this revision to Diff 442308. sammccall added a comment. Simplify, leaning more on the throttler's lifetime and expecting it to manage state. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129100/new/

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

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. +

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

sammccall planned changes to this revision. sammccall added a comment. Summarizing an offline discussion: - ThrottlerState isolates complexity inside TUScheduler by absolving Throttler of the need to worry about callback lifetimes - but it does so at the cost of extra aggregate complexity

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

sammccall added a comment. I wanted to send a first version out for feedback even if incomplete. (I want to do more extensive tests, and likely we should think about having an in-tree throttler on by default) To motivate the design: - when attempting to acquire a slot, we want to wait on

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

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added subscribers: usaxena95, arphaman, javed.absar. Herald added a project: All. sammccall requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: