Bug 78572 - webkitpy: add a worker_args concept to start_worker()
Summary: webkitpy: add a worker_args concept to start_worker()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 78171
  Show dependency treegraph
 
Reported: 2012-02-13 21:01 PST by Dirk Pranke
Modified: 2012-02-15 18:43 PST (History)
4 users (show)

See Also:


Attachments
Patch (15.19 KB, patch)
2012-02-13 21:05 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback, use set_inline_arguments() (18.51 KB, patch)
2012-02-14 17:53 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 2012-02-13 21:01:29 PST
webkitpy: add a worker_args concept to start_worker()
Comment 1 Dirk Pranke 2012-02-13 21:05:49 PST
Created attachment 126902 [details]
Patch
Comment 2 Dirk Pranke 2012-02-13 21:17:43 PST
Tony, please take a look?
Comment 3 Tony Chang 2012-02-14 16:41:23 PST
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?
Comment 4 Dirk Pranke 2012-02-14 16:51:35 PST
(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.
Comment 5 Dirk Pranke 2012-02-14 17:53:31 PST
Created attachment 127091 [details]
update w/ review feedback, use set_inline_arguments()
Comment 6 Tony Chang 2012-02-15 09:54:38 PST
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.
Comment 7 Tony Chang 2012-02-15 09:56:51 PST
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.
Comment 8 Dirk Pranke 2012-02-15 18:41:25 PST
Committed r107867: <http://trac.webkit.org/changeset/107867>
Comment 9 Dirk Pranke 2012-02-15 18:43:01 PST
(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.