clangd: use -j for background index pool

[puremourning]

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.

[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"

[puremourning]

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

[puremourning]

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]

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]

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

[puremourning]

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.

[puremourning]

Rebase on master

[sammccall]

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

[puremourning]

In D66031#1623855, @sammccall wrote:

Thanks! Want me to land this for you?

yes please! I don't have commit rights.

[sammccall]

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

[hans]

In D66031#1623935, @sammccall wrote:

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

Sure! Merged in r368569.

clang-tools-extra/trunk/clangd/ClangdServer.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
+#include <algorithm>
 #include <future>
 #include <memory>
 #include <mutex>
+#include <type_traits>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Update the FileIndex with new ASTs and plumb the diagnostics responses.
 struct UpdateIndexCallbacks : public ParsingCallbacks {
   UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer &DiagConsumer,
ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            DiagnosticsConsumer &DiagConsumer,
                            const Options &Opts)
     : FSProvider(FSProvider),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
                      : nullptr),
       GetClangTidyOptions(Opts.GetClangTidyOptions),
       SuggestMissingIncludes(Opts.SuggestMissingIncludes),
-      TweakFilter(Opts.TweakFilter),
-      WorkspaceRoot(Opts.WorkspaceRoot),
+      TweakFilter(Opts.TweakFilter), WorkspaceRoot(Opts.WorkspaceRoot),
       // Pass a callback into `WorkScheduler` to extract symbols from a newly
       // parsed file and rebuild the file index synchronously each time an AST
       // is parsed.
       // FIXME(ioeric): this can be slow and we may be able to index on less
       // critical paths.
       WorkScheduler(
           CDB, Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
           llvm::make_unique<UpdateIndexCallbacks>(
auto AddIndex = [&](SymbolIndex *Idx) {
     }
   };
   if (Opts.StaticIndex)
     AddIndex(Opts.StaticIndex);
   if (Opts.BackgroundIndex) {
     BackgroundIdx = llvm::make_unique<BackgroundIndex>(
         Context::current().clone(), FSProvider, CDB,
         BackgroundIndexStorage::createDiskBackedStorageFactory(
-            [&CDB](llvm::StringRef File) { return CDB.getProjectInfo(File); }));
+            [&CDB](llvm::StringRef File) { return CDB.getProjectInfo(File); }),
+        std::max(Opts.AsyncThreadsCount, 1u));
     AddIndex(BackgroundIdx.get());
   }
   if (DynamicIdx)
     AddIndex(DynamicIdx.get());
 }
 
 void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
                                WantDiagnostics WantDiags) {

clang-tools-extra/trunk/clangd/TUScheduler.cpp

 #include "Trace.h"
 #include "index/CanonicalIncludes.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Threading.h"
 #include <algorithm>
 #include <memory>
 #include <queue>
 #include <thread>
 
 namespace clang {
 namespace clangd {
 using std::chrono::steady_clock;
case TUAction::Idle:
     break;
   }
   return OS.str();
 }
 
 } // namespace
 
 unsigned getDefaultAsyncThreadsCount() {
-  unsigned HardwareConcurrency = std::thread::hardware_concurrency();
-  // C++ standard says that hardware_concurrency()
-  // may return 0, fallback to 1 worker thread in
-  // that case.
+  unsigned HardwareConcurrency = llvm::heavyweight_hardware_concurrency();
+  // heavyweight_hardware_concurrency may fall back to hardware_concurrency.
+  // C++ standard says that hardware_concurrency() may return 0; fallback to 1
+  // worker thread in that case.
   if (HardwareConcurrency == 0)
     return 1;
   return HardwareConcurrency;
 }
 
 FileStatus TUStatus::render(PathRef File) const {
   FileStatus FStatus;
   FStatus.uri = URIForFile::canonicalize(File, /*TUPath=*/File);

clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp

list<std::string> TweakList{
     desc("Specify a list of Tweaks to enable (only for clangd developers)."),
     Hidden,
     CommaSeparated,
 };
 
 opt<unsigned> WorkerThreadsCount{
     "j",
     cat(Misc),
-    desc("Number of async workers used by clangd"),
+    desc("Number of async workers used by clangd. Background index also "
+         "uses this many workers."),
     init(getDefaultAsyncThreadsCount()),
 };
 
 opt<Path> IndexFile{
     "index-file",
     cat(Misc),
     desc(
         "Index file to build the static index. The file must have been created "
values(
         clEnumValN(PCHStorageFlag::Disk, "disk", "store PCHs on disk"),
         clEnumValN(PCHStorageFlag::Memory, "memory", "store PCHs in memory")),
     init(PCHStorageFlag::Disk),
 };
 
 opt<bool> Sync{
     "sync",
     cat(Misc),
-    desc("Parse on main thread. If set, -j is ignored"),
+    desc("Handle client requests on main thread. Background index still uses "
+         "its own thread."),
     init(false),
     Hidden,
 };
 
 opt<JSONStreamStyle> InputStyle{
     "input-style",
     cat(Protocol),
     desc("Input JSON stream encoding"),