Bug 53158 - nrwt multiprocessing: add simplified message broker for new-style messaging
Summary: nrwt multiprocessing: add simplified message broker for new-style messaging
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: 53157
Blocks: 49566
  Show dependency treegraph
 
Reported: 2011-01-25 20:01 PST by Dirk Pranke
Modified: 2011-02-08 22:13 PST (History)
4 users (show)

See Also:


Attachments
Patch (12.88 KB, patch)
2011-01-25 20:04 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
add more comments describing the design (14.03 KB, patch)
2011-02-02 18:58 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
basic object diagram of the main classes in NRWT pre-multiprocessing rewrite (504.42 KB, image/svg+xml)
2011-02-03 18:41 PST, Dirk Pranke
no flags Details
object diagram post- multiprocessing rewrite (917.13 KB, image/svg+xml)
2011-02-03 18:41 PST, Dirk Pranke
no flags Details
update with review feedback from tony (14.20 KB, patch)
2011-02-04 15:15 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix typos (14.20 KB, patch)
2011-02-04 15:34 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
object diagram post- multiprocessing rewrite (900.05 KB, image/svg+xml)
2011-02-04 15:36 PST, Dirk Pranke
no flags Details
update w/ review feedback from mihaip (13.12 KB, patch)
2011-02-07 21:49 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-01-25 20:01:09 PST
new-run-webkit-tests: add simplified message broker for new-style messaging
Comment 1 Dirk Pranke 2011-01-25 20:04:17 PST
Created attachment 80164 [details]
Patch
Comment 2 Dirk Pranke 2011-02-02 18:58:43 PST
Created attachment 81021 [details]
add more comments describing the design
Comment 3 Tony Chang 2011-02-03 16:36:05 PST
Comment on attachment 81021 [details]
add more comments describing the design

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

r- because I still don't have a good sense of how these classes fit together.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:31
> +"""Module for handling messaging for run-webkit-tests.
> +
> +This modules implements a simple message broker abstraction that will be

This docstring seems overly long and it seems like it could be done simply as a ASCII art diagram with a couple sentences of text.  Having read the docstring a few times, I still don't have a good idea of how these classes interact.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:76
> +class ClientInterface(object):
> +    """Abstract base class / interface that all message broker clients must

Nit: I would maybe just name this BrokerClient.  That seems to be more consistent with the naming WebKit/WebCore.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:116
> +        Args:
> +            options: a runtime option class from optparse

Nit: Can you document queue_maker?

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:124
> +    def add_topic(self, topic_name):
> +        if topic_name not in self._topics:
> +            self._topics[topic_name] = self._queue_maker()

Do we change topics on the fly or is the list of topics something that can be passed into the ctor?

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:127
> +    def _find_topic(self, topic_name):
> +        return self._topics[topic_name]

Nit: Maybe _get_topic_queue or even _get_message_queue_for_topic?
Comment 4 Dirk Pranke 2011-02-03 18:37:49 PST
(In reply to comment #3)
> (From update of attachment 81021 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81021&action=review
> 
> r- because I still don't have a good sense of how these classes fit together.
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:31
> > +"""Module for handling messaging for run-webkit-tests.
> > +
> > +This modules implements a simple message broker abstraction that will be
> 
> This docstring seems overly long and it seems like it could be done simply as a ASCII art diagram with a couple sentences of text.  Having read the docstring a few times, I still don't have a good idea of how these classes interact.

I'm not sure that ASCII art would really help. I did a couple of drawings in Google Docs that I sent you the links to, and am attaching PDFs of them here ... Feel free to try and condense them into ASCII :)

> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:76
> > +class ClientInterface(object):
> > +    """Abstract base class / interface that all message broker clients must
> 
> Nit: I would maybe just name this BrokerClient.  That seems to be more consistent with the naming WebKit/WebCore.
>

Okay, I don't think either ClientInterface or BrokerClient are great names, but I'm happy to change. I'll wait to dicuss the patch with you further before updating the code.
 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:116
> > +        Args:
> > +            options: a runtime option class from optparse
> 
> Nit: Can you document queue_maker?
> 

Whoops. Will do.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:124
> > +    def add_topic(self, topic_name):
> > +        if topic_name not in self._topics:
> > +            self._topics[topic_name] = self._queue_maker()
> 
> Do we change topics on the fly or is the list of topics something that can be passed into the ctor?
>

The way the code is written now, we only create two topics, immediately after creating the broker, so they could be passed into the constructor if so desired.
 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:127
> > +    def _find_topic(self, topic_name):
> > +        return self._topics[topic_name]
> 
> Nit: Maybe _get_topic_queue or even _get_message_queue_for_topic?

Sure, I can make that change.
Comment 5 Dirk Pranke 2011-02-03 18:41:10 PST
Created attachment 81167 [details]
basic object diagram of the main classes in NRWT pre-multiprocessing rewrite
Comment 6 Dirk Pranke 2011-02-03 18:41:44 PST
Created attachment 81168 [details]
object diagram post- multiprocessing rewrite
Comment 7 Dirk Pranke 2011-02-03 18:43:19 PST
Note that it may help things to combine message_broker2.py and manager_worker_broker.py into a single file, since they're kinda related, and only manager_worker_broker calls into message_broker2. I initially kept them separate under the theory that that might make things easier to understand (and because WebKit seems to like the one-big-class-per-file model)
Comment 8 Dirk Pranke 2011-02-04 15:15:04 PST
Created attachment 81302 [details]
update with review feedback from tony
Comment 9 Dirk Pranke 2011-02-04 15:34:59 PST
Created attachment 81308 [details]
fix typos
Comment 10 Dirk Pranke 2011-02-04 15:36:21 PST
Created attachment 81309 [details]
object diagram post- multiprocessing rewrite

updated diagram to be correct about who talks to who between clients, brokerconnections, and brokers.
Comment 11 Mihai Parparita 2011-02-07 14:32:58 PST
Comment on attachment 81308 [details]
fix typos

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:50
> +    TestRunner     ------>    BrokerConnection

So TestRunner implements BrokerClient? It might be worthwhile to make that explicit.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:82
> +    def __init__(self, broker_connection, worker_number, options, **kwargs):

There's a bunch of worker references in the BrokerClient class that seem out of place (i.e. at the wrong abstraction layer). If some (most?) BrokerClient implementations are going to be workers, perhaps it makes sense to have a WorkerBrokerClient abstract class (in a different file) that subclasses BrokerClient has the cancel() and run() abstract methods.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:108
> +    """Brokers provide the basic model of named queues. Clients can post a

There's some inconsistency in naming (topics vs. named queues), both in comments and code. I think consistency would help (I have a slight preference for "topic", since that's more common in publish-subscribe systems).

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:129
> +    def post_message(self, client, topic_name, message_name, *optargs):

Call optargs message_args for consistency with BrokerConnection.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:140
> +        self._run_loop(topic_name, client, True, delay_secs)

Use block=True so that it's easier to read the call.

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:201
> +    def client(self):

Is there a significant benefit from forcing users of this package to subclass BrokerConnection to provide a client, instead of just making it a constructor parameter?
Comment 12 Mihai Parparita 2011-02-07 14:33:48 PST
(In reply to comment #10)
> Created an attachment (id=81309) [details]
> object diagram post- multiprocessing rewrite
> 
> updated diagram to be correct about who talks to who between clients, brokerconnections, and brokers.

Seems like the diagram needs to be updated again (e.g. I think ClientInterface is now BrokerClient).
Comment 13 Mihai Parparita 2011-02-07 14:39:03 PST
Comment on attachment 81308 [details]
fix typos

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2_unittest.py:67
> +    def test_clientinterface_is_abstract(self):

Should be renamed to test_brokerclient_is_abstract (also, I'm not sure if this is a worthwhile test, are we that concerned about 100% coverage?)
Comment 14 Dirk Pranke 2011-02-07 21:48:14 PST
(In reply to comment #11)
> (From update of attachment 81308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81308&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:50
> > +    TestRunner     ------>    BrokerConnection
> 
> So TestRunner implements BrokerClient? It might be worthwhile to make that explicit.
> 

TestRunner doesn't, actually, but TestRunner2 will eventually. I've updated the docstring to use "BrokerClient" for now.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:82
> > +    def __init__(self, broker_connection, worker_number, options, **kwargs):
> 
> There's a bunch of worker references in the BrokerClient class that seem out of place (i.e. at the wrong abstraction layer). If some (most?) BrokerClient implementations are going to be workers, perhaps it makes sense to have a WorkerBrokerClient abstract class (in a different file) that subclasses BrokerClient has the cancel() and run() abstract methods.

Yeah. Mostly the comments were wrong, left over from a previous incarnation where BrokerClient and Workers weren't really different. I've cleaned things up.

> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:108
> > +    """Brokers provide the basic model of named queues. Clients can post a
> 
> There's some inconsistency in naming (topics vs. named queues), both in comments and code. I think consistency would help (I have a slight preference for "topic", since that's more common in publish-subscribe systems).

Agreed. I've used topic.

> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:129
> > +    def post_message(self, client, topic_name, message_name, *optargs):
> 
> Call optargs message_args for consistency with BrokerConnection.
>

Good suggestion. Done.
 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:140
> > +        self._run_loop(topic_name, client, True, delay_secs)
> 
> Use block=True so that it's easier to read the call.
>

Done.
 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:201
> > +    def client(self):
> 
> Is there a significant benefit from forcing users of this package to subclass BrokerConnection to provide a client, instead of just making it a constructor parameter?

Sorry, client was supposed to be passed into the constructor as you say, and callers do not subclass BrokerConnection normally (although manager_worker_broker does).
(In reply to comment #13)

> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2_unittest.py:67
> > +    def test_clientinterface_is_abstract(self):
> 
> Should be renamed to test_brokerclient_is_abstract (also, I'm not sure if this is a worthwhile test, are we that concerned about 100% coverage?)

Renamed. I personally try to get 100% coverage, since it makes it much easier to spot regressions in coverage.
Comment 15 Dirk Pranke 2011-02-07 21:49:08 PST
Created attachment 81582 [details]
update w/ review feedback from mihaip
Comment 16 Tony Chang 2011-02-08 10:29:04 PST
Comment on attachment 81582 [details]
update w/ review feedback from mihaip

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:53
> +    BrokerClient   ------>    BrokerConnection
> +                       <           |
> +                        \          v
> +                                Broker

This might be clearer:
    BrokerClient   ------>    BrokerConnection
           ^                       |
           |                       v
           \----------------    Broker

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:59
> +

Nit: Remove blank line
Comment 17 Dirk Pranke 2011-02-08 16:08:40 PST
(In reply to comment #16)
> (From update of attachment 81582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81582&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:53
> > +    BrokerClient   ------>    BrokerConnection
> > +                       <           |
> > +                        \          v
> > +                                Broker
> 
> This might be clearer:
>     BrokerClient   ------>    BrokerConnection
>            ^                       |
>            |                       v
>            \----------------    Broker
> 

Good idea.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:59
> > +
> 
> Nit: Remove blank line

Will do.
Comment 18 Dirk Pranke 2011-02-08 16:31:47 PST
Committed r77989: <http://trac.webkit.org/changeset/77989>
Comment 19 Mihai Parparita 2011-02-08 20:28:10 PST
Comment on attachment 81582 [details]
update w/ review feedback from mihaip

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:136
> +        queue = self._find_topic(topic_name)

You might have fixed this in a later patch, but _find_topic is not _get_queue_for_topic
Comment 20 Dirk Pranke 2011-02-08 22:13:04 PST
(In reply to comment #19)
> (From update of attachment 81582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81582&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/message_broker2.py:136
> > +        queue = self._find_topic(topic_name)
> 
> You might have fixed this in a later patch, but _find_topic is not _get_queue_for_topic

Yeah, that one slipped through :( It'll get fixed in the next patch I upload.