Bug 54079 - nrwt multiprocessing: move non-testing logic out of single_test_runner, create worker_mixin
Summary: nrwt multiprocessing: move non-testing logic out of single_test_runner, creat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 49566 54082
  Show dependency treegraph
 
Reported: 2011-02-09 01:29 PST by Dirk Pranke
Modified: 2011-02-14 14:56 PST (History)
5 users (show)

See Also:


Attachments
Patch (23.83 KB, patch)
2011-02-09 01:34 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
merge in changes from 54070, 54071, 54072, 54074, feedback from mihaip (25.73 KB, patch)
2011-02-11 18:33 PST, Dirk Pranke
mihai: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-02-09 01:29:46 PST
nrwt multiprocessing: move non-testing logic out of single_test_runner, create worker_mixin
Comment 1 Dirk Pranke 2011-02-09 01:34:59 PST
Created attachment 81771 [details]
Patch
Comment 2 Dirk Pranke 2011-02-09 01:52:19 PST
This patch is actually simpler than it appears. You can ignore all of the diffs in single_test_runner.py, as it is simply being rolled back to what it was as of r77076. 

Ito-san, I've moved all of the non-testing logic back out of single_test_runner and into a new class called worker_mixin that will be shared between TestShellThread and Worker. It contains all of the worker logic that isn't specific to actually running the test and producing the outputs (which is in single_test_runner) and isn't specific to the concurrency/messaging model (which will either be in TestShellThread or Worker).
Comment 3 Hayato Ito 2011-02-09 02:26:30 PST
Hi Dirk,

Thank you for your very quick and great effort to roll back single_test_runner.

I was thinking how to move concurrency code back to TestShellThread in dump_render_tree.py.
I was not aware that Work should share same code. So the roll back doesn't seem to be an easy task than I thought.

I am not sure whether coding convention of WebKit allows multiple inheritance, mix-in, or not. If it is okay, the patch looks good to me.


(In reply to comment #2)
> This patch is actually simpler than it appears. You can ignore all of the diffs in single_test_runner.py, as it is simply being rolled back to what it was as of r77076. 
> 
> Ito-san, I've moved all of the non-testing logic back out of single_test_runner and into a new class called worker_mixin that will be shared between TestShellThread and Worker. It contains all of the worker logic that isn't specific to actually running the test and producing the outputs (which is in single_test_runner) and isn't specific to the concurrency/messaging model (which will either be in TestShellThread or Worker).
Comment 4 Mihai Parparita 2011-02-11 11:51:48 PST
Comment on attachment 81771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81771&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:31
> +import os

Not seeing any usage of is in this file.

> Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:50
> +    def deferred_init(self, port):

The naming of this confused me; what is supposed to be deferred about it?

> Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:57
> +        self._has_http_lock = False

Seems strange that you have to maintain your own flag instead of having it in the port, which actually acquires the lock. Should this be moved there?
Comment 5 Dirk Pranke 2011-02-11 13:07:48 PST
(In reply to comment #4)
> (From update of attachment 81771 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81771&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:31
> > +import os
> 
> Not seeing any usage of is in this file.
> 

That's probably a leftover. I'll delete it.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:50
> > +    def deferred_init(self, port):
> 
> The naming of this confused me; what is supposed to be deferred about it?
>

It isn't called during the constructor, when most initialization is done (and can't be, because the constructor is called pre-fork). It is called during the run() method, when it's actually in the right process. I thought I had added a comment, but I guess not. I'll add one.
  
> > Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:57
> > +        self._has_http_lock = False
> 
> Seems strange that you have to maintain your own flag instead of having it in the port, which actually acquires the lock. Should this be moved there?

I don't think so. This flag mostly exists for the caller to track whether or not he needs to call the stop() routine to release the lock. If I pushed this flag into the port, it would need to expose it publicly (which  I could do).

Arguably the flag should be set by the caller as well, but this class needs to know whether or not it has the lock as well in order to be able to clean up reliably (I suppose it could always release the lock during clean up, but that feels wrong).

Frankly, it's a bad design in the first place to even need to track whether or not we're holding the lock across multiple function calls. This is the fault of the code in TestShellThread.run(), which has an inverted loop structure. That could be re-written, but frankly I'd prefer to just get these patches landed and kill that class completely and then come back and fix this. I can add a FIXME if that would help.
Comment 6 Mihai Parparita 2011-02-11 15:08:30 PST
Comment on attachment 81771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81771&action=review

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:50
>>> +    def deferred_init(self, port):
>> 
>> The naming of this confused me; what is supposed to be deferred about it?
> 
> It isn't called during the constructor, when most initialization is done (and can't be, because the constructor is called pre-fork). It is called during the run() method, when it's actually in the right process. I thought I had added a comment, but I guess not. I'll add one.

At least in dump_render_tree_thread.py, it's called from TestShellThread.__init__. Maybe it should be called init_in_worker_process? (assuming that it's OK for it to be called in TestShellThread's constructor because we're not actually forking in the threading model)?

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:57

>> 
>> Seems strange that you have to maintain your own flag instead of having it in the port, which actually acquires the lock. Should this be moved there?
> 
> I don't think so. This flag mostly exists for the caller to track whether or not he needs to call the stop() routine to release the lock. If I pushed this flag into the port, it would need to expose it publicly (which  I could do).
> 
> Arguably the flag should be set by the caller as well, but this class needs to know whether or not it has the lock as well in order to be able to clean up reliably (I suppose it could always release the lock during clean up, but that feels wrong).
> 
> Frankly, it's a bad design in the first place to even need to track whether or not we're holding the lock across multiple function calls. This is the fault of the code in TestShellThread.run(), which has an inverted loop structure. That could be re-written, but frankly I'd prefer to just get these patches landed and kill that class completely and then come back and fix this. I can add a FIXME if that would help.

A FIXME is fine.
Comment 7 Dirk Pranke 2011-02-11 16:25:19 PST
(In reply to comment #6)
> (From update of attachment 81771 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81771&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/layout_package/worker_mixin.py:50
> >>> +    def deferred_init(self, port):
> >> 
> >> The naming of this confused me; what is supposed to be deferred about it?
> > 
> > It isn't called during the constructor, when most initialization is done (and can't be, because the constructor is called pre-fork). It is called during the run() method, when it's actually in the right process. I thought I had added a comment, but I guess not. I'll add one.
> 
> At least in dump_render_tree_thread.py, it's called from TestShellThread.__init__. Maybe it should be called init_in_worker_process? (assuming that it's OK for it to be called in TestShellThread's constructor because we're not actually forking in the threading model)?
>

ok. In bug 54082, you'd have seen it getting called from inside Worker.run(). In dump_render_thread it's safe to be called during __init__, but it could also be called during run(). Maybe something like run_init()?
I'm a little reluctant to call it init_in_worker_process because only one of the five ways it's called is actually from a child process (the others being two called in the same thread and two called from a separate thread).
Comment 8 Dirk Pranke 2011-02-11 18:33:30 PST
Created attachment 82220 [details]
merge in changes from 54070, 54071, 54072, 54074, feedback from mihaip
Comment 9 Dirk Pranke 2011-02-14 14:56:56 PST
Committed r78511: <http://trac.webkit.org/changeset/78511>