[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL310821: [clangd] Use multiple working threads in clangd. 
(authored by ibiryukov).

Changed prior to commit:
  https://reviews.llvm.org/D36261?vs=110381=110922#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D36261

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
  clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h
@@ -26,7 +26,7 @@
 /// dispatch and ClangdServer together.
 class ClangdLSPServer {
 public:
-  ClangdLSPServer(JSONOutput , bool RunSynchronously,
+  ClangdLSPServer(JSONOutput , unsigned AsyncThreadsCount,
   llvm::Optional ResourceDir);
 
   /// Run LSP server loop, receiving input for it from \p In. \p In must be
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -220,10 +220,10 @@
   R"(,"result":[)" + Locations + R"(]})");
 }
 
-ClangdLSPServer::ClangdLSPServer(JSONOutput , bool RunSynchronously,
+ClangdLSPServer::ClangdLSPServer(JSONOutput , unsigned AsyncThreadsCount,
  llvm::Optional ResourceDir)
 : Out(Out), DiagConsumer(*this),
-  Server(CDB, DiagConsumer, FSProvider, RunSynchronously, ResourceDir) {}
+  Server(CDB, DiagConsumer, FSProvider, AsyncThreadsCount, ResourceDir) {}
 
 void ClangdLSPServer::run(std::istream ) {
   assert(!IsDone && "Run was called before");
Index: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
@@ -16,14 +16,20 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace clang;
 using namespace clang::clangd;
 
-static llvm::cl::opt
-RunSynchronously("run-synchronously",
- llvm::cl::desc("Parse on main thread"),
- llvm::cl::init(false), llvm::cl::Hidden);
+static llvm::cl::opt
+WorkerThreadsCount("j",
+   llvm::cl::desc("Number of async workers used by clangd"),
+   llvm::cl::init(getDefaultAsyncThreadsCount()));
+
+static llvm::cl::opt RunSynchronously(
+"run-synchronously",
+llvm::cl::desc("Parse on main thread. If set, -j is ignored"),
+llvm::cl::init(false), llvm::cl::Hidden);
 
 static llvm::cl::opt
 ResourceDir("resource-dir",
@@ -33,6 +39,17 @@
 int main(int argc, char *argv[]) {
   llvm::cl::ParseCommandLineOptions(argc, argv, "clangd");
 
+  if (!RunSynchronously && WorkerThreadsCount == 0) {
+llvm::errs() << "A number of worker threads cannot be 0. Did you mean to "
+"specify -run-synchronously?";
+return 1;
+  }
+
+  // Ignore -j option if -run-synchonously is used.
+  // FIXME: a warning should be shown here.
+  if (RunSynchronously)
+WorkerThreadsCount = 0;
+
   llvm::raw_ostream  = llvm::outs();
   llvm::raw_ostream  = llvm::errs();
   JSONOutput Out(Outs, Logs);
@@ -43,6 +60,7 @@
   llvm::Optional ResourceDirRef = None;
   if (!ResourceDir.empty())
 ResourceDirRef = ResourceDir;
-  ClangdLSPServer LSPServer(Out, RunSynchronously, ResourceDirRef);
+
+  ClangdLSPServer LSPServer(Out, WorkerThreadsCount, ResourceDirRef);
   LSPServer.run(std::cin);
 }
Index: clang-tools-extra/trunk/clangd/ClangdServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdServer.h
+++ clang-tools-extra/trunk/clangd/ClangdServer.h
@@ -101,11 +101,20 @@
 
 class ClangdServer;
 
-/// Handles running WorkerRequests of ClangdServer on a separate threads.
-/// Currently runs only one worker thread.
+/// Returns a number of a default async threads to use for ClangdScheduler.
+/// Returned value is always >= 1 (i.e. will not cause requests to be processed
+/// synchronously).
+unsigned getDefaultAsyncThreadsCount();
+
+/// Handles running WorkerRequests of ClangdServer on a number of worker
+/// threads.
 class ClangdScheduler {
 public:
-  ClangdScheduler(bool RunSynchronously);
+  /// If \p AsyncThreadsCount is 0, requests added using addToFront and addToEnd
+  /// will be processed synchronously on the calling thread.
+  // Otherwise, \p AsyncThreadsCount threads will be created to schedule the
+  // requests.
+  ClangdScheduler(unsigned AsyncThreadsCount);
   ~ClangdScheduler();
 
   /// Add a new request 

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Manuel Klimek via Phabricator via cfe-commits]

klimek accepted this revision.
klimek added a comment.
This revision is now accepted and ready to land.

lg


https://reviews.llvm.org/D36261



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Added a test that exercises some multi-threading behaviour. 
It really finds bugs, at least it found a problem that was fixed by 
https://reviews.llvm.org/D36397. (Will make sure to submit that other change 
before this one).

As discussed with @klimek, will move to `llvm::ThreadPool` after submitting a 
fix for it to allow functions with moveable-only arguments to be used there 
(that's what clangd uses).


https://reviews.llvm.org/D36261



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 110381.
ilya-biryukov added a comment.

- Added a stress test for multithreading.


https://reviews.llvm.org/D36261

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -136,18 +137,23 @@
 static const std::chrono::seconds DefaultFutureTimeout =
 std::chrono::seconds(10);
 
+static bool diagsContainErrors(ArrayRef Diagnostics) {
+  for (const auto  : Diagnostics) {
+// FIXME: severities returned by clangd should have a descriptive
+// diagnostic severity enum
+const int ErrorSeverity = 1;
+if (DiagAndFixIts.Diag.severity == ErrorSeverity)
+  return true;
+  }
+  return false;
+}
+
 class ErrorCheckingDiagConsumer : public DiagnosticsConsumer {
 public:
   void
   onDiagnosticsReady(PathRef File,
  Tagged Diagnostics) override {
-bool HadError = false;
-for (const auto  : Diagnostics.Value) {
-  // FIXME: severities returned by clangd should have a descriptive
-  // diagnostic severity enum
-  const int ErrorSeverity = 1;
-  HadError = DiagAndFixIts.Diag.severity == ErrorSeverity;
-}
+bool HadError = diagsContainErrors(Diagnostics.Value);
 
 std::lock_guard Lock(Mutex);
 HadErrorInLastDiags = HadError;
@@ -196,24 +202,46 @@
   std::vector ExtraClangFlags;
 };
 
+IntrusiveRefCntPtr
+buildTestFS(llvm::StringMap const ) {
+  IntrusiveRefCntPtr MemFS(
+  new vfs::InMemoryFileSystem);
+  for (auto  : Files)
+MemFS->addFile(FileAndContents.first(), time_t(),
+   llvm::MemoryBuffer::getMemBuffer(FileAndContents.second,
+FileAndContents.first()));
+
+  auto OverlayFS = IntrusiveRefCntPtr(
+  new vfs::OverlayFileSystem(vfs::getTempOnlyFS()));
+  OverlayFS->pushOverlay(std::move(MemFS));
+  return OverlayFS;
+}
+
+class ConstantFSProvider : public FileSystemProvider {
+public:
+  ConstantFSProvider(IntrusiveRefCntPtr FS,
+ VFSTag Tag = VFSTag())
+  : FS(std::move(FS)), Tag(std::move(Tag)) {}
+
+  Tagged
+  getTaggedFileSystem(PathRef File) override {
+return make_tagged(FS, Tag);
+  }
+
+private:
+  IntrusiveRefCntPtr FS;
+  VFSTag Tag;
+};
+
 class MockFSProvider : public FileSystemProvider {
 public:
   Tagged
   getTaggedFileSystem(PathRef File) override {
-IntrusiveRefCntPtr MemFS(
-new vfs::InMemoryFileSystem);
 if (ExpectedFile)
   EXPECT_EQ(*ExpectedFile, File);
 
-for (auto  : Files)
-  MemFS->addFile(FileAndContents.first(), time_t(),
- llvm::MemoryBuffer::getMemBuffer(FileAndContents.second,
-  FileAndContents.first()));
-
-auto OverlayFS = IntrusiveRefCntPtr(
-new vfs::OverlayFileSystem(vfs::getTempOnlyFS()));
-OverlayFS->pushOverlay(std::move(MemFS));
-return make_tagged(OverlayFS, Tag);
+auto FS = buildTestFS(Files);
+return make_tagged(FS, Tag);
   }
 
   llvm::Optional> ExpectedFile;
@@ -272,17 +300,17 @@
 MockFSProvider FS;
 ErrorCheckingDiagConsumer DiagConsumer;
 MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
-ClangdServer Server(CDB, DiagConsumer, FS,
-/*RunSynchronously=*/false);
+ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount());
 for (const auto  : ExtraFiles)
   FS.Files[getVirtualTestFilePath(FileWithContents.first)] =
   FileWithContents.second;
 
 auto SourceFilename = getVirtualTestFilePath(SourceFileRelPath);
 
 FS.ExpectedFile = SourceFilename;
 
-// Have to sync reparses because RunSynchronously is false.
+// Have to sync reparses because requests are processed on the calling
+// thread.
 auto AddDocFuture = Server.addDocument(SourceFilename, SourceContents);
 
 auto Result = dumpASTWithoutMemoryLocs(Server, SourceFilename);
@@ -334,8 +362,7 @@
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
-  ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount());
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
@@ -379,8 +406,7 @@
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 
-  ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  ClangdServer Server(CDB, 

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D36261#834902, @klimek wrote:

> High-level question: Why can't we use llvm::ThreadPool?


It is not an in-place replacement as it does not allow to prioritize new tasks 
over old ones (new tasks are usually more important for clangd as the old ones 
are often outdated when new ones are added).
I looked into using `llvm::ThreadPool` before, but decided to stay with our own 
implementation.
It gives us more flexibility over threading and it is not hard to maintain (I 
think it's under 100 lines of code, and the code is rather simple).


https://reviews.llvm.org/D36261



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

High-level question: Why can't we use llvm::ThreadPool?


https://reviews.llvm.org/D36261



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.h:121-122
+public:
+  /// Returns the number of threads to use when shouldRunsynchronously() is
+  /// false. Must not be called if shouldRunsynchronously() is true.
+  unsigned getThreadsCount();

ilya-biryukov wrote:
> klimek wrote:
> > Why not: 1 -> run synchronously, > 1, run in parallel?
> Currently 1 means: start 1 worker thread to run async operations (that thread 
> is separate from the main thread).
> This makes sense for clangd, as if you do that, you still get code completion 
> that doesn't wait for diagnostics to finish.
> On the other hand, it's useful to have `-run-synchronously` for some tests.
As discussed, replaced with `unsigned AsyncThreadsCount`. Zero means run 
synchronously.


https://reviews.llvm.org/D36261



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 109534.
ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added a comment.

- Fixed more typos/inconsistences in comments, pointed out by @krasimir.


https://reviews.llvm.org/D36261

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -272,17 +272,17 @@
 MockFSProvider FS;
 ErrorCheckingDiagConsumer DiagConsumer;
 MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
-ClangdServer Server(CDB, DiagConsumer, FS,
-/*RunSynchronously=*/false);
+ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount());
 for (const auto  : ExtraFiles)
   FS.Files[getVirtualTestFilePath(FileWithContents.first)] =
   FileWithContents.second;
 
 auto SourceFilename = getVirtualTestFilePath(SourceFileRelPath);
 
 FS.ExpectedFile = SourceFilename;
 
-// Have to sync reparses because RunSynchronously is false.
+// Have to sync reparses because requests are processed on the calling
+// thread.
 auto AddDocFuture = Server.addDocument(SourceFilename, SourceContents);
 
 auto Result = dumpASTWithoutMemoryLocs(Server, SourceFilename);
@@ -334,8 +334,7 @@
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
-  ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount());
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
@@ -379,8 +378,7 @@
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 
-  ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount());
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
@@ -425,16 +423,17 @@
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+  // Run ClangdServer synchronously.
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/true);
+  /*AsyncThreadsCount=*/0);
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
   const auto SourceContents = "int a;";
   FS.Files[FooCpp] = SourceContents;
   FS.ExpectedFile = FooCpp;
 
-  // No need to sync reparses, because RunSynchronously is set
-  // to true.
+  // No need to sync reparses, because requests are processed on the calling
+  // thread.
   FS.Tag = "123";
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS.Tag);
@@ -456,8 +455,9 @@
   CDB.ExtraClangFlags.insert(CDB.ExtraClangFlags.end(),
  {"-xc++", "-target", "x86_64-linux-unknown",
   "-m64", "--gcc-toolchain=/randomusr"});
+  // Run ClangdServer synchronously.
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/true);
+  /*AsyncThreadsCount=*/0);
 
   // Just a random gcc version string
   SmallString<8> Version("4.9.3");
@@ -486,8 +486,8 @@
 )cpp";
   FS.Files[FooCpp] = SourceContents;
 
-  // No need to sync reparses, because RunSynchronously is set
-  // to true.
+  // No need to sync reparses, because requests are processed on the calling
+  // thread.
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 
@@ -516,8 +516,7 @@
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 
-  ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount());
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
   const auto SourceContents = R"cpp(
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -16,14 +16,20 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace clang;
 using namespace clang::clangd;
 
-static llvm::cl::opt
-RunSynchronously("run-synchronously",
- llvm::cl::desc("Parse on main thread"),
- llvm::cl::init(false), llvm::cl::Hidden);
+static llvm::cl::opt
+WorkerThreadsCount("j",
+   llvm::cl::desc("Number of async workers used by clangd"),
+   llvm::cl::init(getDefaultAsyncThreadsCount()));
+
+static llvm::cl::opt RunSynchronously(
+

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/tool/ClangdMain.cpp:69
+  SchedulingOptions SchedOpts =
+  !RunSynchronously ? SchedulingOptions::RunOnWorkerThreads(ThreadsCount)
+: SchedulingOptions::RunOnCallingThread();

krasimir wrote:
> Consider inverting this condition.
Thanks, good point. I also like non-negated conditions more.
That code was removed in the updated version, though.


https://reviews.llvm.org/D36261



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 109532.
ilya-biryukov added a comment.

- Removed SchedulingOptions altogether, replaced with AsyncThreadsCount.


https://reviews.llvm.org/D36261

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -272,17 +272,17 @@
 MockFSProvider FS;
 ErrorCheckingDiagConsumer DiagConsumer;
 MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
-ClangdServer Server(CDB, DiagConsumer, FS,
-/*RunSynchronously=*/false);
+ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount());
 for (const auto  : ExtraFiles)
   FS.Files[getVirtualTestFilePath(FileWithContents.first)] =
   FileWithContents.second;
 
 auto SourceFilename = getVirtualTestFilePath(SourceFileRelPath);
 
 FS.ExpectedFile = SourceFilename;
 
-// Have to sync reparses because RunSynchronously is false.
+// Have to sync reparses because requests are processed on the calling
+// thread.
 auto AddDocFuture = Server.addDocument(SourceFilename, SourceContents);
 
 auto Result = dumpASTWithoutMemoryLocs(Server, SourceFilename);
@@ -334,8 +334,7 @@
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
-  ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount());
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
@@ -379,8 +378,7 @@
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 
-  ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount());
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
@@ -425,16 +423,17 @@
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+  // Run ClangdServer synchronously.
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/true);
+  /*AsyncThreadsCount=*/0);
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
   const auto SourceContents = "int a;";
   FS.Files[FooCpp] = SourceContents;
   FS.ExpectedFile = FooCpp;
 
-  // No need to sync reparses, because RunSynchronously is set
-  // to true.
+  // No need to sync reparses, because requests are processed on the calling
+  // thread.
   FS.Tag = "123";
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS.Tag);
@@ -456,8 +455,9 @@
   CDB.ExtraClangFlags.insert(CDB.ExtraClangFlags.end(),
  {"-xc++", "-target", "x86_64-linux-unknown",
   "-m64", "--gcc-toolchain=/randomusr"});
+  // Run ClangdServer synchronously.
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/true);
+  /*AsyncThreadsCount=*/0);
 
   // Just a random gcc version string
   SmallString<8> Version("4.9.3");
@@ -486,8 +486,8 @@
 )cpp";
   FS.Files[FooCpp] = SourceContents;
 
-  // No need to sync reparses, because RunSynchronously is set
-  // to true.
+  // No need to sync reparses, because requests are processed on the calling
+  // thread.
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 
@@ -516,8 +516,7 @@
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 
-  ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount());
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
   const auto SourceContents = R"cpp(
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -16,14 +16,20 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace clang;
 using namespace clang::clangd;
 
-static llvm::cl::opt
-RunSynchronously("run-synchronously",
- llvm::cl::desc("Parse on main thread"),
- llvm::cl::init(false), llvm::cl::Hidden);
+static llvm::cl::opt
+WorkerThreadsCount("j",
+   llvm::cl::desc("Number of async workers used by clangd"),
+   llvm::cl::init(getDefaultAsyncThreadsCount()));
+
+static llvm::cl::opt RunSynchronously(
+"run-synchronously",
+llvm::cl::desc("Parse on 

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

Great!




Comment at: clangd/ClangdServer.h:131
+
 /// Handles running WorkerRequests of ClangdServer on a separate threads.
 /// Currently runs only one worker thread.

typo: "on separate threads"



Comment at: clangd/ClangdServer.h:176
   std::mutex Mutex;
   /// We run some tasks on a separate threads(parsing, CppFile cleanup).
   /// This thread looks into RequestQueue to find requests to handle and

typo: "on separate threads"



Comment at: clangd/tool/ClangdMain.cpp:24
 
-static llvm::cl::opt
-RunSynchronously("run-synchronously",
- llvm::cl::desc("Parse on main thread"),
- llvm::cl::init(false), llvm::cl::Hidden);
+static unsigned getDefaultThreadsCount() {
+  unsigned HardwareConcurrency = std::thread::hardware_concurrency();

Extract this somewhere when it can be reused both by this and in 
`clangd/ClangdServer.cpp:90`.



Comment at: clangd/tool/ClangdMain.cpp:69
+  SchedulingOptions SchedOpts =
+  !RunSynchronously ? SchedulingOptions::RunOnWorkerThreads(ThreadsCount)
+: SchedulingOptions::RunOnCallingThread();

Consider inverting this condition.


https://reviews.llvm.org/D36261



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 109527.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.

Addressed review comments.

- Fixed typos.
- Renamed SchedulingParams to SchedulingOptions.


https://reviews.llvm.org/D36261

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -273,16 +273,17 @@
 ErrorCheckingDiagConsumer DiagConsumer;
 MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 ClangdServer Server(CDB, DiagConsumer, FS,
-/*RunSynchronously=*/false);
+SchedulingOptions::RunOnDefaultNumberOfThreads());
 for (const auto  : ExtraFiles)
   FS.Files[getVirtualTestFilePath(FileWithContents.first)] =
   FileWithContents.second;
 
 auto SourceFilename = getVirtualTestFilePath(SourceFileRelPath);
 
 FS.ExpectedFile = SourceFilename;
 
-// Have to sync reparses because RunSynchronously is false.
+// Have to sync reparses because requests are processed on the calling
+// thread.
 auto AddDocFuture = Server.addDocument(SourceFilename, SourceContents);
 
 auto Result = dumpASTWithoutMemoryLocs(Server, SourceFilename);
@@ -335,7 +336,7 @@
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  SchedulingOptions::RunOnDefaultNumberOfThreads());
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
@@ -380,7 +381,7 @@
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  SchedulingOptions::RunOnDefaultNumberOfThreads());
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
@@ -426,15 +427,15 @@
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/true);
+  SchedulingOptions::RunOnCallingThread());
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
   const auto SourceContents = "int a;";
   FS.Files[FooCpp] = SourceContents;
   FS.ExpectedFile = FooCpp;
 
-  // No need to sync reparses, because RunSynchronously is set
-  // to true.
+  // No need to sync reparses, because requests are processed on the calling
+  // thread.
   FS.Tag = "123";
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS.Tag);
@@ -457,7 +458,7 @@
  {"-xc++", "-target", "x86_64-linux-unknown",
   "-m64", "--gcc-toolchain=/randomusr"});
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/true);
+  SchedulingOptions::RunOnCallingThread());
 
   // Just a random gcc version string
   SmallString<8> Version("4.9.3");
@@ -486,8 +487,8 @@
 )cpp";
   FS.Files[FooCpp] = SourceContents;
 
-  // No need to sync reparses, because RunSynchronously is set
-  // to true.
+  // No need to sync reparses, because requests are processed on the calling
+  // thread.
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 
@@ -517,7 +518,7 @@
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  SchedulingOptions::RunOnDefaultNumberOfThreads());
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
   const auto SourceContents = R"cpp(
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -16,14 +16,30 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace clang;
 using namespace clang::clangd;
 
-static llvm::cl::opt
-RunSynchronously("run-synchronously",
- llvm::cl::desc("Parse on main thread"),
- llvm::cl::init(false), llvm::cl::Hidden);
+static unsigned getDefaultThreadsCount() {
+  unsigned HardwareConcurrency = std::thread::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;
+}
+
+static llvm::cl::opt
+ThreadsCount("j",
+ llvm::cl::desc("Number of parallel workers used by clangd."),
+ llvm::cl::init(getDefaultThreadsCount()));
+

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.h:121-122
+public:
+  /// Returns the number of threads to use when shouldRunsynchronously() is
+  /// false. Must not be called if shouldRunsynchronously() is true.
+  unsigned getThreadsCount();

klimek wrote:
> Why not: 1 -> run synchronously, > 1, run in parallel?
Currently 1 means: start 1 worker thread to run async operations (that thread 
is separate from the main thread).
This makes sense for clangd, as if you do that, you still get code completion 
that doesn't wait for diagnostics to finish.
On the other hand, it's useful to have `-run-synchronously` for some tests.


https://reviews.llvm.org/D36261



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: clangd/ClangdServer.h:105
+/// A helper class to pass concurrency parameters to ClangdScheduler.
+class SchedulingParams {
+public:

I think calling it "Options" is more idiomatic.



Comment at: clangd/ClangdServer.h:110
+  static SchedulingParams RunOnCallingThread();
+  /// Indicates then requests should be executed on separate worker threads. In
+  /// total \p ThreadsCount working threads will be created.

Typo: Indicates *that*.



Comment at: clangd/ClangdServer.h:121-122
+public:
+  /// Returns the number of threads to use when shouldRunsynchronously() is
+  /// false. Must not be called if shouldRunsynchronously() is true.
+  unsigned getThreadsCount();

Why not: 1 -> run synchronously, > 1, run in parallel?


https://reviews.llvm.org/D36261



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: clangd/ClangdServer.h:107
+public:
+  /// Indicates that requests must be executed immidieately on the calling
+  /// thread.

Typo: immediately


https://reviews.llvm.org/D36261



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.

https://reviews.llvm.org/D36261

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -273,16 +273,17 @@
 ErrorCheckingDiagConsumer DiagConsumer;
 MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 ClangdServer Server(CDB, DiagConsumer, FS,
-/*RunSynchronously=*/false);
+SchedulingParams::RunOnDefaultNumberOfThreads());
 for (const auto  : ExtraFiles)
   FS.Files[getVirtualTestFilePath(FileWithContents.first)] =
   FileWithContents.second;
 
 auto SourceFilename = getVirtualTestFilePath(SourceFileRelPath);
 
 FS.ExpectedFile = SourceFilename;
 
-// Have to sync reparses because RunSynchronously is false.
+// Have to sync reparses because requests are processed on the calling
+// thread.
 auto AddDocFuture = Server.addDocument(SourceFilename, SourceContents);
 
 auto Result = dumpASTWithoutMemoryLocs(Server, SourceFilename);
@@ -335,7 +336,7 @@
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  SchedulingParams::RunOnDefaultNumberOfThreads());
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
@@ -380,7 +381,7 @@
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  SchedulingParams::RunOnDefaultNumberOfThreads());
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
@@ -426,15 +427,15 @@
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/true);
+  SchedulingParams::RunOnCallingThread());
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
   const auto SourceContents = "int a;";
   FS.Files[FooCpp] = SourceContents;
   FS.ExpectedFile = FooCpp;
 
-  // No need to sync reparses, because RunSynchronously is set
-  // to true.
+  // No need to sync reparses, because requests are processed on the calling
+  // thread.
   FS.Tag = "123";
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS.Tag);
@@ -457,7 +458,7 @@
  {"-xc++", "-target", "x86_64-linux-unknown",
   "-m64", "--gcc-toolchain=/randomusr"});
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/true);
+  SchedulingParams::RunOnCallingThread());
 
   // Just a random gcc version string
   SmallString<8> Version("4.9.3");
@@ -486,8 +487,8 @@
 )cpp";
   FS.Files[FooCpp] = SourceContents;
 
-  // No need to sync reparses, because RunSynchronously is set
-  // to true.
+  // No need to sync reparses, because requests are processed on the calling
+  // thread.
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 
@@ -517,7 +518,7 @@
   MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
 
   ClangdServer Server(CDB, DiagConsumer, FS,
-  /*RunSynchronously=*/false);
+  SchedulingParams::RunOnDefaultNumberOfThreads());
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
   const auto SourceContents = R"cpp(
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -16,14 +16,30 @@
 #include 
 #include 
 #include 
+#include 
 
 using namespace clang;
 using namespace clang::clangd;
 
-static llvm::cl::opt
-RunSynchronously("run-synchronously",
- llvm::cl::desc("Parse on main thread"),
- llvm::cl::init(false), llvm::cl::Hidden);
+static unsigned getDefaultThreadsCount() {
+  unsigned HardwareConcurrency = std::thread::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;
+}
+
+static llvm::cl::opt
+ThreadsCount("j",
+ llvm::cl::desc("Number of parallel workers used by clangd."),
+ llvm::cl::init(getDefaultThreadsCount()));
+
+static llvm::cl::opt RunSynchronously(
+"run-synchronously",
+llvm::cl::desc("Parse on main thread. If set, -j is ignored."),
+llvm::cl::init(false), llvm::cl::Hidden);
 
 static