Summary: | nrwt multiprocessing - simpify message_broker and move logic back into run_webkit_tests | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||
Status: | RESOLVED WONTFIX | ||||||||
Severity: | Normal | CC: | eric, hayato, ojan, tony, yutak | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | 50372, 50443 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Dirk Pranke
2010-12-01 22:53:10 PST
Created attachment 75357 [details]
Patch
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(.... (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. (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. (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! Created attachment 75445 [details]
update w/ review feedback
Committed r73231: <http://trac.webkit.org/changeset/73231> 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. closing ... the new patches are different. |