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 &FileWithContents : 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 <iostream> #include <memory> #include <string> +#include <thread> using namespace clang; using namespace clang::clangd; -static llvm::cl::opt<bool> - RunSynchronously("run-synchronously", - llvm::cl::desc("Parse on main thread"), - llvm::cl::init(false), llvm::cl::Hidden); +static llvm::cl::opt<unsigned> + WorkerThreadsCount("j", + llvm::cl::desc("Number of async workers used by clangd"), + llvm::cl::init(getDefaultAsyncThreadsCount())); + +static llvm::cl::opt<bool> 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<std::string> 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 &Outs = llvm::outs(); llvm::raw_ostream &Logs = llvm::errs(); JSONOutput Out(Outs, Logs); @@ -43,6 +60,7 @@ llvm::Optional<StringRef> ResourceDirRef = None; if (!ResourceDir.empty()) ResourceDirRef = ResourceDir; - ClangdLSPServer LSPServer(Out, RunSynchronously, ResourceDirRef); + + ClangdLSPServer LSPServer(Out, WorkerThreadsCount, ResourceDirRef); LSPServer.run(std::cin); } Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ 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 to run function \p F with args \p As to the start of the @@ -146,41 +155,37 @@ private: bool RunSynchronously; 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 - /// terminates when Done is set to true. - std::thread Worker; - /// Setting Done to true will make the worker thread terminate. + /// 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<std::thread> Workers; + /// Setting Done to true will make the worker threads terminate. bool Done = false; - /// A queue of requests. - /// FIXME(krasimir): code completion should always have priority over parsing - /// for diagnostics. + /// A queue of requests. Elements of this vector are async computations (i.e. + /// results of calling std::async(std::launch::deferred, ...)). std::deque<std::future<void>> RequestQueue; - /// Condition variable to wake up the worker thread. + /// Condition variable to wake up worker threads. std::condition_variable RequestCV; }; /// Provides API to manage ASTs for a collection of C++ files and request /// various language features(currently, only codeCompletion and async /// diagnostics for tracked files). class ClangdServer { public: - /// Creates a new ClangdServer. If \p RunSynchronously is false, no worker - /// thread will be created and all requests will be completed synchronously on - /// the calling thread (this is mostly used for tests). If \p RunSynchronously - /// is true, a worker thread will be created to parse files in the background - /// and provide diagnostics results via DiagConsumer.onDiagnosticsReady - /// callback. File accesses for each instance of parsing will be conducted via - /// a vfs::FileSystem provided by \p FSProvider. Results of code - /// completion/diagnostics also include a tag, that \p FSProvider returns - /// along with the vfs::FileSystem. - /// When \p ResourceDir is set, it will be used to search for internal headers + /// Creates a new ClangdServer. To server parsing requests ClangdScheduler, + /// that spawns \p AsyncThreadsCount worker threads will be created (when \p + /// AsyncThreadsCount is 0, requests will be processed on the calling thread). + /// instance of parsing will be conducted via a vfs::FileSystem provided by \p + /// FSProvider. Results of code completion/diagnostics also include a tag, + /// that \p FSProvider returns along with the vfs::FileSystem. When \p + /// ResourceDir is set, it will be used to search for internal headers /// (overriding defaults and -resource-dir compiler flag, if set). If \p /// ResourceDir is None, ClangdServer will attempt to set it to a standard /// location, obtained via CompilerInvocation::GetResourcePath. ClangdServer(GlobalCompilationDatabase &CDB, DiagnosticsConsumer &DiagConsumer, - FileSystemProvider &FSProvider, bool RunSynchronously, + FileSystemProvider &FSProvider, unsigned AsyncThreadsCount, llvm::Optional<StringRef> ResourceDir = llvm::None); /// Add a \p File to the list of tracked C++ files or update the contents if Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -78,40 +78,52 @@ return make_tagged(vfs::getRealFileSystem(), VFSTag()); } -ClangdScheduler::ClangdScheduler(bool RunSynchronously) - : RunSynchronously(RunSynchronously) { +unsigned clangd::getDefaultAsyncThreadsCount() { + 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; +} + +ClangdScheduler::ClangdScheduler(unsigned AsyncThreadsCount) + : RunSynchronously(AsyncThreadsCount == 0) { if (RunSynchronously) { // Don't start the worker thread if we're running synchronously return; } - // Initialize Worker in ctor body, rather than init list to avoid potentially - // using not-yet-initialized members - Worker = std::thread([this]() { - while (true) { - std::future<void> Request; - - // Pick request from the queue - { - std::unique_lock<std::mutex> 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 - // ClangdScheduler 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.get(); - } - }); + Workers.reserve(AsyncThreadsCount); + for (unsigned I = 0; I < AsyncThreadsCount; ++I) { + Workers.push_back(std::thread([this]() { + while (true) { + std::future<void> Request; + + // Pick request from the queue + { + std::unique_lock<std::mutex> 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 + // ClangdScheduler 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.get(); + } + })); + } } ClangdScheduler::~ClangdScheduler() { @@ -123,19 +135,21 @@ // Wake up the worker thread Done = true; } // unlock Mutex - RequestCV.notify_one(); - Worker.join(); + RequestCV.notify_all(); + + for (auto &Worker : Workers) + Worker.join(); } ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB, DiagnosticsConsumer &DiagConsumer, FileSystemProvider &FSProvider, - bool RunSynchronously, + unsigned AsyncThreadsCount, llvm::Optional<StringRef> ResourceDir) : CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider), ResourceDir(ResourceDir ? ResourceDir->str() : getStandardResourceDir()), PCHs(std::make_shared<PCHContainerOperations>()), - WorkScheduler(RunSynchronously) {} + WorkScheduler(AsyncThreadsCount) {} std::future<void> ClangdServer::addDocument(PathRef File, StringRef Contents) { DocVersion Version = DraftMgr.updateDraft(File, Contents); Index: clangd/ClangdLSPServer.h =================================================================== --- clangd/ClangdLSPServer.h +++ clangd/ClangdLSPServer.h @@ -26,7 +26,7 @@ /// dispatch and ClangdServer together. class ClangdLSPServer { public: - ClangdLSPServer(JSONOutput &Out, bool RunSynchronously, + ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount, llvm::Optional<StringRef> ResourceDir); /// Run LSP server loop, receiving input for it from \p In. \p In must be Index: clangd/ClangdLSPServer.cpp =================================================================== --- clangd/ClangdLSPServer.cpp +++ clangd/ClangdLSPServer.cpp @@ -220,10 +220,10 @@ R"(,"result":[)" + Locations + R"(]})"); } -ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, bool RunSynchronously, +ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount, llvm::Optional<StringRef> ResourceDir) : Out(Out), DiagConsumer(*this), - Server(CDB, DiagConsumer, FSProvider, RunSynchronously, ResourceDir) {} + Server(CDB, DiagConsumer, FSProvider, AsyncThreadsCount, ResourceDir) {} void ClangdLSPServer::run(std::istream &In) { assert(!IsDone && "Run was called before");
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits