sammccall accepted this revision.
sammccall added a comment.
Thanks! Want me to land this for you?
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:152
+ std::forward<decltype(DBSF)>(DBSF),
+ Opts.AsyncThreadsCount );
+ } else {
----------------
puremourning wrote:
> sammccall wrote:
> > puremourning wrote:
> > > sammccall wrote:
> > > > can we use `std::max(Opts.AsyncThreadsCount, 1)` instead?
> > > >
> > > > Having `-sync -background-index` use one thread seems less weird than
> > > > having it use all the cores.
> > > > (Or at least not more weird, and simpler in the code here)
> > > Hmm. What I was thinking is more that if you specify none of sync or -j,
> > > you should get physical cores as you do now.
> > >
> > > But I realise that this change doesn't do that, because AsyncThreadsCount
> > > defaults slightly differently to
> > > `llvm::heavyweight_hardware_concurrency()` (it uses
> > > std::thread::hardware_concurrency)
> > >
> > > The difference is pretty small, so probably not material ?
> > yikes, I forgot about that difference.
> >
> > We observed *significantly* worse performance and responsiveness when
> > background threads was equal to the number of hardware threads rather than
> > number of cores.
> >
> > If you don't mind, we should just use cores for everything: change
> > `getDefaultAsyncThreadCount()` in TUScheduler.cpp to call
> > llvm::heavyweight_hardware_concurrency() instead of
> > std::thread::hardware_concurrency().
> Sure thing. That makes sense.
>
> It occurs to me that we might want to change the default value used by
> `BackgroundIndex` constructor, because it also can end up with `0` return
> from `heavyweight_hardware_concurrency`.
>
> Worth changing that here ? I think the default is only used by the tests now
> though, so probably not a big issue.
I think better to keep this patch tiny to make it easier to merge onto the
release branch.
I'll fix `heavyweight_hardware_concurrency` separately to never return zero,
but the tests will be fine meanwhile :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66031/new/
https://reviews.llvm.org/D66031
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
