nrwt multiprocessing - add 'worker number' concept, move stuff to worker thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt multiprocessing - add 'worker number' concept, move stuff to worker thread

[Dirk Pranke]

Created attachment 74316 [details]
Patch

[Dirk Pranke]

Created attachment 74321 [details]
minor cleanup

[Tony Chang]

Comment on attachment 74321 [details]
minor cleanup

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

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:55
> +from webkitpy.layout_tests.test_types import test_type_base
> +from webkitpy.layout_tests.test_types import text_diff
> +from webkitpy.layout_tests.test_types import image_diff

Nit: Sort alphabetically

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:486
> +                                  self._worker_number,
> +                                  self._name,

It seems redundant to have to pass worker number and worker name.  Are there cases where you can't derive the name from the number?  Maybe there should just be a static function somewhere that converts number to name?

[Dirk Pranke]

Created attachment 74335 [details]
review feedback - sort imports alphabetically

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 74321 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74321&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:55
> > +from webkitpy.layout_tests.test_types import test_type_base
> > +from webkitpy.layout_tests.test_types import text_diff
> > +from webkitpy.layout_tests.test_types import image_diff
> 
> Nit: Sort alphabetically
>

Done.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:486
> > +                                  self._worker_number,
> > +                                  self._name,
> 
> It seems redundant to have to pass worker number and worker name.  Are there cases where you can't derive the name from the number?  Maybe there should just be a static function somewhere that converts number to name?

In fact, if you look at older versions of the rollup patches that's what I used to have. Then I realized that there's no reason for SingleTestThread to know that you can derive worker_name from worker_number, and that this better encapsulate things. I'd prefer to leave it as it is.

[Tony Chang]

Comment on attachment 74335 [details]
review feedback - sort imports alphabetically

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

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:171
> +    def __init__(self, port, options, worker_number, worker_name,
> +                 test_input, test_types, test_args):

My main concern with having a separate name and number is that it makes functions with lots of parameters fragile.  Since there's no compile time checking of parameters, it's easy to miss a caller and break things when refactoring.

In this case, it's relatively isolated (only this call takes both), so I don't feel that strongly about it.  This patch doesn't use worker_number, do you need it in the future?  Maybe it should be added when it's used?

[Tony Chang]

Another option for encapsulating is to make a small class that combines worker name and number.  You could have a getter on the class that derives the name from the number.

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 74335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74335&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:171
> > +    def __init__(self, port, options, worker_number, worker_name,
> > +                 test_input, test_types, test_args):
> 
> My main concern with having a separate name and number is that it makes functions with lots of parameters fragile.  Since there's no compile time checking of parameters, it's easy to miss a caller and break things when refactoring.
> 

It's a fair concern. A bunch of these arguments go away in subsequent patches.

[Dirk Pranke]

Comment on attachment 74335 [details]
review feedback - sort imports alphabetically

Clearing flags on attachment: 74335

Committed r72458: <http://trac.webkit.org/changeset/72458>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.