Bug 50374 – nrwt multiprocessing - simpify message_broker and move logic back into run_webkit_tests

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Dirk Pranke]

nrwt multiprocessing - move logic back into run_webkit_tests

[Dirk Pranke]

Created attachment 75357 [details]
Patch

[Tony Chang]

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(....

[Dirk Pranke]

(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.

[Tony Chang]

(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.

[Dirk Pranke]

(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!

[Dirk Pranke]

Created attachment 75445 [details]
update w/ review feedback

[Dirk Pranke]

Committed r73231: <http://trac.webkit.org/changeset/73231>

[Yuta Kitamura]

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.

[Dirk Pranke]

closing ... the new patches are different.