This revision was automatically updated to reflect the committed changes.
tberghammer marked 4 inline comments as done.
Closed by commit rL250820: Add a new task pool class to LLDB (authored by
tberghammer).
Changed prior to commit:
http://reviews.llvm.org/D13727?vs=37566=37865#toc
tfiala added a comment.
That seems totally reasonable. We're fine with "this isn't an issue until it's
proven to be an issue."
(And I like stressing the concurrency - we initially found a number of issues
when the concurrent test runner came online in the first place).
clayborg added a comment.
Yes, lets get this in an iterate on it afterwards!
http://reviews.llvm.org/D13727
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
tberghammer updated this revision to Diff 37566.
tberghammer added a comment.
Create optional std::async based implementation
http://reviews.llvm.org/D13727
Files:
include/lldb/Utility/TaskPool.h
source/Utility/CMakeLists.txt
source/Utility/TaskPool.cpp
unittests/Utility/CMakeLists.txt
labath added a comment.
Zachary, I don't think using std::async is a good idea because it provides a
very different threading model than the one we want here. Let me demonstrate
that with an example:
#include
#include
#include
#include
#include
using namespace std;
zturner added a comment.
Sorry, should have mentioned that the lgtm is contingent on Todd's concerns. I
like the idea of having a setting in LLDB that controls the maximum number of
threads to use for parallel work. We should probably set this at a very h igh
level in dotest so that all
tfiala added a comment.
In http://reviews.llvm.org/D13727#268096, @clayborg wrote:
> In http://reviews.llvm.org/D13727#268091, @tberghammer wrote:
>
> > I agree that parking the threads in the kernel should be very cheap (I am
> > not sure about Windows) but on high end multi core machines
clayborg added a comment.
Stopping the threads is fine as it seems to be desired from most others.
http://reviews.llvm.org/D13727
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
tberghammer added a comment.
In http://reviews.llvm.org/D13727#268233, @zturner wrote:
> In http://reviews.llvm.org/D13727#268086, @clayborg wrote:
>
> > > Yes. But an implementation of std::async is either going to use a global
> > > thread pool or it's not. If it does there's no problem,
zturner added a comment.
In http://reviews.llvm.org/D13727#268086, @clayborg wrote:
> > Yes. But an implementation of std::async is either going to use a global
> > thread pool or it's not. If it does there's no problem, and if it doesn't,
> > it's going to create one thread for each call,
clayborg added a comment.
In http://reviews.llvm.org/D13727#268053, @zturner wrote:
> The main thought I had was just that it would make the code simpler and
> delegate more of the synchronization stuff to the library. But if we don't
> get any of that benefit then I guess there's no point in
clayborg added a comment.
> Yes. But an implementation of std::async is either going to use a global
> thread pool or it's not. If it does there's no problem, and if it doesn't,
> it's going to create one thread for each call, and when the work in that
> thread is done, the thread will exit.
tberghammer updated this revision to Diff 37498.
tberghammer added a comment.
Addressing comments from the discussion with destroying the treads not in use
while keeping a global thread pool with at most hardware_concurrency threads.
IMO this update also simplifies the implementation of the
tberghammer updated this revision to Diff 37492.
tberghammer marked 7 inline comments as done.
tberghammer added a comment.
- Address review comments
- Add some more documentation
- Add some unit tests
http://reviews.llvm.org/D13727
Files:
include/lldb/Utility/TaskPool.h
clayborg accepted this revision.
clayborg added a comment.
Very nice! I look forward to seeing this patch and the associated DWARF patches
make it in!
http://reviews.llvm.org/D13727
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
tberghammer added a comment.
Zach: I thought about your idea and I think it won't make the code any simpler
because your suggestion would only implement the functionality what is
currently implemented by TaskPool and TaskPoolImpl what is using only a single
condition variable and a single
zturner added a comment.
The main thought I had was just that it would make the code simpler and
delegate more of the synchronization stuff to the library. But if we don't get
any of that benefit then I guess there's no point in doing that.
That said, I really dont' want to see 48 threads
tfiala added a subscriber: tfiala.
tfiala added a comment.
In http://reviews.llvm.org/D13727#268053, @zturner wrote:
> The main thought I had was just that it would make the code simpler and
> delegate more of the synchronization stuff to the library. But if we don't
> get any of that benefit
zturner added a comment.
In http://reviews.llvm.org/D13727#268066, @clayborg wrote:
> In http://reviews.llvm.org/D13727#268053, @zturner wrote:
>
> > The main thought I had was just that it would make the code simpler and
> > delegate more of the synchronization stuff to the library. But if we
This may or may not be relevant, but the stack size for std::thread on
OS X is a fixed size of 512KB (DEFAULT_STACK_SIZE in pthread.c).
There's no way to specify a stack size with std::thread and no
per-process override like I think you can do with rlimit on Linux.
It's an issue I ran into when
clayborg added a comment.
In http://reviews.llvm.org/D13727#268091, @tberghammer wrote:
> I agree that parking the threads in the kernel should be very cheap (I am not
> sure about Windows) but on high end multi core machines having 40+ threads
> around just to wait for work is a bit strange
tberghammer created this revision.
tberghammer added reviewers: labath, clayborg, vharron, zturner.
tberghammer added a subscriber: lldb-commits.
Herald added a subscriber: iancottrell.
Add a new task pool class to LLDB to make it easy to execute tasks in parallel
Basic design goals:
* Have a
zturner added a comment.
I thought about this some more and I'm fine with going with a single
implementation and not using std async. It would be nice to take advantage of
any deep optimizations std::async provides over a hand-rolled solution, but at
the same time there's a cost to adding
zturner added a comment.
Can this be done in such a way that everything boils down to a single call to
std::async on platforms that support thread limiting?
Alternatively, why not just put a semaphore inside of the lambda that you run
with std::async to limit the number of threads? This seems
tberghammer added a comment.
We can change this class to use std::async on Windows and an std::thread based
implementation on Linux with the same interface (other OS-es should be decided)
but I would prefer to use just 1 logic as it is easier to maintain.
Limiting the number of threads with
clayborg added a comment.
Zach: If these are implementation details, lets get this in first and then
worry about optimizations later.
http://reviews.llvm.org/D13727
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
zturner added a comment.
Well, it's not just an optimization. Threading code is hard to reason about,
and the more complicated an implementation the more likely it is to have race
conditions or other problems. So any opportunity to reduce the amount of
manual thread management is a win in
zturner added a comment.
Ok, it seems reasonable to just use std::async on platforms that this is ok on,
and not use it on platforms which it's not ok on. I think it should be enabled
on Windows, but I'll leave it up to you to decide whether it's a whitelist or a
blacklist.
28 matches
Mail list logo
