Bug 50374 - nrwt multiprocessing - simpify message_broker and move logic back into run_webkit_tests
Summary: nrwt multiprocessing - simpify message_broker and move logic back into run_we...
Status: RESOLVED WONTFIX
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: 50372 50443
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-01 22:53 PST by Dirk Pranke
Modified: 2011-01-11 19:01 PST (History)
5 users (show)

See Also:


Attachments
Patch (22.91 KB, patch)
2010-12-01 23:48 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback (24.49 KB, patch)
2010-12-02 17:38 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-12-01 22:53:10 PST
nrwt multiprocessing - move logic back into run_webkit_tests
Comment 1 Dirk Pranke 2010-12-01 23:48:22 PST
Created attachment 75357 [details]
Patch
Comment 2 Tony Chang 2010-12-02 12:15:46 PST
Comment on attachment 75357 [details]
Patch

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

Seems like this is mostly just moving stuff around.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:117
> +        thread = super(_MultiThreadedBroker, self).start_worker(test_runner,

Nit: I think the preferred calling convention is _WorkerMessageBroker.start_worker(self, test_runner, worker_number) because super can be ambiguous in the case of multiple inheritance.  I think this is consistent to how we use the super class name in __init__.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:233
> +# FIXME: This name is somewhat misleading because the actual workers have
> +# their own internal state in TestShellThread. However, a better name for
> +# this has yet to be found.

Nit: I think this name is fine for now and the docstring describes it.  I would just remove this comment.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:269
> +        # or processes. This is a bit awkward but a better name has yet to

Nit: Missing closing paren.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:687
> +        for thread in [w.thread for w in workers.values()]:
> +            thread_timings.append({'name': thread.name(),

This is a bit convoluted.  Why not just:
for worker in workers.values():
    thread = worker.thread
    thread_timings.append(....
Comment 3 Dirk Pranke 2010-12-02 14:03:17 PST
(In reply to comment #2)
> (From update of attachment 75357 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75357&action=review
> 
> Seems like this is mostly just moving stuff around.
> 

Yeah, pretty much.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:117
> > +        thread = super(_MultiThreadedBroker, self).start_worker(test_runner,
> 
> Nit: I think the preferred calling convention is _WorkerMessageBroker.start_worker(self, test_runner, worker_number) because super can be ambiguous in the case of multiple inheritance.  I think this is consistent to how we use the super class name in __init__.
> 

Maybe I'm misunderstanding something. Are you're saying it's ambiguous because super() might end up calling threading.start_broker() if MultiThreadedBroker didn't implement the method, and it's better to be explicit about which class you intended to invoke?

Yeah, I could agree that that might be clearer, but I wanted to confirm what you were saying before changing anything.
Comment 4 Tony Chang 2010-12-02 14:41:47 PST
(In reply to comment #3)
> Maybe I'm misunderstanding something. Are you're saying it's ambiguous because super() might end up calling threading.start_broker() if MultiThreadedBroker didn't implement the method, and it's better to be explicit about which class you intended to invoke?

I'm just saying being explicit is better than using super().  I agree that it's not actually dangerous in this case.
Comment 5 Dirk Pranke 2010-12-02 17:35:29 PST
(In reply to comment #2)
> (From update of attachment 75357 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75357&action=review
> 
> Seems like this is mostly just moving stuff around.
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:117
> > +        thread = super(_MultiThreadedBroker, self).start_worker(test_runner,
> 
> Nit: I think the preferred calling convention is _WorkerMessageBroker.start_worker(self, test_runner, worker_number) because super can be ambiguous in the case of multiple inheritance.  I think this is consistent to how we use the super class name in __init__.

Fixed in bug 50372.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:233
> > +# FIXME: This name is somewhat misleading because the actual workers have
> > +# their own internal state in TestShellThread. However, a better name for
> > +# this has yet to be found.
> 
> Nit: I think this name is fine for now and the docstring describes it.  I would just remove this comment.
>

Done.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:269
> > +        # or processes. This is a bit awkward but a better name has yet to
> 
> Nit: Missing closing paren.
>

Fixed.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:687
> > +        for thread in [w.thread for w in workers.values()]:
> > +            thread_timings.append({'name': thread.name(),
> 
> This is a bit convoluted.  Why not just:
> for worker in workers.values():
>     thread = worker.thread
>     thread_timings.append(....

Done. Thanks for the review!
Comment 6 Dirk Pranke 2010-12-02 17:38:09 PST
Created attachment 75445 [details]
update w/ review feedback
Comment 7 Dirk Pranke 2010-12-02 19:42:04 PST
Committed r73231: <http://trac.webkit.org/changeset/73231>
Comment 8 Yuta Kitamura 2010-12-03 02:10:00 PST
Hi,

It seemed that r73211 broke Chromium's "Webkit Win (dbg)(2)" bot. Unfortunately, there is some conflict with this change, so I needed to roll out this patch.

http://trac.webkit.org/changeset/73255

Feel free to re-land this patch after r73211 is rolled out. Thank you for your understandings.
Comment 9 Dirk Pranke 2011-01-11 19:01:10 PST
closing ... the new patches are different.