Bug 49779 – nrwt multiprocessing - implement first part of broker object

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

[Dirk Pranke]

nrwt multiprocessing - implement first part of broker object

[Dirk Pranke]

Created attachment 74341 [details]
Patch

[Dirk Pranke]

Created attachment 74343 [details]
fix missing import

[Dirk Pranke]

Note that this patch depends on bug 49768 landing first.

[Dirk Pranke]

Created attachment 74351 [details]
yet more cleanup

[Tony Chang]

Comment on attachment 74351 [details]
yet more cleanup

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

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:48
> +    if options.workers_are == 'inline':
> +        return SingleThreadedBroker(port, options)

Where is the workers_are flag declared?  I think workers_are is a poor name.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:56
> +class _WorkerState(object):
> +    def __init__(self):

Maybe this class should be declared in AbstractBroker?  Do other classes need to instantiate it?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:62
> +class AbstractBroker(object):
> +    def __init__(self, port, options):

Can you add a docstring describing AbstractBroker?  The name is very vague (could be named better?).

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:76
> +            worker = _WorkerState()
> +            worker.thread = self._start_worker(worker_number)
> +            worker.name = 'worker-%d' % worker_number

Wouldn't it be better to put this in __init__?  Also, isn't this name generation a duplicate with code from bug 49768?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:81
> +    def _start_worker(self, worker_number):
> +        raise NotImplementedError

Please document what implementations should go here and what the return type should be.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:85
> +    def loop(self):
> +        """Loop waiting for threads to finish."""
> +        raise NotImplementedError

Based on the docstring, it's not clear to me what the implementation is supposed to do.  Looking at SingleThreadedBroker, it looks like this is the run_loop() command.  What should be returned?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:101
> +        # Note: Don't start() the thread!

Fill this comment out and say why.  It's not obvious to me why this method doesn't start the thread and MultiThreadedBroker does.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:141
> +                    if (next_timeout and t > next_timeout):

Nit: remove the ()

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker_unittest.py:109
> +        # FIXME: implement
> +        return

Why do these tests no longer work?  Can we make it work in this patch?  It's nice to have each step of the code landing be in a working state.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:253
> +        self._broker = broker

I think this variable should be named more specifically than broker.  The name broker doesn't tell you anything about what it does.  A broker for what?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:629
> +        (thread_timings, test_timings, individual_test_timings) = \

Nit: Not () on the left side.

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 74351 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74351&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:48
> > +    if options.workers_are == 'inline':
> > +        return SingleThreadedBroker(port, options)
> 
> Where is the workers_are flag declared?  I think workers_are is a poor name.
> 

It's declared in run_webkit_teste.parse_args, along with all the other options fields. It's an enum, with valid values 'inline', 'threads', and 'processes', i.e., --workers-are=threads . Which reads nicely. But I grant your point about it being awkward without seeing the value of the field. Do you have any suggestions you like better?

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:56
> > +class _WorkerState(object):
> > +    def __init__(self):
> 
> Maybe this class should be declared in AbstractBroker?  Do other classes need to instantiate it?
>

It could be in AbstractBroker, and only AbstractBroker creates them, but I'm not generally a fan of nested classes.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:62
> > +class AbstractBroker(object):
> > +    def __init__(self, port, options):
> 
> Can you add a docstring describing AbstractBroker?  The name is very vague (could be named better?).
> 

Will do. It's an abstract base class for the other broker classes - would you prefer just Broker, or BrokerBase, or BaseBroker, or something else?

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:76
> > +            worker = _WorkerState()
> > +            worker.thread = self._start_worker(worker_number)
> > +            worker.name = 'worker-%d' % worker_number
> 
> Wouldn't it be better to put this in __init__?  Also, isn't this name generation a duplicate with code from bug 49768?

It could go in init(); It's just a struct, so I don't feel strongly about needing to use constructors (but I will add it, because there's also no particular reason not to). 

Yes, the name generation is duplicated with bug 49768. That code goes away in favor of this code. The downside of breaking up a single patch into multiple patches :(

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:81
> > +    def _start_worker(self, worker_number):
> > +        raise NotImplementedError
> 
> Please document what implementations should go here and what the return type should be.
>

Will do.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:85
> > +    def loop(self):
> > +        """Loop waiting for threads to finish."""
> > +        raise NotImplementedError
> 
> Based on the docstring, it's not clear to me what the implementation is supposed to do.  Looking at SingleThreadedBroker, it looks like this is the run_loop() command.  What should be returned?
>

I will add comments. Nothing is returned.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:101
> > +        # Note: Don't start() the thread!
> 
> Fill this comment out and say why.  It's not obvious to me why this method doesn't start the thread and MultiThreadedBroker does.
>

I will add more comments. If we called start(), then the function would be run in the other thread, and it would no longer be a SingleThreadedBroker ;) As the FIXME above indicates, this really shouldn't be using threads at all, but that gets fixed later when we can do more surgery on dump_render_tree_thread.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:141
> > +                    if (next_timeout and t > next_timeout):
> 
> Nit: remove the ()
>

Will do.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker_unittest.py:109
> > +        # FIXME: implement
> > +        return
> 
> Why do these tests no longer work?  Can we make it work in this patch?  It's nice to have each step of the code landing be in a working state.
>

I think the threads actually still work in this particular patch, but they break and become irrelevant in subsequent patches. I can make them work in this version, and then delete them later (which is what I will do unless I hear otherwise), or I can just delete them now if you think that would also be okay.

The code that lands is actually working, we just get slightly worse test coverage temporarily.

> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:253
> > +        self._broker = broker
> 
> I think this variable should be named more specifically than broker.  The name broker doesn't tell you anything about what it does.  A broker for what?
>

We only have message brokers in these packages, so if something needed to be more specific than "broker" it could be "message_broker", or perhaps something else. However, the "broker" concept is pretty fundamental to the new design of the code, so it doesn't seem like much to ask for the reader to be able to grok this pretty quickly, and "message_broker" then feels very verbose and cumbersome. 

But then, I almost always prefer things to be terser than the general webkit/python style we seem to use. Given the above, what do you think? Would you prefer "message_broker"? Do you have a better suggestion?
  
> > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:629
> > +        (thread_timings, test_timings, individual_test_timings) = \
> 
> Nit: Not () on the left side.

Will fix.

[Tony Chang]

Comment on attachment 74351 [details]
yet more cleanup

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

>>> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:48
>>> +        return SingleThreadedBroker(port, options)
>> 
>> Where is the workers_are flag declared?  I think workers_are is a poor name.
> 
> It's declared in run_webkit_teste.parse_args, along with all the other options fields. It's an enum, with valid values 'inline', 'threads', and 'processes', i.e., --workers-are=threads . Which reads nicely. But I grant your point about it being awkward without seeing the value of the field. Do you have any suggestions you like better?

I mean it's not declared in ToT and I don't see it in bug 49768 which you said this patch depends on.  It looks like an undeclared option at this point.

>>> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:62
>>> +    def __init__(self, port, options):
>> 
>> Can you add a docstring describing AbstractBroker?  The name is very vague (could be named better?).
> 
> Will do. It's an abstract base class for the other broker classes - would you prefer just Broker, or BrokerBase, or BaseBroker, or something else?

Broker is a poor name for a class since it's just a pattern.  It would be better to name the type of broker like MessageBroker or WorkerQueueBroker or something.  It should be obvious to someone not familiar with the code what the broker is responsible for brokering.

>>> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:76
>>> +            worker.name = 'worker-%d' % worker_number
>> 
>> Wouldn't it be better to put this in __init__?  Also, isn't this name generation a duplicate with code from bug 49768?
> 
> It could go in init(); It's just a struct, so I don't feel strongly about needing to use constructors (but I will add it, because there's also no particular reason not to). 
> 
> Yes, the name generation is duplicated with bug 49768. That code goes away in favor of this code. The downside of breaking up a single patch into multiple patches :(

When breaking up a large patch into smaller patches, I think the goal should be for each additional patch to stand on its own, not on some future patch.  It's ok to add code that is used only for an intermediate state that gets removed as you go along.

Please add the logic to the constructor to avoid forgetting to initialize properly.

>>> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:85
>>> +        raise NotImplementedError
>> 
>> Based on the docstring, it's not clear to me what the implementation is supposed to do.  Looking at SingleThreadedBroker, it looks like this is the run_loop() command.  What should be returned?
> 
> I will add comments. Nothing is returned.

SingleThreadedBroker returns False.  I think run_loop would be a better name than just loop.

>>> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker_unittest.py:109
>>> +        return
>> 
>> Why do these tests no longer work?  Can we make it work in this patch?  It's nice to have each step of the code landing be in a working state.
> 
> I think the threads actually still work in this particular patch, but they break and become irrelevant in subsequent patches. I can make them work in this version, and then delete them later (which is what I will do unless I hear otherwise), or I can just delete them now if you think that would also be okay.
> 
> The code that lands is actually working, we just get slightly worse test coverage temporarily.

If they still work, then let's not disable them.  We can delete them when they're no longer relevant.

>>> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:253
>>> +        self._broker = broker
>> 
>> I think this variable should be named more specifically than broker.  The name broker doesn't tell you anything about what it does.  A broker for what?
> 
> We only have message brokers in these packages, so if something needed to be more specific than "broker" it could be "message_broker", or perhaps something else. However, the "broker" concept is pretty fundamental to the new design of the code, so it doesn't seem like much to ask for the reader to be able to grok this pretty quickly, and "message_broker" then feels very verbose and cumbersome. 
> 
> But then, I almost always prefer things to be terser than the general webkit/python style we seem to use. Given the above, what do you think? Would you prefer "message_broker"? Do you have a better suggestion?

See above comment.  I would prefer message_broker.  Naming it broker is like naming a class Factory instead of FooBarFactory.

[Dirk Pranke]

Created attachment 74455 [details]
update w/ review feedback

[Dirk Pranke]

(In reply to comment #8)
> Created an attachment (id=74455) [details]
> update w/ review feedback

Tony, I think the latest patch covers everything we talked about over IM. Can you take another look?

[Tony Chang]

Comment on attachment 74455 [details]
update w/ review feedback

r- only because it seems like we should resolve the workers-are naming issue in bug 49773 first.

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

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:65
> +        self.name = None

self.name = name

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:74
> +        self._workers = {}

Nit: maybe a comment saying this maps worker names to _WorkerState?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:77
> +        return [w.thread for w in self._workers.values()]

Nit: Should this return a tuple instead of a list?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:98
> +        """Loop processing messages until done. Returns nothing."""

I don't think you need to say the method returns nothing.  I was just confused before because of the implementation returning false.  Same with cancel_workers and cleanup.

[Dirk Pranke]

Created attachment 74702 [details]
update w/ more review feedback.

[Tony Chang]

Comment on attachment 74702 [details]
update w/ more review feedback.

r- for self.name typo.

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

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:65
> +        self.name = None

self.name = name?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:110
> +class SingleThreadedBroker(WorkerMessageBroker):

Nit: Should this be an InlineBroker to match the flag name?

[Dirk Pranke]

Created attachment 74707 [details]
Patch

[Dirk Pranke]

(In reply to comment #12)
> (From update of attachment 74702 [details])
> r- for self.name typo.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=74702&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:65
> > +        self.name = None
> 
> self.name = name?
> 

Good catch. Done.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:110
> > +class SingleThreadedBroker(WorkerMessageBroker):
> 
> Nit: Should this be an InlineBroker to match the flag name?

Sure, why not. Done. Thanks for the feedback.

[Dirk Pranke]

Created attachment 74711 [details]
Patch

[Dirk Pranke]

Created attachment 74716 [details]
Patch

[Dirk Pranke]

Comment on attachment 74716 [details]
Patch

Clearing flags on attachment: 74716

Committed r72698: <http://trac.webkit.org/changeset/72698>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.