Bug 50367 – nrwt multiprocessing - clean up dump_render_tree_thread.py

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

[Dirk Pranke]

new-run-webkit-tests: clean up dump_render_tree_thread.py

[Dirk Pranke]

Created attachment 75347 [details]
Patch

[Tony Chang]

Comment on attachment 75347 [details]
Patch

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

These refactorings all seem reasonable.  I would have been able to review this faster if it were in smaller parts.  In its current form, there was lots of scrolling up and down.  Please fix the comments before committing.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:71
> -        self._canceled = False
> +        self._cancelled = False

According to the internet, cancelled is the British spelling and canceled is the American spelling.  Doesn't seem worthwhile to change it here.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:244
> +    def _cleanup(self):

Should __del__ call _cleanup?  Looks like self._tests_run_file won't get cleaned up if you don't call run().

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:270
> +        self._test_list_timing_stats[list_name] = \
> +           (num_tests, elapsed_time)

Nit: If you put the opening ( on the previous line, you don't need the line continuation \.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:285
> +        driver_timeout = 3.0 * float(test_input.timeout) / 1000.0
> +        thread_padding = 1.0
> +        thread_timeout = driver_timeout + thread_padding

Nit: It would be nice if the variables had time units in their name (e.g., driver_timeout_sec and thread_timeout_sec).

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:333
> +            def run(self):
> +                result = worker._run_single_test(driver, test_input)

I think you can do:
def run(self, worker=self):

And avoid having to set worker = self above.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:471
> +        if self._batch_size > 0 and self._batch_count >= self._batch_size:

Nit: self._batch_count >= self._batch_size > 0

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 75347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75347&action=review
> 
> These refactorings all seem reasonable.  I would have been able to review this faster if it were in smaller parts.  In its current form, there was lots of scrolling up and down.  Please fix the comments before committing.
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:71
> > -        self._canceled = False
> > +        self._cancelled = False
> 
> According to the internet, cancelled is the British spelling and canceled is the American spelling.  Doesn't seem worthwhile to change it here.
> 

Okay. I thought it was just misspelled. I guess I must be British. I'll change it back.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:244
> > +    def _cleanup(self):
> 
> Should __del__ call _cleanup?  Looks like self._tests_run_file won't get cleaned up if you don't call run().
>

Good idea.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:270
> > +        self._test_list_timing_stats[list_name] = \
> > +           (num_tests, elapsed_time)
> 
> Nit: If you put the opening ( on the previous line, you don't need the line continuation \.
>

Hm. I would fear that it would confuse using () for breaking up expressions with using () for creating tuples. Maybe I'll just go over 80 cols ;)
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:285
> > +        driver_timeout = 3.0 * float(test_input.timeout) / 1000.0
> > +        thread_padding = 1.0
> > +        thread_timeout = driver_timeout + thread_padding
> 
> Nit: It would be nice if the variables had time units in their name (e.g., driver_timeout_sec and thread_timeout_sec).
>

Will do.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:333
> > +            def run(self):
> > +                result = worker._run_single_test(driver, test_input)
> 
> I think you can do:
> def run(self, worker=self):
> 
> And avoid having to set worker = self above.
> 

Interesting. I had to think about that for quite a while to see why that would work. I think I'll leave the convention of run() not taking any arguments alone and rely on lexical scoping.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:471
> > +        if self._batch_size > 0 and self._batch_count >= self._batch_size:
> 
> Nit: self._batch_count >= self._batch_size > 0

Aaah. My eyes! I didn't know you could do that w/ Python.

Apparently 1 > 2 > 3 evaluates to False but 1 > (2 > 3) evaluates to True (not too surprising, but still something of a obscurity). Given that no other language that people are commonly used to works this way, I think I'll avoid this construct.

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 75347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75347&action=review
> 
> These refactorings all seem reasonable.  I would have been able to review this faster if it were in smaller parts.  In its current form, there was lots of scrolling up and down.  Please fix the comments before committing.
> 

Yes, this is true. On the other hand, having five patches in flight at once was enough for one day ;)

In retrospect, a lot of this cleanup should've been completely outside of the rest of this set of patches, so I will try to to better in the future.

Thanks for the review!

[Dirk Pranke]

Created attachment 75421 [details]
update w/ review feedback

[Dirk Pranke]

Committed r73211: <http://trac.webkit.org/changeset/73211>

[Yuta Kitamura]

Hi,

This change seemed to break Chromium "Webkit Win (dbg)(2)" bot, so I rolled it out.

After this patch was merged, the bot's output started to contain a LOT of messages such as:

2010-12-03 03:17:20,342 14092 message_broker.py:134 ERROR worker/3 (tid 7996) is wedged
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\depot_tools\python_bin\lib\threading.py", line 499, in __bootstrap
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   self.__bootstrap_inner()
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\depot_tools\python_bin\lib\threading.py", line 527, in __bootstrap_inner
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   self.run()
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 198, in run
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   self._covered_run()
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 207, in _covered_run
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   self._run(test_runner=None, result_summary=None)
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 240, in _run
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   self.handle_test_list(current_group, filename_list)
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 264, in handle_test_list
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   self._run_test(test_input)
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 303, in _run_test
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   result = self._run_test_in_this_thread(test_input)
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 376, in _run_test_in_this_thread
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   test_result = self._run_single_test(test_input, self._driver)
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 388, in _run_single_test
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   test_output = driver.run_test(test_input)
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\chromium.py", line 525, in run_test
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   (line, crash) = self._write_command_and_read_line(input=None)
2010-12-03 03:17:20,342 14092 message_broker.py:149 ERROR File: "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\chromium.py", line 434, in _write_command_and_read_line
2010-12-03 03:17:20,342 14092 message_broker.py:151 ERROR   line = self._proc.stdout.readline()


To roll out this patch, I had to roll out r73222, r73228 and r73231 because there was some conflict and I did not understand how to resolve. Sorry for all the mess I caused, and I deeply appreciate your understandings to keep our bots in a healthy state.

[Yuta Kitamura]

After this change was reverted, the bot recovered and became stable for now.

[Dirk Pranke]

Interesting. This patch shouldn't have changed any functionality. I will look into it.

As for rolling back the others, no problem. You had to roll them all out if you need to roll out this.

[Dirk Pranke]

This patch landed downstream in r68127, and yutak rolled it back in r68163. The buildbot logs for this bot can be found on this page:

http://build.chromium.org/p/chromium/waterfall?builder=Webkit%20Win%20(dbg)(2)&ast_time=1291393636

this corresponds to builds 2182 - 2197 on that bot.

[Dirk Pranke]

all of the debug windows bots seem to wedge far more consistently with this patch. Win Release may be slightly more frequent. Linux doesn't wedge at all, and Mac won't since we only run 1 thread.

[Dirk Pranke]

closing ... the new patches will be different.