Bug 53477 - nrwt multiprocessing: add stubs for manager/worker
Summary: nrwt multiprocessing: add stubs for manager/worker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 49566
  Show dependency treegraph
 
Reported: 2011-02-01 00:03 PST by Dirk Pranke
Modified: 2011-02-08 16:33 PST (History)
4 users (show)

See Also:


Attachments
Patch (12.15 KB, patch)
2011-02-01 00:03 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
add bug # to ChangeLog (12.21 KB, patch)
2011-02-01 00:05 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
remove thread stack code, add better docstrings, clean up imports (10.38 KB, patch)
2011-02-02 19:15 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update comments, rebase w/ latest 53158 patch (10.11 KB, patch)
2011-02-04 15:34 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback from mihaip (11.50 KB, patch)
2011-02-07 22:03 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix multiprocessing import (24.26 KB, patch)
2011-02-07 22:19 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix diff baseline (11.43 KB, patch)
2011-02-07 22:22 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback from tony (11.07 KB, patch)
2011-02-08 15:57 PST, Dirk Pranke
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>