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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
