Changeset 121809 in webkit


Ignore:
Timestamp:
Jul 3, 2012 4:31:57 PM (12 years ago)
Author:
dpranke@chromium.org
Message:

nrwt: make the worker class stand alone with a cleaner interface
​https://bugs.webkit.org/show_bug.cgi?id=90409

Reviewed by Ojan Vafai.

Currently the Worker class derives from AbstractWorker, which is
kind of crufty and awkward; it would be better if we did not
rely on shared state.

This change changes that so that Worker derives from object, and
exposes the following interface:

init() - called in the manager process
safe_init() - called in the worker process to initialize

unpicklable state

handle() - a single routine to handle all messages
cleanup() - called so the worker can clean up

Also, all of the "administrative" messages that are handled by
the worker (notification of start/stop/etc.) move into
manager_worker_broker - this reduces worker.py to just handling
the mechanics of actually running each test.

For the moment, we do this by creating Yet Another wrapper/proxy
class in manager_worker_broker, but this will get simpler
shortly when the rest of m_w_b is cleaned up.

With this change worker is now in its new form but there will be
a follow-on change that cleans up some names and other minor
things.

This change is again mostly just moving things around and should
be covered by the (updated) existing tests.

  • Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:

(get):
(AbstractWorker.init):
(AbstractWorker.run):
(AbstractWorker):
(AbstractWorker.handle_stop):
(AbstractWorker.handle_test_list):
(AbstractWorker.yield_to_broker):
(AbstractWorker.post_message):
(_WorkerConnection.init):
(_Process.run):

  • Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py:

(_TestWorker):
(_TestWorker.init):
(_TestWorker.name):
(_TestWorker.cleanup):
(_TestWorker.handle):
(_TestWorker.safe_init):
(_TestWorker.stop):
(_TestsMixin.handle_finished_test):
(_TestsMixin.setUp):
(_TestsMixin.test_cancel):
(_TestsMixin.test_done):

  • Scripts/webkitpy/layout_tests/controllers/worker.py:

(Worker):
(Worker.init):
(Worker.safe_init):
(Worker.handle):

Location:
trunk/Tools
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Tools/ChangeLog

    r121807 r121809  
     12012-07-03  Dirk Pranke  <dpranke@chromium.org>
     2
     3        nrwt: make the worker class stand alone with a cleaner interface
     4        https://bugs.webkit.org/show_bug.cgi?id=90409
     5
     6        Reviewed by Ojan Vafai.
     7
     8        Currently the Worker class derives from AbstractWorker, which is
     9        kind of crufty and awkward; it would be better if we did not
     10        rely on shared state.
     11
     12        This change changes that so that Worker derives from object, and
     13        exposes the following interface:
     14          __init__() - called in the manager process
     15          safe_init() - called in the worker process to initialize
     16            unpicklable state
     17          handle() - a single routine to handle all messages
     18          cleanup() - called so the worker can clean up
     19
     20        Also, all of the "administrative" messages that are handled by
     21        the worker (notification of start/stop/etc.) move into
     22        manager_worker_broker - this reduces worker.py to just handling
     23        the mechanics of actually running each test.
     24
     25        For the moment, we do this by creating Yet Another wrapper/proxy
     26        class in manager_worker_broker, but this will get simpler
     27        shortly when the rest of m_w_b is cleaned up.
     28
     29        With this change worker is now in its new form but there will be
     30        a follow-on change that cleans up some names and other minor
     31        things.
     32
     33        This change is again mostly just moving things around and should
     34        be covered by the (updated) existing tests.
     35
     36        * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:
     37        (get):
     38        (AbstractWorker.__init__):
     39        (AbstractWorker.run):
     40        (AbstractWorker):
     41        (AbstractWorker.handle_stop):
     42        (AbstractWorker.handle_test_list):
     43        (AbstractWorker.yield_to_broker):
     44        (AbstractWorker.post_message):
     45        (_WorkerConnection.__init__):
     46        (_Process.run):
     47        * Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py:
     48        (_TestWorker):
     49        (_TestWorker.__init__):
     50        (_TestWorker.name):
     51        (_TestWorker.cleanup):
     52        (_TestWorker.handle):
     53        (_TestWorker.safe_init):
     54        (_TestWorker.stop):
     55        (_TestsMixin.handle_finished_test):
     56        (_TestsMixin.setUp):
     57        (_TestsMixin.test_cancel):
     58        (_TestsMixin.test_done):
     59        * Scripts/webkitpy/layout_tests/controllers/worker.py:
     60        (Worker):
     61        (Worker.__init__):
     62        (Worker.safe_init):
     63        (Worker.handle):
     64
    1652012-07-03  Dirk Pranke  <dpranke@chromium.org>
    266

  • trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py

    r121807 r121809  
    7676
    7777
     78from webkitpy.common.host import Host
    7879from webkitpy.common.system import stack_utils
    7980from webkitpy.layout_tests.views import metered_stream
    … …  
    9798        client - BrokerClient implementation to dispatch
    9899            replies to.
    99         worker_factory: factory method for creatin objects that implement the AbstractWorker interface.
     100        worker_factory: factory method for creating objects that implement the Worker interface.
    100101        host: optional picklable host object that can be passed to workers for testing.
    101102    Returns:
    … …  
    264265class AbstractWorker(BrokerClient):
    265266    def __init__(self, worker_connection, worker_number):
    266         """The constructor should be used to do any simple initialization
    267         necessary, but should not do anything that creates data structures
    268         that cannot be Pickled or sent across processes (like opening
    269         files or sockets). Complex initialization should be done at the
    270         start of the run() call.
    271 
    272         Args:
    273             worker_connection - handle to the _BrokerConnection object creating
    274                 the worker and that can be used for messaging.
    275             worker_number - the number/index of the current worker."""
    276267        BrokerClient.__init__(self)
     268        self.worker = None
    277269        self._worker_connection = worker_connection
    278270        self._worker_number = worker_number
    … …  
    281273        self._canceled = False
    282274        self._options = optparse.Values({'verbose': False})
     275        self.host = None
    283276
    284277    def name(self):
    … …  
    291284        self._done = True
    292285
    293     def run(self):
     286    def run(self, host):
    294287        """Callback for the worker to start executing. Typically does any
    295288        remaining initialization and then calls broker_connection.run_message_loop()."""
    296289        exception_msg = ""
     290        self.host = host
     291
     292        self.worker.safe_init()
     293        _log.debug('%s starting' % self._name)
    297294
    298295        try:
    … …  
    309306        finally:
    310307            _log.debug("%s done with message loop%s" % (self._name, exception_msg))
     308            self.worker.cleanup()
     309            self._worker_connection.post_message('done')
     310
     311    def handle_stop(self, source):
     312        self._done = True
     313
     314    def handle_test_list(self, source, list_name, test_list):
     315        self.worker.handle('test_list', source, list_name, test_list)
    311316
    312317    def cancel(self):
    … …  
    315320        method being called, so clients should not rely solely on this."""
    316321        self._canceled = True
     322
     323    def yield_to_broker(self):
     324        self._worker_connection.yield_to_broker()
     325
     326    def post_message(self, *args):
     327        self._worker_connection.post_message(*args)
    317328
    318329
    … …  
    363374class _WorkerConnection(_BrokerConnection):
    364375    def __init__(self, host, broker, worker_factory, worker_number):
    365         self._client = worker_factory(self, worker_number)
     376        # FIXME: keeping track of the differences between the WorkerConnection, the AbstractWorker, and the
     377        # actual Worker (created by worker_factory) is very confusing, but this all gets better when
     378        # _WorkerConnection and AbstractWorker get merged.
     379        self._client = AbstractWorker(self, worker_number)
     380        self._worker = worker_factory(self._client, worker_number)
     381        self._client.worker = self._worker
    366382        self._host = host
    367383        self._log_messages = []
    … …  
    452468
    453469    def run(self):
     470        if not self._worker_connection._host:
     471            self._worker_connection._host = Host()
    454472        self._worker_connection.run()
    455473

  • trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker_unittest.py

    r121807 r121809  
    5555
    5656
    57 class _TestWorker(manager_worker_broker.AbstractWorker):
    58     def __init__(self, worker_connection, worker_number=1):
    59         super(_TestWorker, self).__init__(worker_connection, worker_number)
     57class _TestWorker(object):
     58    def __init__(self, caller, worker_number):
     59        self._caller = caller
    6060        self._thing_to_greet = 'everybody'
    6161        self._starting_queue = starting_queue
    6262        self._stopping_queue = stopping_queue
    63 
    64     def handle_stop(self, src):
    65         self.stop_handling_messages()
    66 
    67     def handle_test(self, src, an_int, a_str):
     63        self._options = optparse.Values({'verbose': False})
     64
     65    def name(self):
     66        return WORKER_NAME
     67
     68    def cleanup(self):
     69        pass
     70
     71    def handle(self, message, src, an_int, a_str):
    6872        assert an_int == 1
    6973        assert a_str == "hello, world"
    70         self._worker_connection.post_message('test', 2, 'hi, ' + self._thing_to_greet)
    71 
    72     def run(self, host):
     74        self._caller.post_message('finished_test', 2)
     75
     76    def safe_init(self):
    7377        if self._starting_queue:
    7478            self._starting_queue.put('')
    … …  
    7680        if self._stopping_queue:
    7781            self._stopping_queue.get()
    78         try:
    79             super(_TestWorker, self).run()
    80         finally:
    81             self._worker_connection.post_message('done')
     82
     83    def stop(self):
     84        self._caller.post_message('done')
    8285
    8386
    … …  
    106109        self._done = True
    107110
    108     def handle_test(self, src, an_int, a_str):
     111    def handle_finished_test(self, src, an_int, log_messages):
    109112        self._an_int = an_int
    110         self._a_str = a_str
    111113
    112114    def handle_exception(self, src, exception_type, exception_value, stack):
    … …  
    115117    def setUp(self):
    116118        self._an_int = None
    117         self._a_str = None
    118119        self._broker = None
    119120        self._done = False
    … …  
    137138        self.make_broker()
    138139        worker = self._broker.start_worker(1)
    139         self._broker.post_message('test', 1, 'hello, world')
     140        self._broker.post_message('test_list', 1, 'hello, world')
    140141        worker.cancel()
    141142        worker.join(0.1)
    … …  
    146147        self.make_broker()
    147148        worker = self._broker.start_worker(1)
    148         self._broker.post_message('test', 1, 'hello, world')
     149        self._broker.post_message('test_list', 1, 'hello, world')
    149150        self._broker.post_message('stop')
    150151        self._broker.run_message_loop()
    … …  
    153154        self.assertTrue(self.is_done())
    154155        self.assertEqual(self._an_int, 2)
    155         self.assertEqual(self._a_str, 'hi, everybody')
    156156        self._broker.cleanup()
    157157

  • trunk/Tools/Scripts/webkitpy/layout_tests/controllers/worker.py

    r121807 r121809  
    3333import time
    3434
    35 from webkitpy.common.host import Host
    36 from webkitpy.layout_tests.controllers import manager_worker_broker
    3735from webkitpy.layout_tests.controllers import single_test_runner
    3836from webkitpy.layout_tests.models import test_expectations
    … …  
    4341
    4442
    45 class Worker(manager_worker_broker.AbstractWorker):
     43class Worker(object):
    4644    def __init__(self, worker_connection, worker_number, results_directory, options):
    47         super(Worker, self).__init__(worker_connection, worker_number)
     45        self._worker_connection = worker_connection
     46        self._worker_number = worker_number
     47        self._name = 'worker/%d' % worker_number
    4848        self._results_directory = results_directory
    4949        self._options = options
     50
     51        # The remaining fields are initialized in safe_init()
     52        self._host = None
    5053        self._port = None
    5154        self._batch_size = None
    … …  
    6063
    6164    def safe_init(self):
    62         """This method should only be called when it is is safe for the mixin
    63         to create state that can't be Pickled.
    64 
    65         This routine exists so that the mixin can be created and then marshaled
    66         across into a child process."""
    67         self._filesystem = self._port.host.filesystem
     65        """This method is called when it is safe for the object to create state that
     66        does not need to be pickled (usually this means it is called in a child process)."""
     67        self._host = self._worker_connection.host
     68        self._filesystem = self._host.filesystem
     69        self._port = self._host.port_factory.get(self._options.platform, self._options)
     70
    6871        self._batch_count = 0
    6972        self._batch_size = self._options.batch_size or 0
    … …  
    7174        self._tests_run_file = self._filesystem.open_text_file_for_writing(tests_run_filename)
    7275
    73     def run(self, host):
    74         if not host:
    75             host = Host()
    76 
    77         options = self._options
    78         self._port = host.port_factory.get(options.platform, options)
    79 
    80         self.safe_init()
    81         try:
    82             _log.debug("%s starting" % self._name)
    83             super(Worker, self).run()
    84         finally:
    85             self.kill_driver()
    86             _log.debug("%s exiting" % self._name)
    87             self.cleanup()
    88             self._worker_connection.post_message('done')
    89 
    90     def handle_test_list(self, src, list_name, test_list):
     76    def handle(self, name, source, list_name, test_list):
     77        assert name == 'test_list'
    9178        start_time = time.time()
    9279        num_tests = 0
    … …  
    9986        elapsed_time = time.time() - start_time
    10087        self._worker_connection.post_message('finished_list', list_name, num_tests, elapsed_time)
    101 
    102     def handle_stop(self, src):
    103         self.stop_handling_messages()
    10488
    10589    def _update_test_input(self, test_input):
Note: See TracChangeset for help on using the changeset viewer.