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&id=37865#toc
Reposito
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
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).
http://reviews.llvm.or
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
I think this can go in now. Currently, the paralelization happens on compile
unit level, and the test programs generally only have one compile unit. Since
the threads are created on an as-need
tberghammer added a comment.
I thought a bit about the points raised by Todd and in general I am happy with
overloading the system during testing with a very high number of tests as it
will most likely help us to detect race conditions and based on my testing (on
a 40 core Linux machine) it isn
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 insta
zturner accepted this revision.
zturner added a comment.
Your example makes sense. So go ahead and delete the std async implementation
for now. As Tamas said, its very easy to put in, so a better thing to do
perhaps is once there's something doing actual parallel work, just test it in a
real
tfiala added a comment.
I'm in agreement with Pavel's assessment on this.
Regarding the tests, I had brought that up with Greg earlier. We can handle
that in a few different ways:
- We do nothing, and perhaps some collections of tests run slower if they are
all pounding on their thread pools
labath added a comment.
In http://reviews.llvm.org/D13727#268747, @tberghammer wrote:
> Based on Pavel's example and some additional experimenting we done I am not
> sure if std::async will give us any benefit even on Windows as it don't limit
> the number of threads to the number of cores (bec
tberghammer added a comment.
Based on Pavel's example and some additional experimenting we done I am not
sure if std::async will give us any benefit even on Windows as it don't limit
the number of threads to the number of cores (because it can't do if it want to
implement the standard). I have
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;
u
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, and
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, an
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 havin
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 imp
tberghammer added a comment.
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 for me too and it definitely
makes debugging of LLDB more difficult
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 fo
clayborg added a comment.
In http://reviews.llvm.org/D13727#268084, @tberghammer wrote:
> Addressing comments from the discussion with destroying the treads not in use
> while keeping a global thread pool with at most hardware_concurrency threads.
We should perf test this, but I would be OK wi
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 Thre
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.
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
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
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
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
http://l
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 runn
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 queue
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
source/Utility/CMak
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 tha
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
http://lists.llvm.org/cgi
zturner added a comment.
The only real suggestion / question I have is a design one.
By using this implementation we can't take advantage of the system thread pool.
That was the point of using std async in the first place, but we found that it
doesn't always limit the number of threads. Maybe
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 comp
clayborg added a comment.
Any std::async adoption will need to be able to deliver tasks as they complete
via "TaskRunner::WaitForNextCompletedTask()".
We had a previous example where 1000 items could be pushed onto a std::vector
of tasks and then the code was doing:
for (i=0; ihttp://reviews
clayborg requested changes to this revision.
clayborg added a comment.
I agree with labath's comments and see if we can move TaskPoolImpl into the
.cpp file.
Comment at: include/lldb/Utility/TaskPool.h:39-66
@@ +38,30 @@
+
+class TaskPoolImpl
+{
+public:
+Ta
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.
http://reviews.ll
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 add
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
labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.
Although it may not seem from the number of my comments, I actually quite like
this implementation. However, I think more work needs to be done to make this
future-proof.
- we defin
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 ver
40 matches
Mail list logo
