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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:152
+          std::forward<decltype(DBSF)>(DBSF),
+          Opts.AsyncThreadsCount );
+    } else {
----------------
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().


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:149
+            [&CDB](llvm::StringRef File) { return CDB.getProjectInfo(File); }),
+        std::max(Opts.AsyncThreadsCount, static_cast<unsigned int>(1)));
     AddIndex(BackgroundIdx.get());
----------------
(nit: we tend to just write `1u`)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66031/new/

https://reviews.llvm.org/D66031