Bug 78572 – webkitpy: add a worker_args concept to start_worker()

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

[Dirk Pranke]

webkitpy: add a worker_args concept to start_worker()

[Dirk Pranke]

Created attachment 126902 [details]
Patch

[Dirk Pranke]

Tony, please take a look?

[Tony Chang]

Comment on attachment 126902 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:108
> +        If the worker is being run in the same thread/process as the manager, it may be
> +        passed an optional inline_arg from the manager. Use of this object is discouraged
> +        as workers should be able to run in a shared-nothing environment in separate processes."""

Rather than passing a separate param, why not make |port| an optional param for WorkerParameters.  Or add a setter method to WorkerParameters that adds port?  It's weird to have this param hang around for some callers.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:47
> +class WorkerArgs(object):

WorkerArguments or WorkerParameters

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:55
> +    def __init__(self, worker_connection, worker_arg_obj):

worker_arg_obj -> worker_arguments or worker_parameters.  _obj doesn't add anything.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:56
> +        super(Worker, self).__init__(worker_connection, worker_arg_obj)

Nit: Why not be explicit about which init we're calling?

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 126902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126902&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:108
> > +        If the worker is being run in the same thread/process as the manager, it may be
> > +        passed an optional inline_arg from the manager. Use of this object is discouraged
> > +        as workers should be able to run in a shared-nothing environment in separate processes."""
> 
> Rather than passing a separate param, why not make |port| an optional param for WorkerParameters.  Or add a setter method to WorkerParameters that adds port?  It's weird to have this param hang around for some callers.

We need to distinguish between parameters that are always passed and available to the Worker, and parameters that are only passed and available in the inline (same-process) case. The inline-only args can't be a part of WorkerArgs, because they are passed to the worker during the constructor and pickled and sent to the child process, and Ports are not expected to be Picklable.

I agree this is weird, but I haven't yet come up with a better way of segregating inline-only from the other parameters (and I've tried a few different approaches). 

> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:47
> > +class WorkerArgs(object):
> 
> WorkerArguments or WorkerParameters
> 

Will change.

> > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:55
> > +    def __init__(self, worker_connection, worker_arg_obj):
> 
> worker_arg_obj -> worker_arguments or worker_parameters.  _obj doesn't add anything.
>

Will change. I was trying to capture the concept that it was a single object rather than a list or a dict might be useful, but the type doesn't actually matter in practice as long as it's pickleable and I agree the name is awkward.

> > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:56
> > +        super(Worker, self).__init__(worker_connection, worker_arg_obj)
> 
> Nit: Why not be explicit about which init we're calling?

No real reason. I will change it.

[Dirk Pranke]

Created attachment 127091 [details]
update w/ review feedback, use set_inline_arguments()

[Tony Chang]

Comment on attachment 127091 [details]
update w/ review feedback, use set_inline_arguments()

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:766
> +            if self._options.worker_model == 'inline':
> +                # Note that this only works because the worker hasn't really started yet
> +                # and won't start running until we call run_message_loop(), below.

I would add a FIXME because we want to remove this hack in the long run.

[Tony Chang]

Comment on attachment 127091 [details]
update w/ review feedback, use set_inline_arguments()

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

> Tools/ChangeLog:10
> +        wrapper class and a separate set_inline_arguments() call that can

I would probably just have named it set_port because that's all we need it for (it's less confusing about what's being passed around). It's fine for the test to not actually receive a port but some mock object since it's just a test.

[Dirk Pranke]

Committed r107867: <http://trac.webkit.org/changeset/107867>

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 127091 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127091&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:766
> > +            if self._options.worker_model == 'inline':
> > +                # Note that this only works because the worker hasn't really started yet
> > +                # and won't start running until we call run_message_loop(), below.
> 
> I would add a FIXME because we want to remove this hack in the long run.

Done.

(In reply to comment #7)
> > Tools/ChangeLog:10
> > +        wrapper class and a separate set_inline_arguments() call that can
> 
> I would probably just have named it set_port because that's all we need it for (it's less confusing about what's being passed around). It's fine for the test to not actually receive a port but some mock object since it's just a test.

Ultimately all of these changes are so that we can reuse the broker logic for test-webkitpy (and possibly other python usages), and it's not yet 100% clear to me that we won't generically need this, so using a generic name seemed more appropriate.