[PATCH] D66031: clangd: use -j for background index pool

[Sam McCall via Phabricator via cfe-commits]

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