[PATCH] D42174: [clangd] Refactored threading in ClangdServer
This revision was automatically updated to reflect the committed changes.
Closed by commit rL323851: [clangd] Refactored threading in ClangdServer
(authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D42174
Files:
clang-tools-extra/trunk/clangd/CMakeLists.txt
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/ClangdUnitStore.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/clangd/Threading.cpp
clang-tools-extra/trunk/clangd/Threading.h
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
@@ -0,0 +1,176 @@
+//===-- TUSchedulerTests.cpp *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TUScheduler.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+using ::testing::Pair;
+using ::testing::Pointee;
+
+void ignoreUpdate(Context, llvm::Optional) {}
+void ignoreError(llvm::Error Err) {
+ handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
+}
+
+class TUSchedulerTests : public ::testing::Test {
+protected:
+ ParseInputs getInputs(PathRef File, std::string Contents) {
+return ParseInputs{*CDB.getCompileCommand(File), buildTestFS(Files),
+ std::move(Contents)};
+ }
+
+ void changeFile(PathRef File, std::string Contents) {
+Files[File] = Contents;
+ }
+
+private:
+ llvm::StringMap Files;
+ MockCompilationDatabase CDB;
+};
+
+TEST_F(TUSchedulerTests, MissingFiles) {
+ TUScheduler S(getDefaultAsyncThreadsCount(),
+/*StorePreamblesInMemory=*/true,
+/*ASTParsedCallback=*/nullptr);
+
+ auto Added = getVirtualTestFilePath("added.cpp");
+ changeFile(Added, "");
+
+ auto Missing = getVirtualTestFilePath("missing.cpp");
+ changeFile(Missing, "");
+
+ S.update(Context::empty(), Added, getInputs(Added, ""), ignoreUpdate);
+
+ // Assert each operation for missing file is an error (even if it's available
+ // in VFS).
+ S.runWithAST(Missing, [&](llvm::Expected AST) {
+ASSERT_FALSE(bool(AST));
+ignoreError(AST.takeError());
+ });
+ S.runWithPreamble(Missing, [&](llvm::Expected Preamble) {
+ASSERT_FALSE(bool(Preamble));
+ignoreError(Preamble.takeError());
+ });
+ S.remove(Missing, [&](llvm::Error Err) {
+EXPECT_TRUE(bool(Err));
+ignoreError(std::move(Err));
+ });
+
+ // Assert there aren't any errors for added file.
+ S.runWithAST(
+ Added, [&](llvm::Expected AST) { EXPECT_TRUE(bool(AST)); });
+ S.runWithPreamble(Added, [&](llvm::Expected Preamble) {
+EXPECT_TRUE(bool(Preamble));
+ });
+ S.remove(Added, [&](llvm::Error Err) { EXPECT_FALSE(bool(Err)); });
+
+ // Assert that all operations fail after removing the file.
+ S.runWithAST(Added, [&](llvm::Expected AST) {
+ASSERT_FALSE(bool(AST));
+ignoreError(AST.takeError());
+ });
+ S.runWithPreamble(Added, [&](llvm::Expected Preamble) {
+ASSERT_FALSE(bool(Preamble));
+ignoreError(Preamble.takeError());
+ });
+ S.remove(Added, [&](llvm::Error Err) {
+EXPECT_TRUE(bool(Err));
+ignoreError(std::move(Err));
+ });
+}
+
+TEST_F(TUSchedulerTests, ManyUpdates) {
+ const int FilesCount = 3;
+ const int UpdatesPerFile = 10;
+
+ std::mutex Mut;
+ int TotalASTReads = 0;
+ int TotalPreambleReads = 0;
+ int TotalUpdates = 0;
+
+ // Run TUScheduler and collect some stats.
+ {
+TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr);
+
+std::vector Files;
+for (int I = 0; I < FilesCount; ++I) {
+ Files.push_back(
+ getVirtualTestFilePath("foo" + std::to_string(I) + ".cpp").str());
+ changeFile(Files.back(), "");
+}
+
+llvm::StringRef Contents1 = R"cpp(int a;)cpp";
+llvm::StringRef Contents2 = R"cpp(int main() { return 1; })cpp";
+llvm::StringRef Contents3 =
+R"cpp(int a; int b; int sum() { return a + b; })cpp";
+
+llvm::StringRef AllContents[] = {Contents1, Contents2, Contents3};
+const int AllContentsSize = 3;
+
+for (int FileI = 0; FileI < FilesCount; ++FileI) {
+ for (int UpdateI = 0; UpdateI < UpdatesPerFile; ++UpdateI)
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.h:23 +/// synchronously). +unsigned getDefaultAsyncThreadsCount(); + sammccall wrote: > just use llvm::hardware_concurrency()? It can return 0, which will cause clangd to run synchronously. This function is only called when someone wants to have at least one worker thread for async processing. We can change it if you want, but I'd rather leave it as is in this patch. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov updated this revision to Diff 132107.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
Addressed last review comments:
- Rename blockingRead to blockingRun
- Added a comment to ThreadPool's destructor
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnitStore.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
clangd/Threading.cpp
clangd/Threading.h
unittests/clangd/CMakeLists.txt
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- /dev/null
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -0,0 +1,176 @@
+//===-- TUSchedulerTests.cpp *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TUScheduler.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+using ::testing::Pair;
+using ::testing::Pointee;
+
+void ignoreUpdate(Context, llvm::Optional) {}
+void ignoreError(llvm::Error Err) {
+ handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
+}
+
+class TUSchedulerTests : public ::testing::Test {
+protected:
+ ParseInputs getInputs(PathRef File, std::string Contents) {
+return ParseInputs{*CDB.getCompileCommand(File), buildTestFS(Files),
+ std::move(Contents)};
+ }
+
+ void changeFile(PathRef File, std::string Contents) {
+Files[File] = Contents;
+ }
+
+private:
+ llvm::StringMap Files;
+ MockCompilationDatabase CDB;
+};
+
+TEST_F(TUSchedulerTests, MissingFiles) {
+ TUScheduler S(getDefaultAsyncThreadsCount(),
+/*StorePreamblesInMemory=*/true,
+/*ASTParsedCallback=*/nullptr);
+
+ auto Added = getVirtualTestFilePath("added.cpp");
+ changeFile(Added, "");
+
+ auto Missing = getVirtualTestFilePath("missing.cpp");
+ changeFile(Missing, "");
+
+ S.update(Context::empty(), Added, getInputs(Added, ""), ignoreUpdate);
+
+ // Assert each operation for missing file is an error (even if it's available
+ // in VFS).
+ S.runWithAST(Missing, [&](llvm::Expected AST) {
+ASSERT_FALSE(bool(AST));
+ignoreError(AST.takeError());
+ });
+ S.runWithPreamble(Missing, [&](llvm::Expected Preamble) {
+ASSERT_FALSE(bool(Preamble));
+ignoreError(Preamble.takeError());
+ });
+ S.remove(Missing, [&](llvm::Error Err) {
+EXPECT_TRUE(bool(Err));
+ignoreError(std::move(Err));
+ });
+
+ // Assert there aren't any errors for added file.
+ S.runWithAST(
+ Added, [&](llvm::Expected AST) { EXPECT_TRUE(bool(AST)); });
+ S.runWithPreamble(Added, [&](llvm::Expected Preamble) {
+EXPECT_TRUE(bool(Preamble));
+ });
+ S.remove(Added, [&](llvm::Error Err) { EXPECT_FALSE(bool(Err)); });
+
+ // Assert that all operations fail after removing the file.
+ S.runWithAST(Added, [&](llvm::Expected AST) {
+ASSERT_FALSE(bool(AST));
+ignoreError(AST.takeError());
+ });
+ S.runWithPreamble(Added, [&](llvm::Expected Preamble) {
+ASSERT_FALSE(bool(Preamble));
+ignoreError(Preamble.takeError());
+ });
+ S.remove(Added, [&](llvm::Error Err) {
+EXPECT_TRUE(bool(Err));
+ignoreError(std::move(Err));
+ });
+}
+
+TEST_F(TUSchedulerTests, ManyUpdates) {
+ const int FilesCount = 3;
+ const int UpdatesPerFile = 10;
+
+ std::mutex Mut;
+ int TotalASTReads = 0;
+ int TotalPreambleReads = 0;
+ int TotalUpdates = 0;
+
+ // Run TUScheduler and collect some stats.
+ {
+TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr);
+
+std::vector Files;
+for (int I = 0; I < FilesCount; ++I) {
+ Files.push_back(
+ getVirtualTestFilePath("foo" + std::to_string(I) + ".cpp").str());
+ changeFile(Files.back(), "");
+}
+
+llvm::StringRef Contents1 = R"cpp(int a;)cpp";
+llvm::StringRef Contents2 = R"cpp(int main() { return 1; })cpp";
+llvm::StringRef Contents3 =
+R"cpp(int a; int b; int sum() { return a + b; })cpp";
+
+llvm::StringRef AllContents[] = {Contents1, Contents2, Contents3};
+const int AllContentsSize = 3;
+
+for (int FileI = 0; FileI < FilesCount; ++FileI) {
+ for (int UpdateI = 0; UpdateI < UpdatesPerFile; ++UpdateI) {
+auto Contents = AllContents[(FileI + UpdateI) % AllContentsSize];
+
+auto File = Files[FileI];
+auto Inputs = getInputs(File, Contents.str());
+static Key> FileAndUpdateKey;
+auto Ctx = Context::empty().derive(FileAndUpdateKey,
+
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Ship it!
Comment at: clangd/ClangdServer.cpp:37
+template
+Ret waitForASTAction(Scheduler , PathRef File, Func &) {
+ std::packaged_task Task(
sammccall wrote:
> Would be nice to have parallel names to the Scheduler methods, e.g.
> blockingASTRead() and blockingPreambleRead()
Nit: these names got out of sync again
Comment at: clangd/TUScheduler.h:23
+/// synchronously).
+unsigned getDefaultAsyncThreadsCount();
+
just use llvm::hardware_concurrency()?
Comment at: clangd/Threading.h:30
+ ThreadPool(unsigned AsyncThreadsCount);
+ ~ThreadPool();
+
add a comment to the destructor saying what it blocks on?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. All comments should be addressed now. Let me know if I missed anything else. Comment at: clangd/threading/TUScheduler.h:1 +//===--- TUScheduler.h ---*-C++-*-===// +// ilya-biryukov wrote: > sammccall wrote: > > this class needs tests > Will do :-( Added a simple test for it. Please take a look and let me know if you have more ideas on how we should test it. Comment at: clangd/threading/ThreadPool.h:1 +//===--- ThreadPool.h *- C++-*-===// +// ilya-biryukov wrote: > sammccall wrote: > > this class needs tests > Will do :-( We have `ClangdThreadingTest.StressTest` and `TUSchedulerTests` that both run concurrent operations on `ThreadPool`. As you pointed out, this should provide enough coverage for `ThreadPool`, so I didn't create any extra tests for it. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov updated this revision to Diff 132100.
ilya-biryukov added a comment.
- Properly ignore errors.
- Run all requests to completion when destroying ThreadPool.
- Added simple tests for TUScheduler.
- Fixed include guards.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnitStore.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
clangd/Threading.cpp
clangd/Threading.h
unittests/clangd/CMakeLists.txt
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- /dev/null
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -0,0 +1,176 @@
+//===-- TUSchedulerTests.cpp *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TUScheduler.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+using ::testing::Pair;
+using ::testing::Pointee;
+
+void ignoreUpdate(Context, llvm::Optional) {}
+void ignoreError(llvm::Error Err) {
+ handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
+}
+
+class TUSchedulerTests : public ::testing::Test {
+protected:
+ ParseInputs getInputs(PathRef File, std::string Contents) {
+return ParseInputs{*CDB.getCompileCommand(File), buildTestFS(Files),
+ std::move(Contents)};
+ }
+
+ void changeFile(PathRef File, std::string Contents) {
+Files[File] = Contents;
+ }
+
+private:
+ llvm::StringMap Files;
+ MockCompilationDatabase CDB;
+};
+
+TEST_F(TUSchedulerTests, MissingFiles) {
+ TUScheduler S(getDefaultAsyncThreadsCount(),
+/*StorePreamblesInMemory=*/true,
+/*ASTParsedCallback=*/nullptr);
+
+ auto Added = getVirtualTestFilePath("added.cpp");
+ changeFile(Added, "");
+
+ auto Missing = getVirtualTestFilePath("missing.cpp");
+ changeFile(Missing, "");
+
+ S.update(Context::empty(), Added, getInputs(Added, ""), ignoreUpdate);
+
+ // Assert each operation for missing file is an error (even if it's available
+ // in VFS).
+ S.runWithAST(Missing, [&](llvm::Expected AST) {
+ASSERT_FALSE(bool(AST));
+ignoreError(AST.takeError());
+ });
+ S.runWithPreamble(Missing, [&](llvm::Expected Preamble) {
+ASSERT_FALSE(bool(Preamble));
+ignoreError(Preamble.takeError());
+ });
+ S.remove(Missing, [&](llvm::Error Err) {
+EXPECT_TRUE(bool(Err));
+ignoreError(std::move(Err));
+ });
+
+ // Assert there aren't any errors for added file.
+ S.runWithAST(
+ Added, [&](llvm::Expected AST) { EXPECT_TRUE(bool(AST)); });
+ S.runWithPreamble(Added, [&](llvm::Expected Preamble) {
+EXPECT_TRUE(bool(Preamble));
+ });
+ S.remove(Added, [&](llvm::Error Err) { EXPECT_FALSE(bool(Err)); });
+
+ // Assert that all operations fail after removing the file.
+ S.runWithAST(Added, [&](llvm::Expected AST) {
+ASSERT_FALSE(bool(AST));
+ignoreError(AST.takeError());
+ });
+ S.runWithPreamble(Added, [&](llvm::Expected Preamble) {
+ASSERT_FALSE(bool(Preamble));
+ignoreError(Preamble.takeError());
+ });
+ S.remove(Added, [&](llvm::Error Err) {
+EXPECT_TRUE(bool(Err));
+ignoreError(std::move(Err));
+ });
+}
+
+TEST_F(TUSchedulerTests, ManyUpdates) {
+ const int FilesCount = 3;
+ const int UpdatesPerFile = 10;
+
+ std::mutex Mut;
+ int TotalASTReads = 0;
+ int TotalPreambleReads = 0;
+ int TotalUpdates = 0;
+
+ // Run TUScheduler and collect some stats.
+ {
+TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr);
+
+std::vector Files;
+for (int I = 0; I < FilesCount; ++I) {
+ Files.push_back(
+ getVirtualTestFilePath("foo" + std::to_string(I) + ".cpp").str());
+ changeFile(Files.back(), "");
+}
+
+llvm::StringRef Contents1 = R"cpp(int a;)cpp";
+llvm::StringRef Contents2 = R"cpp(int main() { return 1; })cpp";
+llvm::StringRef Contents3 =
+R"cpp(int a; int b; int sum() { return a + b; })cpp";
+
+llvm::StringRef AllContents[] = {Contents1, Contents2, Contents3};
+const int AllContentsSize = 3;
+
+for (int FileI = 0; FileI < FilesCount; ++FileI) {
+ for (int UpdateI = 0; UpdateI < UpdatesPerFile; ++UpdateI) {
+auto Contents = AllContents[(FileI + UpdateI) % AllContentsSize];
+
+auto File = Files[FileI];
+auto Inputs = getInputs(File, Contents.str());
+static Key> FileAndUpdateKey;
+auto Ctx = Context::empty().derive(FileAndUpdateKey,
+
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
sammccall added a comment. As discussed offline, basically the only thing to do for testing ThreadPool is to bash it with a workload, and some sort of whole-program stress test seems ideal for this and will also give some coverage to other components (and we should run under tsan!). TUScheduler on the other hand is a big important class with a clear interface and contract, and is about to get a new implementation - testcases verifying the contracts will be extremely valuable. These tests don't really need to be concurrency-heavy I think. Comment at: clangd/TUScheduler.h:10 + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_SCHEDULER_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_SCHEDULER_H header guards are stale, sorry! Comment at: clangd/Threading.h:10 + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_THREADPOOL_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_THREADPOOL_H this one also stale Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov updated this revision to Diff 131941.
ilya-biryukov added a comment.
- Remove threading/ dir, moved everything to the top-level
- Rename ThreadPool.h to Threading.h
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnitStore.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
clangd/Threading.cpp
clangd/Threading.h
Index: clangd/Threading.h
===
--- /dev/null
+++ clangd/Threading.h
@@ -0,0 +1,81 @@
+//===--- ThreadPool.h *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_THREADPOOL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_THREADPOOL_H
+
+#include "Function.h"
+#include
+#include
+#include
+#include
+#include
+
+namespace clang {
+namespace clangd {
+/// A simple fixed-size thread pool implementation.
+class ThreadPool {
+public:
+ /// 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.
+ ThreadPool(unsigned AsyncThreadsCount);
+ ~ThreadPool();
+
+ /// Add a new request to run function \p F with args \p As to the start of the
+ /// queue. The request will be run on a separate thread.
+ template
+ void addToFront(Func &, Args &&... As) {
+if (RunSynchronously) {
+ std::forward(F)(std::forward(As)...);
+ return;
+}
+
+{
+ std::lock_guard Lock(Mutex);
+ RequestQueue.push_front(
+ BindWithForward(std::forward(F), std::forward(As)...));
+}
+RequestCV.notify_one();
+ }
+
+ /// Add a new request to run function \p F with args \p As to the end of the
+ /// queue. The request will be run on a separate thread.
+ template void addToEnd(Func &, Args &&... As) {
+if (RunSynchronously) {
+ std::forward(F)(std::forward(As)...);
+ return;
+}
+
+{
+ std::lock_guard Lock(Mutex);
+ RequestQueue.push_back(
+ BindWithForward(std::forward(F), std::forward(As)...));
+}
+RequestCV.notify_one();
+ }
+
+private:
+ bool RunSynchronously;
+ mutable std::mutex Mutex;
+ /// We run some tasks on separate threads(parsing, CppFile cleanup).
+ /// These threads looks into RequestQueue to find requests to handle and
+ /// terminate when Done is set to true.
+ std::vector Workers;
+ /// Setting Done to true will make the worker threads terminate.
+ bool Done = false;
+ /// A queue of requests.
+ std::deque> RequestQueue;
+ /// Condition variable to wake up worker threads.
+ std::condition_variable RequestCV;
+};
+} // namespace clangd
+} // namespace clang
+#endif
Index: clangd/Threading.cpp
===
--- /dev/null
+++ clangd/Threading.cpp
@@ -0,0 +1,61 @@
+#include "Threading.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Threading.h"
+
+namespace clang {
+namespace clangd {
+ThreadPool::ThreadPool(unsigned AsyncThreadsCount)
+: RunSynchronously(AsyncThreadsCount == 0) {
+ if (RunSynchronously) {
+// Don't start the worker thread if we're running synchronously
+return;
+ }
+
+ Workers.reserve(AsyncThreadsCount);
+ for (unsigned I = 0; I < AsyncThreadsCount; ++I) {
+Workers.push_back(std::thread([this, I]() {
+ llvm::set_thread_name(llvm::formatv("scheduler/{0}", I));
+ while (true) {
+UniqueFunction Request;
+
+// Pick request from the queue
+{
+ std::unique_lock Lock(Mutex);
+ // Wait for more requests.
+ RequestCV.wait(Lock,
+ [this] { return !RequestQueue.empty() || Done; });
+ if (Done)
+return;
+
+ assert(!RequestQueue.empty() && "RequestQueue was empty");
+
+ // We process requests starting from the front of the queue. Users of
+ // ThreadPool have a way to prioritise their requests by putting
+ // them to the either side of the queue (using either addToEnd or
+ // addToFront).
+ Request = std::move(RequestQueue.front());
+ RequestQueue.pop_front();
+} // unlock Mutex
+
+Request();
+ }
+}));
+ }
+}
+
+ThreadPool::~ThreadPool() {
+ if (RunSynchronously)
+return; // no worker thread is running in that case
+
+ {
+std::lock_guard Lock(Mutex);
+// Wake up the worker thread
+Done = true;
+ } // unlock Mutex
+ RequestCV.notify_all();
+
+ for (auto : Workers)
+
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
sammccall added a comment. Can you please remove the `threading/` subdirectory? It seems premature for these two files, and `TUScheduler` doesn't fit. It's unclear that there will be more. I'd suggest renaming `Threadpool.h` -> `Threading.h`, CancellationFlag might fit in there, though up to you. Comment at: clangd/threading/TUScheduler.h:1 +//===--- TUScheduler.h ---*-C++-*-===// +// this class needs tests Comment at: clangd/threading/ThreadPool.h:1 +//===--- ThreadPool.h *- C++-*-===// +// this class needs tests Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov updated this revision to Diff 131926.
ilya-biryukov added a comment.
- Consume error in dumpAST
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnitStore.h
clangd/threading/TUScheduler.cpp
clangd/threading/TUScheduler.h
clangd/threading/ThreadPool.cpp
clangd/threading/ThreadPool.h
Index: clangd/threading/ThreadPool.h
===
--- /dev/null
+++ clangd/threading/ThreadPool.h
@@ -0,0 +1,81 @@
+//===--- ThreadPool.h *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_THREADPOOL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_THREADPOOL_H
+
+#include "Function.h"
+#include
+#include
+#include
+#include
+#include
+
+namespace clang {
+namespace clangd {
+/// A simple fixed-size thread pool implementation.
+class ThreadPool {
+public:
+ /// 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.
+ ThreadPool(unsigned AsyncThreadsCount);
+ ~ThreadPool();
+
+ /// Add a new request to run function \p F with args \p As to the start of the
+ /// queue. The request will be run on a separate thread.
+ template
+ void addToFront(Func &, Args &&... As) {
+if (RunSynchronously) {
+ std::forward(F)(std::forward(As)...);
+ return;
+}
+
+{
+ std::lock_guard Lock(Mutex);
+ RequestQueue.push_front(
+ BindWithForward(std::forward(F), std::forward(As)...));
+}
+RequestCV.notify_one();
+ }
+
+ /// Add a new request to run function \p F with args \p As to the end of the
+ /// queue. The request will be run on a separate thread.
+ template void addToEnd(Func &, Args &&... As) {
+if (RunSynchronously) {
+ std::forward(F)(std::forward(As)...);
+ return;
+}
+
+{
+ std::lock_guard Lock(Mutex);
+ RequestQueue.push_back(
+ BindWithForward(std::forward(F), std::forward(As)...));
+}
+RequestCV.notify_one();
+ }
+
+private:
+ bool RunSynchronously;
+ mutable std::mutex Mutex;
+ /// We run some tasks on separate threads(parsing, CppFile cleanup).
+ /// These threads looks into RequestQueue to find requests to handle and
+ /// terminate when Done is set to true.
+ std::vector Workers;
+ /// Setting Done to true will make the worker threads terminate.
+ bool Done = false;
+ /// A queue of requests.
+ std::deque> RequestQueue;
+ /// Condition variable to wake up worker threads.
+ std::condition_variable RequestCV;
+};
+} // namespace clangd
+} // namespace clang
+#endif
Index: clangd/threading/ThreadPool.cpp
===
--- /dev/null
+++ clangd/threading/ThreadPool.cpp
@@ -0,0 +1,61 @@
+#include "ThreadPool.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Threading.h"
+
+namespace clang {
+namespace clangd {
+ThreadPool::ThreadPool(unsigned AsyncThreadsCount)
+: RunSynchronously(AsyncThreadsCount == 0) {
+ if (RunSynchronously) {
+// Don't start the worker thread if we're running synchronously
+return;
+ }
+
+ Workers.reserve(AsyncThreadsCount);
+ for (unsigned I = 0; I < AsyncThreadsCount; ++I) {
+Workers.push_back(std::thread([this, I]() {
+ llvm::set_thread_name(llvm::formatv("scheduler/{0}", I));
+ while (true) {
+UniqueFunction Request;
+
+// Pick request from the queue
+{
+ std::unique_lock Lock(Mutex);
+ // Wait for more requests.
+ RequestCV.wait(Lock,
+ [this] { return !RequestQueue.empty() || Done; });
+ if (Done)
+return;
+
+ assert(!RequestQueue.empty() && "RequestQueue was empty");
+
+ // We process requests starting from the front of the queue. Users of
+ // ThreadPool have a way to prioritise their requests by putting
+ // them to the either side of the queue (using either addToEnd or
+ // addToFront).
+ Request = std::move(RequestQueue.front());
+ RequestQueue.pop_front();
+} // unlock Mutex
+
+Request();
+ }
+}));
+ }
+}
+
+ThreadPool::~ThreadPool() {
+ if (RunSynchronously)
+return; // no worker thread is running in that case
+
+ {
+std::lock_guard Lock(Mutex);
+// Wake up the worker thread
+Done = true;
+ } // unlock Mutex
+ RequestCV.notify_all();
+
+ for (auto :
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov updated this revision to Diff 131925.
ilya-biryukov added a comment.
- Renamed Scheduler to TUScheduler
- Use make_scope_exit instead of onScopeExit
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnitStore.h
clangd/threading/TUScheduler.cpp
clangd/threading/TUScheduler.h
clangd/threading/ThreadPool.cpp
clangd/threading/ThreadPool.h
Index: clangd/threading/ThreadPool.h
===
--- /dev/null
+++ clangd/threading/ThreadPool.h
@@ -0,0 +1,81 @@
+//===--- ThreadPool.h *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_THREADPOOL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_THREADPOOL_H
+
+#include "Function.h"
+#include
+#include
+#include
+#include
+#include
+
+namespace clang {
+namespace clangd {
+/// A simple fixed-size thread pool implementation.
+class ThreadPool {
+public:
+ /// 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.
+ ThreadPool(unsigned AsyncThreadsCount);
+ ~ThreadPool();
+
+ /// Add a new request to run function \p F with args \p As to the start of the
+ /// queue. The request will be run on a separate thread.
+ template
+ void addToFront(Func &, Args &&... As) {
+if (RunSynchronously) {
+ std::forward(F)(std::forward(As)...);
+ return;
+}
+
+{
+ std::lock_guard Lock(Mutex);
+ RequestQueue.push_front(
+ BindWithForward(std::forward(F), std::forward(As)...));
+}
+RequestCV.notify_one();
+ }
+
+ /// Add a new request to run function \p F with args \p As to the end of the
+ /// queue. The request will be run on a separate thread.
+ template void addToEnd(Func &, Args &&... As) {
+if (RunSynchronously) {
+ std::forward(F)(std::forward(As)...);
+ return;
+}
+
+{
+ std::lock_guard Lock(Mutex);
+ RequestQueue.push_back(
+ BindWithForward(std::forward(F), std::forward(As)...));
+}
+RequestCV.notify_one();
+ }
+
+private:
+ bool RunSynchronously;
+ mutable std::mutex Mutex;
+ /// We run some tasks on separate threads(parsing, CppFile cleanup).
+ /// These threads looks into RequestQueue to find requests to handle and
+ /// terminate when Done is set to true.
+ std::vector Workers;
+ /// Setting Done to true will make the worker threads terminate.
+ bool Done = false;
+ /// A queue of requests.
+ std::deque> RequestQueue;
+ /// Condition variable to wake up worker threads.
+ std::condition_variable RequestCV;
+};
+} // namespace clangd
+} // namespace clang
+#endif
Index: clangd/threading/ThreadPool.cpp
===
--- /dev/null
+++ clangd/threading/ThreadPool.cpp
@@ -0,0 +1,61 @@
+#include "ThreadPool.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Threading.h"
+
+namespace clang {
+namespace clangd {
+ThreadPool::ThreadPool(unsigned AsyncThreadsCount)
+: RunSynchronously(AsyncThreadsCount == 0) {
+ if (RunSynchronously) {
+// Don't start the worker thread if we're running synchronously
+return;
+ }
+
+ Workers.reserve(AsyncThreadsCount);
+ for (unsigned I = 0; I < AsyncThreadsCount; ++I) {
+Workers.push_back(std::thread([this, I]() {
+ llvm::set_thread_name(llvm::formatv("scheduler/{0}", I));
+ while (true) {
+UniqueFunction Request;
+
+// Pick request from the queue
+{
+ std::unique_lock Lock(Mutex);
+ // Wait for more requests.
+ RequestCV.wait(Lock,
+ [this] { return !RequestQueue.empty() || Done; });
+ if (Done)
+return;
+
+ assert(!RequestQueue.empty() && "RequestQueue was empty");
+
+ // We process requests starting from the front of the queue. Users of
+ // ThreadPool have a way to prioritise their requests by putting
+ // them to the either side of the queue (using either addToEnd or
+ // addToFront).
+ Request = std::move(RequestQueue.front());
+ RequestQueue.pop_front();
+} // unlock Mutex
+
+Request();
+ }
+}));
+ }
+}
+
+ThreadPool::~ThreadPool() {
+ if (RunSynchronously)
+return; // no worker thread is running in that case
+
+ {
+std::lock_guard Lock(Mutex);
+// Wake up the worker thread
+Done = true;
+ } // unlock
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
sammccall added a subscriber: hokein.
sammccall added inline comments.
Comment at: clangd/ClangdServer.h:177
+/// preambles and ASTs) for opened files.
+class Scheduler {
+public:
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > sammccall wrote:
> > > > sammccall wrote:
> > > > > This class has important responsibilities beyond threading itself,
> > > > > which "Scheduler" suggests.
> > > > >
> > > > > I can't think of a perfectly coherent name, options that seem
> > > > > reasonable:
> > > > > - TUManager - pretty bland/vague, but gets what this class is mostly
> > > > > about
> > > > > - Workshop - kind of a silly metaphor, but unlikely to be confused
> > > > > with something else
> > > > > - Clearinghouse - another silly metaphor, maybe more accurate but
> > > > > more obscure
> > > > Worth saying something abouth the threading properties here:
> > > >
> > > > - Scheduler is not threadsafe, only the main thread should be providing
> > > > updates and scheduling tasks.
> > > > - callbacks are run on a large threadpool, and it's appropriate to do
> > > > slow, blocking work in them
> > > Added comments.
> > It'd be nice to have some mention of the fact that the class handles
> > threading responsibilities. None of the options seem to capture this.
> > I don't have good suggestions either, though.
> >
> Rename of the Scheduler seems to be the only thing blocking this patch from
> landing.
> I'm happy to go with either of the suggested alternatives or leave as is, I
> couldn't come up with anything better.
>
> @sammccall, what option would you prefer?
For a descriptive name I think we need something TU-related in there. If we
want it to cover threads too, I can't think of anything better than
`TUScheduler`.
I'm also happy with `Workshop` or similar, which prods you to work out what the
class actually does. Maybe other people will find this confusing. Will solicit
opinions.
...
Chatted with @ioeric and @hokein - Workshop is too silly. Suggestions were
along the lines of what we've been discussing: TUTaskManager or similar.
So I'd probably go with TUScheduler, it's not horrendously long and it's close
enough to Scheduler that those of us who spent last week discussing it won't
get confused :-)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:246
+
+ ParseInputs Inputs = getInputs(File);
+ std::shared_ptr Preamble =
sammccall wrote:
> I think you want to take a reference here, and then capture by value
Makes sense, less copies. Thanks.
Comment at: clangd/ClangdServer.cpp:400
+ CallbackType Callback,
+ llvm::Expected InpPreamble) {
+assert(InpPreamble &&
sammccall wrote:
> nit: "Inp" in InpPreamble is pretty awkward. In this context, Read might
> work? or IP.
Used IP
Comment at: clangd/ClangdServer.cpp:403
+ "error when trying to read preamble for codeComplete");
+auto Preamble = InpPreamble->Preamble;
+auto = InpPreamble->Inputs.CompileCommand;
sammccall wrote:
> InpPreamble->Preamble->Preamble is pretty hard to understand. Can we find
> better names for these things? Or at least unpack it on one line?
It's now IP->PreambleData->Preamble. Not great, but should make more sense.
Comment at: clangd/ClangdServer.h:164
+/// Handles running tasks for ClangdServer and managing the resources (e.g.,
+/// preambles and ASTs) for opened files.
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > This is a nice abstraction, so much better than dealing with Cppfile! A
> > > couple of observations:
> > >
> > > 1) all methods refer to a single file, neither the contracts nor the
> > > implementation have any interactions between files. An API that reflects
> > > this seems more natural. e.g. it naturally provides operations like "are
> > > we tracking this file", and makes it natural to be able to e.g. lock at
> > > the per-file level. e.g.
> > >
> > > class WorkingSet {
> > > shared_ptr get(Path);
> > > shared_ptr getOrCreate(Path)
> > >}
> > >class TranslationUnit {
> > > void update(ParseInputs);
> > > void read(Callback);
> > >}
> > >
> > > This seems like it would make it easier to explain what the semantics of
> > > scheduleRemove are, too.
> > >
> > > 2) The callbacks from individual methods seem more powerful than needed,
> > > and encourage deeper coupling. Basically, the inputs (changes) and
> > > outputs (diagnostics, reads) don't seem to want to interact with the same
> > > code. This suggests decoupling them: the changes are a sequence of input
> > > events, diagnostics are a sequence of output events, reads look much as
> > > they do now.
> > >
> > > One benefit here is that the versioning relationship between inputs and
> > > outputs no longer show up in the function signatures (by coupling an
> > > input to a matching output). Expressing them as data makes it easier to
> > > tweak them.
> > >
> > > 3) It's not spelled out how this interacts with drafts: whether
> > > text<->version is maintained here or externally, and what the contracts
> > > around versions are. There are no options offered, so I would guess that
> > > scheduleUpdate delivers new-version-or-nothing, and scheduleASTRead
> > > delivers... current-or-newer? current-or-nothing?
> > >
> > > I think it would *probably* be clearer to have versions minted by the
> > > external DraftStore, that way we can decouple "we know about the contents
> > > of this file" from "we're building this file". e.g. we probably want
> > > wildly different policies for discarding resources of old versions, when
> > > "resources" = source code vs "resources" = ASTs and preambles.
> > >
> > > 4) Scheduler (or anything it decomposes into) is important and isolated
> > > enough that it deserves its own header.
> > 1. Given the nature of the LSP, the users of the interface will probably
> > always call only a single function of `TranslationUnit`, so we won't win
> > much in terms of the code clarity.
> > ```
> > scheduleUpdate(File, Inputs) --> get(File).update(Inputs)
> > ```
> > That adds some complexity to the interface, though. I'd opt for not doing
> > that in the initial version.
> >
> > 2. One place where I find these callbacks useful are tests where we could
> > wait for latest `addDocument` to complete. A more pressing concern is that
> > the interface of `ClangdServer` does not allow to remove the callbacks
> > (`addDocument` returns a future) and I would really love to keep the public
> > interface of `ClangdServer` the same for the first iteration.
> >
> > 3. I would err on the side of saying that `scheduleASTRead` delivers
> > current version. This gives correct results for the current callers of the
> > interface. If we do current-or-newer I'd start with adding the versions to
> > the interface, so that the callers of the API would have enough context to
> > know whether the results correspond to the latest version or not. I'd
> > really love to keep version out of the initial patch, happy to chat about
> > them for follow-up
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov updated this revision to Diff 131565.
ilya-biryukov marked 14 inline comments as done.
ilya-biryukov added a comment.
Herald added subscribers: hintonda, mgorny.
- Adressed review comments.
- Move threading code to separate files.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnitStore.h
clangd/threading/Scheduler.cpp
clangd/threading/Scheduler.h
clangd/threading/ThreadPool.cpp
clangd/threading/ThreadPool.h
Index: clangd/threading/ThreadPool.h
===
--- /dev/null
+++ clangd/threading/ThreadPool.h
@@ -0,0 +1,81 @@
+//===--- ThreadPool.h *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_THREADPOOL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_THREADING_THREADPOOL_H
+
+#include "Function.h"
+#include
+#include
+#include
+#include
+#include
+
+namespace clang {
+namespace clangd {
+/// A simple fixed-size thread pool implementation.
+class ThreadPool {
+public:
+ /// 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.
+ ThreadPool(unsigned AsyncThreadsCount);
+ ~ThreadPool();
+
+ /// Add a new request to run function \p F with args \p As to the start of the
+ /// queue. The request will be run on a separate thread.
+ template
+ void addToFront(Func &, Args &&... As) {
+if (RunSynchronously) {
+ std::forward(F)(std::forward(As)...);
+ return;
+}
+
+{
+ std::lock_guard Lock(Mutex);
+ RequestQueue.push_front(
+ BindWithForward(std::forward(F), std::forward(As)...));
+}
+RequestCV.notify_one();
+ }
+
+ /// Add a new request to run function \p F with args \p As to the end of the
+ /// queue. The request will be run on a separate thread.
+ template void addToEnd(Func &, Args &&... As) {
+if (RunSynchronously) {
+ std::forward(F)(std::forward(As)...);
+ return;
+}
+
+{
+ std::lock_guard Lock(Mutex);
+ RequestQueue.push_back(
+ BindWithForward(std::forward(F), std::forward(As)...));
+}
+RequestCV.notify_one();
+ }
+
+private:
+ bool RunSynchronously;
+ mutable std::mutex Mutex;
+ /// We run some tasks on separate threads(parsing, CppFile cleanup).
+ /// These threads looks into RequestQueue to find requests to handle and
+ /// terminate when Done is set to true.
+ std::vector Workers;
+ /// Setting Done to true will make the worker threads terminate.
+ bool Done = false;
+ /// A queue of requests.
+ std::deque> RequestQueue;
+ /// Condition variable to wake up worker threads.
+ std::condition_variable RequestCV;
+};
+} // namespace clangd
+} // namespace clang
+#endif
Index: clangd/threading/ThreadPool.cpp
===
--- /dev/null
+++ clangd/threading/ThreadPool.cpp
@@ -0,0 +1,61 @@
+#include "ThreadPool.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Threading.h"
+
+namespace clang {
+namespace clangd {
+ThreadPool::ThreadPool(unsigned AsyncThreadsCount)
+: RunSynchronously(AsyncThreadsCount == 0) {
+ if (RunSynchronously) {
+// Don't start the worker thread if we're running synchronously
+return;
+ }
+
+ Workers.reserve(AsyncThreadsCount);
+ for (unsigned I = 0; I < AsyncThreadsCount; ++I) {
+Workers.push_back(std::thread([this, I]() {
+ llvm::set_thread_name(llvm::formatv("scheduler/{0}", I));
+ while (true) {
+UniqueFunction Request;
+
+// Pick request from the queue
+{
+ std::unique_lock Lock(Mutex);
+ // Wait for more requests.
+ RequestCV.wait(Lock,
+ [this] { return !RequestQueue.empty() || Done; });
+ if (Done)
+return;
+
+ assert(!RequestQueue.empty() && "RequestQueue was empty");
+
+ // We process requests starting from the front of the queue. Users of
+ // ThreadPool have a way to prioritise their requests by putting
+ // them to the either side of the queue (using either addToEnd or
+ // addToFront).
+ Request = std::move(RequestQueue.front());
+ RequestQueue.pop_front();
+} // unlock Mutex
+
+Request();
+ }
+}));
+ }
+}
+
+ThreadPool::~ThreadPool() {
+ if (RunSynchronously)
+return; // no worker thread is running in that case
+
+ {
+std::lock_guard
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
sammccall added a comment.
Agreed we can defer lots of stuff in order to keep this patch compact.
Generally the things I think we can get right before landing:
- names and file organization for new things
- documentation including where we want to get to
Comment at: clangd/ClangdServer.cpp:246
+
+ ParseInputs Inputs = getInputs(File);
+ std::shared_ptr Preamble =
I think you want to take a reference here, and then capture by value
Comment at: clangd/ClangdServer.cpp:323
+ [](Context Ctx, std::promise DonePromise, llvm::Error Err) {
+// FIXME(ibiryukov): allow to report error to the caller.
+// Discard error, if any.
this fixme doesn't make sense if we're removing the callback
Comment at: clangd/ClangdServer.cpp:345
+ return scheduleReparseAndDiags(std::move(Ctx), File, std::move(FileContents),
+ TaggedFS);
}
nit: you dropped a move here
Comment at: clangd/ClangdServer.cpp:400
+ CallbackType Callback,
+ llvm::Expected InpPreamble) {
+assert(InpPreamble &&
nit: "Inp" in InpPreamble is pretty awkward. In this context, Read might work?
or IP.
Comment at: clangd/ClangdServer.cpp:403
+ "error when trying to read preamble for codeComplete");
+auto Preamble = InpPreamble->Preamble;
+auto = InpPreamble->Inputs.CompileCommand;
InpPreamble->Preamble->Preamble is pretty hard to understand. Can we find
better names for these things? Or at least unpack it on one line?
Comment at: clangd/ClangdServer.h:164
+/// Handles running tasks for ClangdServer and managing the resources (e.g.,
+/// preambles and ASTs) for opened files.
ilya-biryukov wrote:
> sammccall wrote:
> > This is a nice abstraction, so much better than dealing with Cppfile! A
> > couple of observations:
> >
> > 1) all methods refer to a single file, neither the contracts nor the
> > implementation have any interactions between files. An API that reflects
> > this seems more natural. e.g. it naturally provides operations like "are we
> > tracking this file", and makes it natural to be able to e.g. lock at the
> > per-file level. e.g.
> >
> > class WorkingSet {
> > shared_ptr get(Path);
> > shared_ptr getOrCreate(Path)
> >}
> >class TranslationUnit {
> > void update(ParseInputs);
> > void read(Callback);
> >}
> >
> > This seems like it would make it easier to explain what the semantics of
> > scheduleRemove are, too.
> >
> > 2) The callbacks from individual methods seem more powerful than needed,
> > and encourage deeper coupling. Basically, the inputs (changes) and outputs
> > (diagnostics, reads) don't seem to want to interact with the same code.
> > This suggests decoupling them: the changes are a sequence of input events,
> > diagnostics are a sequence of output events, reads look much as they do now.
> >
> > One benefit here is that the versioning relationship between inputs and
> > outputs no longer show up in the function signatures (by coupling an input
> > to a matching output). Expressing them as data makes it easier to tweak
> > them.
> >
> > 3) It's not spelled out how this interacts with drafts: whether
> > text<->version is maintained here or externally, and what the contracts
> > around versions are. There are no options offered, so I would guess that
> > scheduleUpdate delivers new-version-or-nothing, and scheduleASTRead
> > delivers... current-or-newer? current-or-nothing?
> >
> > I think it would *probably* be clearer to have versions minted by the
> > external DraftStore, that way we can decouple "we know about the contents
> > of this file" from "we're building this file". e.g. we probably want wildly
> > different policies for discarding resources of old versions, when
> > "resources" = source code vs "resources" = ASTs and preambles.
> >
> > 4) Scheduler (or anything it decomposes into) is important and isolated
> > enough that it deserves its own header.
> 1. Given the nature of the LSP, the users of the interface will probably
> always call only a single function of `TranslationUnit`, so we won't win much
> in terms of the code clarity.
> ```
> scheduleUpdate(File, Inputs) --> get(File).update(Inputs)
> ```
> That adds some complexity to the interface, though. I'd opt for not doing
> that in the initial version.
>
> 2. One place where I find these callbacks useful are tests where we could
> wait for latest `addDocument` to complete. A more pressing concern is that
> the interface of `ClangdServer` does not allow to remove the callbacks
> (`addDocument` returns a future) and I would really love to keep the public
> interface of `ClangdServer` the same for the first iteration.
>
> 3. I would err on the side of saying that
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.h:164
+/// Handles running tasks for ClangdServer and managing the resources (e.g.,
+/// preambles and ASTs) for opened files.
sammccall wrote:
> This is a nice abstraction, so much better than dealing with Cppfile! A
> couple of observations:
>
> 1) all methods refer to a single file, neither the contracts nor the
> implementation have any interactions between files. An API that reflects this
> seems more natural. e.g. it naturally provides operations like "are we
> tracking this file", and makes it natural to be able to e.g. lock at the
> per-file level. e.g.
>
> class WorkingSet {
> shared_ptr get(Path);
> shared_ptr getOrCreate(Path)
>}
>class TranslationUnit {
> void update(ParseInputs);
> void read(Callback);
>}
>
> This seems like it would make it easier to explain what the semantics of
> scheduleRemove are, too.
>
> 2) The callbacks from individual methods seem more powerful than needed, and
> encourage deeper coupling. Basically, the inputs (changes) and outputs
> (diagnostics, reads) don't seem to want to interact with the same code. This
> suggests decoupling them: the changes are a sequence of input events,
> diagnostics are a sequence of output events, reads look much as they do now.
>
> One benefit here is that the versioning relationship between inputs and
> outputs no longer show up in the function signatures (by coupling an input to
> a matching output). Expressing them as data makes it easier to tweak them.
>
> 3) It's not spelled out how this interacts with drafts: whether
> text<->version is maintained here or externally, and what the contracts
> around versions are. There are no options offered, so I would guess that
> scheduleUpdate delivers new-version-or-nothing, and scheduleASTRead
> delivers... current-or-newer? current-or-nothing?
>
> I think it would *probably* be clearer to have versions minted by the
> external DraftStore, that way we can decouple "we know about the contents of
> this file" from "we're building this file". e.g. we probably want wildly
> different policies for discarding resources of old versions, when "resources"
> = source code vs "resources" = ASTs and preambles.
>
> 4) Scheduler (or anything it decomposes into) is important and isolated
> enough that it deserves its own header.
1. Given the nature of the LSP, the users of the interface will probably always
call only a single function of `TranslationUnit`, so we won't win much in terms
of the code clarity.
```
scheduleUpdate(File, Inputs) --> get(File).update(Inputs)
```
That adds some complexity to the interface, though. I'd opt for not doing that
in the initial version.
2. One place where I find these callbacks useful are tests where we could wait
for latest `addDocument` to complete. A more pressing concern is that the
interface of `ClangdServer` does not allow to remove the callbacks
(`addDocument` returns a future) and I would really love to keep the public
interface of `ClangdServer` the same for the first iteration.
3. I would err on the side of saying that `scheduleASTRead` delivers current
version. This gives correct results for the current callers of the interface.
If we do current-or-newer I'd start with adding the versions to the interface,
so that the callers of the API would have enough context to know whether the
results correspond to the latest version or not. I'd really love to keep
version out of the initial patch, happy to chat about them for follow-up
patches.
4. Totally agree. I planned to move it into a separate header after the
changes land. Happy to do that with a follow-up patch.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:222
+ }
+ // We currently do all the reads of the AST on a running thread to avoid
+ // inconsistent states coming from subsequent updates.
sammccall wrote:
> Having trouble with this one.
> is this the same as "We currently block the calling thread until the AST is
> available, to avoid..."?
Yes. Used your wording, thanks.
Comment at: clangd/ClangdServer.h:169
+ Scheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
+std::shared_ptr PCHs,
+ASTParsedCallback ASTCallback);
sammccall wrote:
> this is a performance optimization, right?
> I think the scheduler does enough of the compiling that giving it one of its
> own isn't too wasteful.
It is probably redundant, yes. Removed it.
Comment at: clangd/ClangdServer.h:173
+ /// Get the CompileCommand passed at the last scheduleUpdate call for \p
File.
+ llvm::Optional getLatestCommand(PathRef File) const;
+
sammccall wrote:
> This seems like a smell - the command is sometimes an input from callsites
> that want builds, and sometimes an output to callsites that want builds. Also
> this class is all about threading, scheduling, and versions, and this method
> bypasses all of that.
>
> (If this is a hack to keep code complete working while we refactor, that's
> fine - but the docs should say that and we should know what next steps look
> like!)
>
> ISTM the issue here is coupling updates to the source with updates to the
> compile command.
> Ultimately we should indeed be compiling a given version with a fixed CC, but
> that doesn't seem like the clearest interface for callers.
>
> One option is:
> - Scheduler has a reference to the CDB (or a std::function wrapping it),
> and needs to come up with commands itself
> - it caches commands whenever it can, and has an "invalidateCommand(Path)"
> to drop its cache. `with_file_lock { invalidateCommand(), scheduleUpdate() }`
> is the equivalent to `forceRebuild`.
> - it provides a `scheduleSourceRead` which is like scheduleASTRead but
> provides source, compile command, and the latest preamble available without
> blocking. This would be used for operations like codecomplete that can't use
> scheduleASTRead.
I've opted for passing action inputs to the read methods, as discussed offline.
The implementation turned out to be ugly, we now store a separate
`StringMap` in `Scheduler` to keep track of the latest inputs and
not put that into `CppFile`.
Comment at: clangd/ClangdServer.h:184
+ /// resources. \p Action will be called when resources are freed.
+ /// If an error occurs during processing, it is forwarded to the \p Action
+ /// callback.
sammccall wrote:
> The callback here is a bit confusing and YAGNI (both clangd and our known
> embedders discard the returned future).
> It seems enough to return synchronously and know that subsequent reads that
> ask for = or >= version are going to get nothing back.
I'm fine with removing the callback, but I'd like this change to focus on
internals of `ClangdServer` without changing its interface, happy to remove the
callback afterwards.
Added a FIXME for that.
Comment at: clangd/ClangdServer.h:200
+ /// callback.
+ void schedulePreambleRead(
+ PathRef File,
sammccall wrote:
> can we pass in a consistency policy here? even if we only support a subset of
> {ast, preamble} x policy for now, a richer API would enable
> experiments/tradeoffs later. e.g.
>
> enum class VersionConstraint {
> ANY, // returns the first data that's available, typically immediately
> EQUAL, // returns the data for the previous update
> GREATER_EQUAL, // Returns the data for the previous update, or some
> subsequent update.
> }
>
> (I guess the semantics you've described for AST read are EQUAL, and ANY for
> preamble read)
I would rather not extend the API beyond what it can do at the moment, but
happy to explore this approach after threading gets rid of the legacy stuff
that we have now.
Comment at: clangd/ClangdServer.h:206
+ CppFileCollection Units;
+ SimpleThreadPool Executor;
+};
sammccall wrote:
> these two names seem a bit confusing - might be easier as just CppFiles,
> Threads?
>
> The names seem to be ~synonyms for the types, which I don't think is better
> than echoing the types. (Often there's a completely separate good name, but I
> don't think so here)
- I'd go with `Files` instead of `CppFiles`. WDYT?
- `Executor` sounds better to me than `Threads`. Could you elaborate why you
find it confusing?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov updated this revision to Diff 131105.
ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added a comment.
- Renamed SimpleThreadPool to ThreadPool
- Removed PCHs from Scheduler's constructor
- Renamed waitFor(AST|Preamble)Action to blocking(AST|Preamble)Read
- Updates
- Remove getLastCommand from Scheduler's interface, pass Inputs to actions
instead
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnitStore.h
Index: clangd/ClangdUnitStore.h
===
--- clangd/ClangdUnitStore.h
+++ clangd/ClangdUnitStore.h
@@ -27,26 +27,26 @@
public:
/// \p ASTCallback is called when a file is parsed synchronously. This should
/// not be expensive since it blocks diagnostics.
- explicit CppFileCollection(ASTParsedCallback ASTCallback)
- : ASTCallback(std::move(ASTCallback)) {}
+ explicit CppFileCollection(bool StorePreamblesInMemory,
+ std::shared_ptr PCHs,
+ ASTParsedCallback ASTCallback)
+ : ASTCallback(std::move(ASTCallback)), PCHs(std::move(PCHs)),
+StorePreamblesInMemory(StorePreamblesInMemory) {}
- std::shared_ptr
- getOrCreateFile(PathRef File, bool StorePreamblesInMemory,
- std::shared_ptr PCHs) {
+ std::shared_ptr getOrCreateFile(PathRef File) {
std::lock_guard Lock(Mutex);
auto It = OpenedFiles.find(File);
if (It == OpenedFiles.end()) {
It = OpenedFiles
.try_emplace(File, CppFile::Create(File, StorePreamblesInMemory,
- std::move(PCHs), ASTCallback))
+ PCHs, ASTCallback))
.first;
}
return It->second;
}
- std::shared_ptr getFile(PathRef File) {
+ std::shared_ptr getFile(PathRef File) const {
std::lock_guard Lock(Mutex);
-
auto It = OpenedFiles.find(File);
if (It == OpenedFiles.end())
return nullptr;
@@ -58,9 +58,11 @@
std::shared_ptr removeIfPresent(PathRef File);
private:
- std::mutex Mutex;
+ mutable std::mutex Mutex;
llvm::StringMap OpenedFiles;
ASTParsedCallback ASTCallback;
+ std::shared_ptr PCHs;
+ bool StorePreamblesInMemory;
};
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -98,21 +98,20 @@
class ClangdServer;
-/// Returns a number of a default async threads to use for ClangdScheduler.
+/// Returns a number of a default async threads to use for Scheduler.
/// 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 {
+/// A simple fixed-size thread pool implementation.
+class ThreadPool {
public:
/// 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();
+ ThreadPool(unsigned AsyncThreadsCount);
+ ~ThreadPool();
/// Add a new request to run function \p F with args \p As to the start of the
/// queue. The request will be run on a separate thread.
@@ -149,20 +148,77 @@
private:
bool RunSynchronously;
- std::mutex Mutex;
+ mutable std::mutex Mutex;
/// We run some tasks on separate threads(parsing, CppFile cleanup).
/// These threads looks into RequestQueue to find requests to handle and
/// terminate when Done is set to true.
std::vector Workers;
/// Setting Done to true will make the worker threads terminate.
bool Done = false;
- /// A queue of requests. Elements of this vector are async computations (i.e.
- /// results of calling std::async(std::launch::deferred, ...)).
+ /// A queue of requests.
std::deque> RequestQueue;
/// Condition variable to wake up worker threads.
std::condition_variable RequestCV;
};
+
+struct InputsAndAST {
+ const ParseInputs
+ ParsedAST
+};
+
+struct InputsAndPreamble {
+ const ParseInputs
+ const PreambleData* Preamble;
+};
+
+/// Handles running tasks for ClangdServer and managing the resources (e.g.,
+/// preambles and ASTs) for opened files.
+class Scheduler {
+public:
+ Scheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
+ASTParsedCallback ASTCallback);
+
+ /// Schedule an update for \p File. Adds \p File to a list of tracked files if
+ /// \p File was not part of it before.
+ void scheduleUpdate(
+ Context Ctx, PathRef File, ParseInputs Inputs,
+
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
sammccall added a comment.
Herald added a subscriber: ioeric.
So, I simultaneously think this is basically ready to land, and I want
substantial changes :-)
This is much better already than what we have, and where I think we can further
improve the design, this is a natural point on the way.
My preferred next steps would be:
- we discuss a little bit the directions I've outlined below, without the
assumption that they belong in this patch.
- to the extent you agree, you make the less-invasive changes here (e.g. adding
a VersionConstraint API, but only actually supporting the cases you've
implemented)
- once we're on the same page for the other stuff, I'm happy to pick up any
amount of it myself - whatever's not going to step on your toes
Comment at: clangd/ClangdServer.cpp:37
+template
+Ret waitForASTAction(Scheduler , PathRef File, Func &) {
+ std::packaged_task Task(
Would be nice to have parallel names to the Scheduler methods, e.g.
blockingASTRead() and blockingPreambleRead()
Comment at: clangd/ClangdServer.cpp:222
+ }
+ // We currently do all the reads of the AST on a running thread to avoid
+ // inconsistent states coming from subsequent updates.
Having trouble with this one.
is this the same as "We currently block the calling thread until the AST is
available, to avoid..."?
Comment at: clangd/ClangdServer.h:164
+/// Handles running tasks for ClangdServer and managing the resources (e.g.,
+/// preambles and ASTs) for opened files.
This is a nice abstraction, so much better than dealing with Cppfile! A couple
of observations:
1) all methods refer to a single file, neither the contracts nor the
implementation have any interactions between files. An API that reflects this
seems more natural. e.g. it naturally provides operations like "are we tracking
this file", and makes it natural to be able to e.g. lock at the per-file level.
e.g.
class WorkingSet {
shared_ptr get(Path);
shared_ptr getOrCreate(Path)
}
class TranslationUnit {
void update(ParseInputs);
void read(Callback);
}
This seems like it would make it easier to explain what the semantics of
scheduleRemove are, too.
2) The callbacks from individual methods seem more powerful than needed, and
encourage deeper coupling. Basically, the inputs (changes) and outputs
(diagnostics, reads) don't seem to want to interact with the same code. This
suggests decoupling them: the changes are a sequence of input events,
diagnostics are a sequence of output events, reads look much as they do now.
One benefit here is that the versioning relationship between inputs and outputs
no longer show up in the function signatures (by coupling an input to a
matching output). Expressing them as data makes it easier to tweak them.
3) It's not spelled out how this interacts with drafts: whether text<->version
is maintained here or externally, and what the contracts around versions are.
There are no options offered, so I would guess that scheduleUpdate delivers
new-version-or-nothing, and scheduleASTRead delivers... current-or-newer?
current-or-nothing?
I think it would *probably* be clearer to have versions minted by the external
DraftStore, that way we can decouple "we know about the contents of this file"
from "we're building this file". e.g. we probably want wildly different
policies for discarding resources of old versions, when "resources" = source
code vs "resources" = ASTs and preambles.
4) Scheduler (or anything it decomposes into) is important and isolated enough
that it deserves its own header.
Comment at: clangd/ClangdServer.h:169
+ Scheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
+std::shared_ptr PCHs,
+ASTParsedCallback ASTCallback);
this is a performance optimization, right?
I think the scheduler does enough of the compiling that giving it one of its
own isn't too wasteful.
Comment at: clangd/ClangdServer.h:173
+ /// Get the CompileCommand passed at the last scheduleUpdate call for \p
File.
+ llvm::Optional getLatestCommand(PathRef File) const;
+
This seems like a smell - the command is sometimes an input from callsites that
want builds, and sometimes an output to callsites that want builds. Also this
class is all about threading, scheduling, and versions, and this method
bypasses all of that.
(If this is a hack to keep code complete working while we refactor, that's fine
- but the docs should say that and we should know what next steps look like!)
ISTM the issue here is coupling updates to the source with updates to the
compile command.
Ultimately we should indeed be compiling a given version with a fixed CC, but
that doesn't seem like the clearest interface for callers.
One option is:
- Scheduler has a reference to the CDB (or a
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.h:107
+/// A simple fixed-size thread pool implementation.
+class SimpleThreadPool {
public:
bkramer wrote:
> What's so simple about it? Why not `clangd::ThreadPool`?
>
> Also there's `llvm::ThreadPool`, what's the difference between them?
Will rename it to `ThreadPool`.
Differences are:
- `llvm::ThreadPool` always process requests in FIFO order, we allow LIFO here
(for code completion).
- `llvm::ThreadPool` will run tasks synchronously when `LLVM_ENABLE_THREADS` is
set to `0`. I'm not sure that makes sense for clangd, which has a runtime
switch for that (`-run-synchronously` flag)
- `llvm::ThreadPool` will not process any tasks when `ThreadsCount` is set to
`0`, our implementation processes the tasks synchronously instead.
I'll also be adding per-unit queues in the latter commit (aka thread affinity)
to our thead pool, so it'll have more differences. I suggest waiting a day or
two before I send the patch for review.
Another minor difference is:
- `llvm::ThreadPool` creates a `std::packaged_task` and `std::future` for each
task, our implementation simply runs the provided actions. The latter means
less book-keeping and is more efficient, but I don't think it matters.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
bkramer added inline comments.
Comment at: clangd/ClangdServer.h:107
+/// A simple fixed-size thread pool implementation.
+class SimpleThreadPool {
public:
What's so simple about it? Why not `clangd::ThreadPool`?
Also there's `llvm::ThreadPool`, what's the difference between them?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42174: [clangd] Refactored threading in ClangdServer
ilya-biryukov created this revision.
ilya-biryukov added reviewers: sammccall, bkramer.
Herald added a subscriber: klimek.
We now provide an abstraction of Scheduler that abstracts threading
and resource management in ClangdServer.
No changes to behavior are intended with an exception of changed error
messages.
This patch is preliminary work to allow a revamped threading
implementation that will move the threading code out of CppFile.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.h
clangd/ClangdUnitStore.h
Index: clangd/ClangdUnitStore.h
===
--- clangd/ClangdUnitStore.h
+++ clangd/ClangdUnitStore.h
@@ -27,27 +27,26 @@
public:
/// \p ASTCallback is called when a file is parsed synchronously. This should
/// not be expensive since it blocks diagnostics.
- explicit CppFileCollection(ASTParsedCallback ASTCallback)
- : ASTCallback(std::move(ASTCallback)) {}
+ explicit CppFileCollection(bool StorePreamblesInMemory,
+ std::shared_ptr PCHs,
+ ASTParsedCallback ASTCallback)
+ : ASTCallback(std::move(ASTCallback)), PCHs(std::move(PCHs)),
+StorePreamblesInMemory(StorePreamblesInMemory) {}
- std::shared_ptr
- getOrCreateFile(PathRef File, PathRef ResourceDir,
- bool StorePreamblesInMemory,
- std::shared_ptr PCHs) {
+ std::shared_ptr getOrCreateFile(PathRef File) {
std::lock_guard Lock(Mutex);
auto It = OpenedFiles.find(File);
if (It == OpenedFiles.end()) {
It = OpenedFiles
.try_emplace(File, CppFile::Create(File, StorePreamblesInMemory,
- std::move(PCHs), ASTCallback))
+ PCHs, ASTCallback))
.first;
}
return It->second;
}
- std::shared_ptr getFile(PathRef File) {
+ std::shared_ptr getFile(PathRef File) const {
std::lock_guard Lock(Mutex);
-
auto It = OpenedFiles.find(File);
if (It == OpenedFiles.end())
return nullptr;
@@ -59,9 +58,11 @@
std::shared_ptr removeIfPresent(PathRef File);
private:
- std::mutex Mutex;
+ mutable std::mutex Mutex;
llvm::StringMap OpenedFiles;
ASTParsedCallback ASTCallback;
+ std::shared_ptr PCHs;
+ bool StorePreamblesInMemory;
};
} // namespace clangd
} // namespace clang
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -64,11 +64,8 @@
ParseInputs(tooling::CompileCommand CompileCommand,
IntrusiveRefCntPtr FS, std::string Contents);
- /// Compilation arguments.
tooling::CompileCommand CompileCommand;
- /// FileSystem.
IntrusiveRefCntPtr FS;
- /// Contents to parse.
std::string Contents;
};
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -98,21 +98,20 @@
class ClangdServer;
-/// Returns a number of a default async threads to use for ClangdScheduler.
+/// Returns a number of a default async threads to use for Scheduler.
/// 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 {
+/// A simple fixed-size thread pool implementation.
+class SimpleThreadPool {
public:
/// 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();
+ SimpleThreadPool(unsigned AsyncThreadsCount);
+ ~SimpleThreadPool();
/// Add a new request to run function \p F with args \p As to the start of the
/// queue. The request will be run on a separate thread.
@@ -149,20 +148,64 @@
private:
bool RunSynchronously;
- std::mutex Mutex;
+ mutable std::mutex Mutex;
/// We run some tasks on separate threads(parsing, CppFile cleanup).
/// These threads looks into RequestQueue to find requests to handle and
/// terminate when Done is set to true.
std::vector Workers;
/// Setting Done to true will make the worker threads terminate.
bool Done = false;
- /// A queue of requests. Elements of this vector are async computations (i.e.
- /// results of calling std::async(std::launch::deferred, ...)).
+ /// A queue of requests.
std::deque> RequestQueue;
/// Condition variable to wake up worker threads.
std::condition_variable RequestCV;
};
+/// Handles running tasks for ClangdServer and
