[webkit-reviews] review granted: [Bug 54071] nrwt multiprocessing: spawn multiple workers : [Attachment 82183] update w/ feedback from tony

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 54071: nrwt multiprocessing: spawn multiple workers
https://bugs.webkit.org/show_bug.cgi?id=54071

Attachment 82183: update w/ feedback from tony
https://bugs.webkit.org/attachment.cgi?id=82183&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82183&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:52
> +    def __init__(self, number, worker):
> +	   self.worker = worker

Should we name this param something other than |worker| to avoid shadowing the
module?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:76
> +    def _worker_is_done(self, worker):
> +	   # FIXME: check if the worker is wedged.
> +	   return worker.done

What about this param?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:120
> +	       # FIXME: If we start workers up too quickly, DumpRenderTree
appears
> +	       # to thrash on something and time out its first few tests. Until

> +	       # we can figure out what's going on, sleep a bit in between
> +	       # workers.
> +	       time.sleep(0.1)

This is fine for now, but it sounds like this could lead to flakiness.	Any
ideas why the old code doesn't have this problem?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:130
> +	   # We post one 'stop' message for each worker. Because the stop
message
> +	   # are sent after all of the tests, and because each worker will stop

> +	   # reading messsages after receiving a stop, we can be sure each
> +	   # worker will get a stop message and hence they will all shut down.
> +	   for i in xrange(num_workers):
> +	       manager_connection.post_message('stop')

This is also fine, but maybe ManagerConnection should have a stop_all_workers
method?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:155
> +    def handle_done(self, source):
> +	   worker = self._workers[source]
> +	   worker.done = True

worker -> worker_state?