Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

If the patch is busted, pretty much every test should start failing. As long as ninja check-lldb actually runs, completes, and behaves pretty much the same way as before, that's pretty much all that needs to be verified. Ctrl+C shouldn't behave any differently with this patch, the idea was to

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

zturner added a comment. If the patch is busted, pretty much every test should start failing. As long as ninja check-lldb actually runs, completes, and behaves pretty much the same way as before, that's pretty much all that needs to be verified. Ctrl+C shouldn't behave any differently with this

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

Cool, lgtm as well. Sorry for the holdup On Tue, Sep 8, 2015 at 1:58 PM Adrian McCarthy wrote: > amccarth added a comment. > > It seems to be general flakiness. In three runs without the patch, I got > the lower numbers every time. In three runs with the patch, I got

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

zturner added a comment. Cool, lgtm as well. Sorry for the holdup http://reviews.llvm.org/D12651 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

zturner added a comment. Can you confirm with 2 runs before and 2 runs after the patch that you see the same results every time? Todd, can you think of a reason why this might happen? I don't think it's worth holding the patch up too long over this because I want to see the other improvements

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala added a comment. In http://reviews.llvm.org/D12651#241810, @zturner wrote: > Can you confirm with 2 runs before and 2 runs after the patch that you see > the same results every time? > > Todd, can you think of a reason why this might happen? Hmm, I vaguely recall finding what looked

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

Adrian, can you verify this in the morning? Basically just trying to ensure that ninja check-lldb still works as it did before. There's a chance I'm going to be OOO tomorrow (or at the very best late) due to something unexpected. On Mon, Sep 7, 2015 at 9:38 PM Todd Fiala

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

zturner added a comment. Adrian, can you verify this in the morning? Basically just trying to ensure that ninja check-lldb still works as it did before. There's a chance I'm going to be OOO tomorrow (or at the very best late) due to something unexpected. http://reviews.llvm.org/D12651

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala added a comment. @zturner, at this point you should be able to run this and see no change on Windows (assuming I did the os check correctly). The Windows test runner is set to be the previous multithreading-pool strategy. For everyone else, they'll get the multithreading strategy by

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala updated this revision to Diff 34186. tfiala added a comment. Adds "threading" test-runner strategy, which mirrors the "multithreading" runner in terms of supporting Ctrl-C and hand-rolling the worker model. Like multiprocessing over multiprocessing-pool, threading outperforms

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala added a comment. That last patch also fixes a regression in the previous patch when --threads 1 was specified --- the serial test runner in dosep.py was busted and I missed that. http://reviews.llvm.org/D12651 ___ lldb-commits mailing list

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala added a subscriber: dawn. tfiala updated this revision to Diff 34112. tfiala added a comment. The latest patch does the following: - adds a test runner strategy layer for the isolated, concurrent test support (in dosep.py). The currently-supported test runners are: 1. multiprocessing

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala added a comment. In http://reviews.llvm.org/D12651#240633, @zturner wrote: > I won't be able to test this until Tuesday, but thanks for working on it No worries. Have a great weekend! (And maybe I'll have a working Windows setup before then). http://reviews.llvm.org/D12651

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

I won't be able to test this until Tuesday, but thanks for working on it On Sat, Sep 5, 2015 at 12:39 PM Todd Fiala wrote: > tfiala added a comment. > > That last patch also fixes a regression in the previous patch when > --threads 1 was specified --- the serial test

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

zturner added a comment. I won't be able to test this until Tuesday, but thanks for working on it http://reviews.llvm.org/D12651 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala updated this revision to Diff 34114. tfiala added a comment. This change adds the threading-module-based parallel test runner strategy. It is not selected by default. It is pool-based. The threading-pool and multiprocessing-pool strategies are 19.8% slower than the newer hand-wrapped

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

That'll also let me set it up so Greg can poke around with the threading version on OS X. On Fri, Sep 4, 2015 at 10:04 PM, Todd Fiala wrote: > Yep, I'm thinking that's right. > > On Fri, Sep 4, 2015 at 10:02 PM, Zachary Turner > wrote: > >> The

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

On Fri, Sep 4, 2015 at 5:40 PM, Zachary Turner wrote: > > > On Fri, Sep 4, 2015 at 5:10 PM Todd Fiala wrote: > >> tfiala added a comment. >> >> In http://reviews.llvm.org/D12651#240480, @zturner wrote: >> >> > Tried out this patch, unfortunately I'm

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

The pluggable method would at least allow everyone to continue working until someone has time to dig into what's wrong with multiprocess on Windows On Fri, Sep 4, 2015 at 9:56 PM Todd Fiala wrote: > On Fri, Sep 4, 2015 at 5:40 PM, Zachary Turner wrote:

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

Correct, it doesn't do that prior to the patch. It looks like it's never exiting this loop: try: for worker in workers: worker.join() except: either when a Ctrl+C happens, or when all the processes finish. I guess it's stuck in there for some reason.

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala updated this revision to Diff 34089. tfiala added a comment. Specify tight queue sizes for the job description queue and the job results queue. This *might* stop unintentional queue blocking when adding items to the queue if they were sized too small by default. Might be a possible

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

Tried out this patch, unfortunately I'm seeing the same thing. The very first call to worker.join() is never returning. It's unfortunate that it's so hard to debug this stuff, do you have any suggestions for how I can try to nail down what the child dotest instance is actually doing? I wonder

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala added a comment. In http://reviews.llvm.org/D12651#240480, @zturner wrote: > Tried out this patch, unfortunately I'm seeing the same thing. The very > first call to worker.join() is never returning. > > It's unfortunate that it's so hard to debug this stuff, do you have any >

[Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala created this revision. tfiala added reviewers: clayborg, zturner. tfiala added a subscriber: lldb-commits. This change does the following: * If Ctrl-C is hit once, the parallel dotest.py runner will stop any further work from being started, but will wait for the existing tests in

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala added a comment. In http://reviews.llvm.org/D12651#240439, @zturner wrote: > Hmm, yea the problem is worse than I thought. Even if I don't do Ctrl+C at > all (I just run the test suite like I always have and let it finish) it's > just leaving a bunch of stale processes at the end

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

clayborg added a comment. I believe that python is also weird in that if you want to catch a CTRL+C it has to be executing python code (not in a python code the entered C/C++ code) otherwise the CTRL+C might not get to the python itself. This might be only a problem in the embedded python

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

clayborg added a comment. Were you running dosep.py on Windows before as the way to run your test suite? Or were you running dotest.py (no multi-threading)? http://reviews.llvm.org/D12651 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala added a comment. In http://reviews.llvm.org/D12651#240443, @zturner wrote: > Correct, it doesn't do that prior to the patch. It looks like it's never > exiting this loop: > > try: > for worker in workers: > worker.join() > except: > > > either when a Ctrl+C

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks fine. Zach, do you need to make some modifications for Windows? Or should we just check this in and you can fix windows with a separate patch? http://reviews.llvm.org/D12651

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala added a comment. In http://reviews.llvm.org/D12651#240423, @zturner wrote: > Ctrl+C once doesn't work on Windows with this patch. It seems to continue > starting up new processes, and after all of them are done, I'm left with the > original set of Python processes not doing any work,

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

tfiala added a comment. In http://reviews.llvm.org/D12651#240429, @zturner wrote: > I can try to put a little time into figuring out what's wrong. I'd rather > it not go in broken though, so let me take a look first and we can figure > out what to do after that. Yep absolutely.

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

Hmm, yea the problem is worse than I thought. Even if I don't do Ctrl+C at all (I just run the test suite like I always have and let it finish) it's just leaving a bunch of stale processes at the end without them shutting down on their own. On Fri, Sep 4, 2015 at 3:39 PM Todd Fiala

Re: [Lldb-commits] [PATCH] D12651: Add ctrl-c support to parallel dotest.py.

On Fri, Sep 4, 2015 at 5:10 PM Todd Fiala wrote: > tfiala added a comment. > > In http://reviews.llvm.org/D12651#240480, @zturner wrote: > > > Tried out this patch, unfortunately I'm seeing the same thing. The very > > first call to worker.join() is never returning. > > >