Bug 53477 – nrwt multiprocessing: add stubs for manager/worker

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

nrwt multiprocessing: add stubs for manager/worker

[Dirk Pranke]

Created attachment 80722 [details]
Patch

[Dirk Pranke]

Created attachment 80723 [details]
add bug # to ChangeLog

[Dirk Pranke]

grr .. can't believe I didn't include y'all on the CC list already.

[Dirk Pranke]

Created attachment 81022 [details]
remove thread stack code, add better docstrings, clean up imports

[Tony Chang]

Comment on attachment 81022 [details]
remove thread stack code, add better docstrings, clean up imports

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

rs=me with the change below

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:57
> +    _multiprocessing_is_available = True
> +    from multiprocessing import Process as _Multiprocessing_Process
> +    from multiprocessing import Queue as _Multiprocessing_Queue

For feature detection like this, I think it's simpler to just try to import multiprocessing and on ImportError, set multiprocessing to None.  You can then use multiprocessing in the code to see what queue you should use.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:109
> +    elif worker_model == 'processes' and _multiprocessing_is_available:
> +        queue_class = _Multiprocessing_Queue
> +        manager_class = _MultiProcessManager

E.g., this would be:
  elif worker_model == 'processes' and multiprocessing:
      queue_class = multiprocessing.Queue
      manager_class = _MultiProcessManager

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:37
> +try:
> +    _multiprocessing_is_available = True
> +    from multiprocessing import Queue as _Multiprocessing_Queue
> +except ImportError:

Same naming as above.

[Dirk Pranke]

Created attachment 81307 [details]
update comments, rebase w/ latest 53158 patch

[Mihai Parparita]

Comment on attachment 81307 [details]
update comments, rebase w/ latest 53158 patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:38
> +eventually work as follows:

I'd really like to know what this says :)

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:44
> +import Queue

Can you move this import into the no multiprocessing branch below?

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:74
> +    by this method."""

Not sure I understand this comment. By "this method" you mean "the specified worker model" (i.e. different models will have different sets of options)?

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:91
> +            the methods in message_broker2.ClientInterface.

s/ClientInterface/BrokerClient/

[Tony Chang]

Comment on attachment 81307 [details]
update comments, rebase w/ latest 53158 patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:53
> +    _multiprocessing_is_available = True
> +    from multiprocessing import Process as _Multiprocessing_Process
> +    from multiprocessing import Queue as _Multiprocessing_Queue

Did the import simplification not work?

[Dirk Pranke]

Created attachment 81584 [details]
update w/ review feedback from mihaip

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 81307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81307&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:38
> > +eventually work as follows:
> 
> I'd really like to know what this says :)
> 

Actually, you don't. That was leftover from a comment that was pretty useless, but I didn't quite delete everything I should've. I've updated it to be slightly better.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:44
> > +import Queue
> 
> Can you move this import into the no multiprocessing branch below?
>

No. This'll be used by the threads-based version either way.
 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:74
> > +    by this method."""
> 
> Not sure I understand this comment. By "this method" you mean "the specified worker model" (i.e. different models will have different sets of options)?

s/method/module. Which should make more sense.

> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:91
> > +            the methods in message_broker2.ClientInterface.
> 
> s/ClientInterface/BrokerClient/

Done.

[Dirk Pranke]

Created attachment 81585 [details]
fix multiprocessing import

[Dirk Pranke]

(In reply to comment #8)
> (From update of attachment 81307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81307&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:53
> > +    _multiprocessing_is_available = True
> > +    from multiprocessing import Process as _Multiprocessing_Process
> > +    from multiprocessing import Queue as _Multiprocessing_Queue
> 
> Did the import simplification not work?

Whoops. Fixed :)

[Dirk Pranke]

Created attachment 81586 [details]
fix diff baseline

[Tony Chang]

Comment on attachment 81586 [details]
fix diff baseline

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:53
> +    _Multiprocessing_Process = multiprocessing.Process

This doesn't appear to be used anywhere.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:105
> +    elif worker_model == 'processes' and multiprocessing:
> +        queue_class = _Multiprocessing_Queue

This appears to be the only use of _Multiprocessing_Queue and it's behind a multiprocessing check.  Can this just be multiprocessing.Queue and remove the variable?

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:35
> +    _multiprocessing_is_available = True

import multiprocessing

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:36
> +    from multiprocessing import Queue as _Multiprocessing_Queue

Not used?

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:38
> +    _multiprocessing_is_available = False

multiprocessing = None

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:39
> +    _Multiprocessing_Queue = Queue.Queue

Not used?

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:74
> +        if _multiprocessing_is_available:

if multiprocessing:

[Dirk Pranke]

Created attachment 81710 [details]
update w/ review feedback from tony

[Dirk Pranke]

(In reply to comment #14)
> (From update of attachment 81586 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81586&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:53
> > +    _Multiprocessing_Process = multiprocessing.Process
> 
> This doesn't appear to be used anywhere.
> 

It will be used in  the future, but I've removed it for now.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:105
> > +    elif worker_model == 'processes' and multiprocessing:
> > +        queue_class = _Multiprocessing_Queue
> 
> This appears to be the only use of _Multiprocessing_Queue and it's behind a multiprocessing check.  Can this just be multiprocessing.Queue and remove the variable?
>

Done.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:35
> > +    _multiprocessing_is_available = True
> 
> import multiprocessing
>

Done.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:36
> > +    from multiprocessing import Queue as _Multiprocessing_Queue
> 
> Not used?
>

Removed.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:38
> > +    _multiprocessing_is_available = False
> 
> multiprocessing = None
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:39
> > +    _Multiprocessing_Queue = Queue.Queue
> 
> Not used?
> 

will be used in the future, but I can call it multiprocessing.Queue then. Removed.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:74
> > +        if _multiprocessing_is_available:
> 
> if multiprocessing:

Done.

[Dirk Pranke]

Committed r77990: <http://trac.webkit.org/changeset/77990>