puremourning marked an inline comment as done.
puremourning added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:152
+ std::forward<decltype(DBSF)>(DBSF),
+ Opts.AsyncThreadsCount );
+ } else {
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66031/new/
https://reviews.llvm.org/D66031
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
