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:
clan
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
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://revi
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 &S, PathRef File, Func &&F) {
+ std::packaged_task)> Task(
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 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/D
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!).
TUSchedule
ilya-biryukov added inline comments.
Comment at: clangd/threading/TUScheduler.h:1
+//===--- TUScheduler.h ---*-C++-*-===//
+//
sammccall wrote:
> this class needs tests
Will do :-(
Comment at: clangd/thre
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/Clangd
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 yo
ilya-biryukov added a comment.
Should be ready now. Will land as soon as the review is accepted.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-
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
clang
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
cla
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:
> > > sammccal
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.h:177
+/// preambles and ASTs) for opened files.
+class Scheduler {
+public:
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > This class has import
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.
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://revie
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/Cla
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 tha
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
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
- Re
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.
M
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::T
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 diff
ilya-biryukov added a comment.
Here's also a combined diff from both this and child revision:
https://reviews.llvm.org/D42177
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42174
___
cfe-commits mailing list
cfe-commits@lists.llvm.
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 e
26 matches
Mail list logo
