sammccall added a comment.
I've sent https://reviews.llvm.org/D82352 to clean up some of the logic in
clangd.
================
Comment at: clang-tools-extra/clangd/index/Background.cpp:154
assert(this->IndexStorageFactory && "Storage factory can not be null!");
- for (unsigned I = 0; I < ThreadPoolSize; ++I) {
+ for (unsigned I = 0; I < Rebuilder.TUsBeforeFirstBuild; ++I) {
ThreadPool.runAsync("background-worker-" + llvm::Twine(I + 1), [this] {
----------------
aganea wrote:
> sammccall wrote:
> > Hmm, I finally stumbled across this today when editing the rebuild policy.
> > I do wish this hadn't been changed without understanding what this code is
> > doing or getting review from an owner.
> >
> > After this change, modifying the background-index rebuild frequency (which
> > was initially tuned to be roughly "after each thread built one file") has
> > the side-effect of changing the number of threads used for background
> > indexing!
> >
> > Less seriously, the use of zero to mean "all the threads" is problematic
> > here because in the other thread roots in this project we use zero to mean
> > "no threads, work synchronously".
> >
> > I'll look into reverting the clangd parts of this patch.
> Sorry about that Sam. Do you think 'one' could be used in clangd instead?
> That is the common value used across other parts of LLVM to signify 'no
> threads', and also when `LLVM_ENABLE_THREADS` is off. 'zero' means to use the
> default settings for the thread strategy. That is,
> `llvm::hardware_concurrency(0)` means to use all hardware threads; or
> `llvm::heavyweight_hardware_concurrency(0)` means to use all hardware cores,
> but only one `std::thread` per core.
No worries, it happens and nothing came of it except a little head-scratching.
Sorry for being grumpy, I shouldn't send email late at night...
> Do you think 'one' could be used in clangd instead? That is the common value
> used across other parts of LLVM to signify 'no threads', and also when
> LLVM_ENABLE_THREADS is off
One can't be used here, it means "spawn one background thread". (Clangd has
several distinct components that spawn threads).
FWIW, clangd can't be built without LLVM_ENABLE_THREADS - it needs concurrency
to be useful, and we designed around threads. Most of the threading can be
turned off *at runtime* for certain tests (that's what a threadpool size of
zero means) but not the background index - we just disable it instead.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71775/new/
https://reviews.llvm.org/D71775
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
