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

[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

[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

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

[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

[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

[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!).

[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

[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

[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

[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

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

[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

[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

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

[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

[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

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

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

[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

[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

[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