Bug 53477

Summary: nrwt multiprocessing: add stubs for manager/worker
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, mihaip, ojan, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 49566    
Attachments:
Description Flags
Patch
none
add bug # to ChangeLog
none
remove thread stack code, add better docstrings, clean up imports
none
update comments, rebase w/ latest 53158 patch
none
update w/ review feedback from mihaip
none
fix multiprocessing import
none
fix diff baseline
none
update w/ review feedback from tony tony: review+

Description Dirk Pranke 2011-02-01 00:03:04 PST
nrwt multiprocessing: add stubs for manager/worker
Comment 1 Dirk Pranke 2011-02-01 00:03:33 PST
Created attachment 80722 [details]
Patch
Comment 2 Dirk Pranke 2011-02-01 00:05:56 PST
Created attachment 80723 [details]
add bug # to ChangeLog
Comment 3 Dirk Pranke 2011-02-02 16:01:23 PST
grr .. can't believe I didn't include y'all on the CC list already.
Comment 4 Dirk Pranke 2011-02-02 19:15:15 PST
Created attachment 81022 [details]
remove thread stack code, add better docstrings, clean up imports
Comment 5 Tony Chang 2011-02-03 14:10:24 PST
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.
Comment 6 Dirk Pranke 2011-02-04 15:34:03 PST
Created attachment 81307 [details]
update comments, rebase w/ latest 53158 patch
Comment 7 Mihai Parparita 2011-02-07 14:44:56 PST
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/
Comment 8 Tony Chang 2011-02-07 15:57:05 PST
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?
Comment 9 Dirk Pranke 2011-02-07 22:03:10 PST
Created attachment 81584 [details]
update w/ review feedback from mihaip
Comment 10 Dirk Pranke 2011-02-07 22:04:58 PST
(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.
Comment 11 Dirk Pranke 2011-02-07 22:19:20 PST
Created attachment 81585 [details]
fix multiprocessing import
Comment 12 Dirk Pranke 2011-02-07 22:21:17 PST
(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 :)
Comment 13 Dirk Pranke 2011-02-07 22:22:05 PST
Created attachment 81586 [details]
fix diff baseline
Comment 14 Tony Chang 2011-02-08 11:02:45 PST
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:
Comment 15 Dirk Pranke 2011-02-08 15:57:45 PST
Created attachment 81710 [details]
update w/ review feedback from tony
Comment 16 Dirk Pranke 2011-02-08 16:07:51 PST
(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.
Comment 17 Dirk Pranke 2011-02-08 16:33:47 PST
Committed r77990: <http://trac.webkit.org/changeset/77990>