Page MenuHomePhabricator

clangd: use -j for background index pool
ClosedPublic

Authored by puremourning on Aug 9 2019, 1:59 PM.

Details

Summary

clangd supports a -j option to limit the amount of threads to use for parsing
TUs. However, when using -background-index (the default in later versions of
clangd), the parallelism used by clangd defaults to the hardware_parallelisn,
i.e. number of physical cores.

On shared hardware environments, with large projects, this can significantly
affect performance with no way to tune it down.

This change makes the -j parameter apply equally to parsing and background
index. It's not perfect, because the total number of threads is 2x the -j value,
which may still be unexpected. But at least this change allows users to prevent
clangd using all CPU cores.

Diff Detail

Repository
rL LLVM

Event Timeline

puremourning created this revision.Aug 9 2019, 1:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 1:59 PM
sammccall accepted this revision.Aug 9 2019, 2:18 PM
sammccall added a subscriber: sammccall.

Yeah, this is much better than only having -background-index=no.

I think we should merge this into the 9.0 branch if we still can.

clang-tools-extra/clangd/ClangdServer.cpp
152 ↗(On Diff #214447)

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)

156 ↗(On Diff #214447)

nit: clang-format

clang-tools-extra/clangd/tool/ClangdMain.cpp
311 ↗(On Diff #214447)

while here... "parse" is a bit outdated, could this be something like "Handle requests on the main thread. The background index still gets its own thread"

This revision is now accepted and ready to land.Aug 9 2019, 2:18 PM
puremourning marked 3 inline comments as done.

When -sync -background-index supplied, use 1 thread for background index.

puremourning added inline comments.Aug 9 2019, 2:42 PM
clang-tools-extra/clangd/ClangdServer.cpp
152 ↗(On Diff #214447)

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 ?

sammccall added inline comments.Aug 9 2019, 2:56 PM
clang-tools-extra/clangd/ClangdServer.cpp
149 ↗(On Diff #214454)

(nit: we tend to just write 1u)

152 ↗(On Diff #214447)

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().

puremourning marked an inline comment as not done.

Always use physical cores rather than logical cores for best performance.

puremourning marked an inline comment as done.Aug 9 2019, 3:16 PM
puremourning added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
152 ↗(On Diff #214447)

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.

Rebase on master

sammccall accepted this revision.Aug 9 2019, 3:44 PM

Thanks! Want me to land this for you?

clang-tools-extra/clangd/ClangdServer.cpp
152 ↗(On Diff #214447)

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 :-)

Thanks! Want me to land this for you?

yes please! I don't have commit rights.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 4:06 PM
sammccall added a subscriber: hans.Aug 9 2019, 4:40 PM

@hans If you don't mind merging this, it's a nice usability improvement.

hans added a comment.Aug 12 2019, 6:39 AM

@hans If you don't mind merging this, it's a nice usability improvement.

Sure! Merged in r368569.