[webkit-reviews] review denied: [Bug 54071] nrwt multiprocessing: spawn multiple workers : [Attachment 81762] reset state properly in _run_tests()

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied 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 81762: reset state properly in _run_tests()
https://bugs.webkit.org/attachment.cgi?id=81762&action=review

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:109
> +	       w = manager_connection.start_worker(worker_number)
> +	       ws = _WorkerState(worker_number, w)
> +	       self._workers[w.name] = ws

w -> worker, ws -> worker_state

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:113
> +	       # We sleep a bit in between workers to give the processes a
chance
> +	       # to start up without thrashing.
> +	       time.sleep(0.1)

This seems prone to a race.  What type of thrashing happens if you don't sleep?


> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:119
> -	   manager_connection.post_message('stop')
> +	   for i in xrange(num_workers):
> +	       manager_connection.post_message('stop')

Why do we have to post multiple stop messages?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:144
> +	   for w in self._workers.values():
> +	       if not w.done:
> +		   done = False

Can you use any() + list comprehension here?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:147
> +	   # FIXME: is the following line safe?
> +	   # self._done = done

Can you elaborate on this?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:150
>      def handle_started_test(self, src, test_info, hang_timeout):

src -> source

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:156
> +	   w = self._workers[src]
> +	   w.done = True

w -> worker, src -> source

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner2.py:158
>      def handle_exception(self, src, exception_info):

src -> source