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

[Ben Jackson via Phabricator via cfe-commits]

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