Bug 76612 – Some perf tests time out when ran by run-perf-tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

Fix perf tests to be ran by run-perf-tests

[Ryosuke Niwa]

Created attachment 123099 [details]
fixes the bug

[Dirk Pranke]

Comment on attachment 123099 [details]
fixes the bug

The change looks fine, but do we really need/want no_timeout? Isn't using a really long timeout like you've changed good enough?

[Eric Seidel (no email)]

Comment on attachment 123099 [details]
fixes the bug

No, I don't like this change.  I don't like modifying the html5 spec, if we don't have to.  DRT normally blocks external loads...

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 123099 [details])
> The change looks fine, but do we really need/want no_timeout? Isn't using a really long timeout like you've changed good enough?

Yes. Otherwise DRT's watchdog timer will fail and DRT times out.

[Eric Seidel (no email)]

Comment on attachment 123099 [details]
fixes the bug

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

> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:291
> +        self.assertEquals(driver.cmd_line(), ['MOCK output of child process/DumpRenderTree', '--pixel-tests', '--no-timeout', '-'])

What ports support --no-timeout?

[Dirk Pranke]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 123099 [details] [details])
> > The change looks fine, but do we really need/want no_timeout? Isn't using a really long timeout like you've changed good enough?
> 
> Yes. Otherwise DRT's watchdog timer will fail and DRT times out.

Which DRT? The Mac DRT, or the Chromium DRT? If it's the Mac DRT, that seems like a bug that should be fixed (it should be taking the passed in timeout), but that doesn't have to block this (i.e., we can land this as-is, and then file a bug to fix the Mac DRT and remove the no_timeout later).

[Eric Seidel (no email)]

See also my attempt at this in bug 74649.

[Eric Seidel (no email)]

Comment on attachment 123099 [details]
fixes the bug

I dont' want to change html5.html unless we have to.

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 123099 [details])
> No, I don't like this change.  I don't like modifying the html5 spec, if we don't have to.  DRT normally blocks external loads...

It does block and spits out a hundreds of errors about it. This patch removes those error messages. Also, isn't it better to load actual images as supposed to being blocked? e.g. if you open the spec on Safari/Chrome, those external images are loaded and affects the layout/loader behavior. My intention was to make the test's behavior on Safari/Chrome and DRT as close as possible. An alternatively we can make local copies of these images into our repository under images.whatwg.org and remove http://

[Eric Seidel (no email)]

Comment on attachment 123099 [details]
fixes the bug

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

I think this is OK, after further discussion over IRC.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:452
> +        Driver.__init__(self, port, worker_number, pixel_tests, no_timeout)

I thought we got the "options" element off the port object?  (I'm not saying that's necessarily a good thing,b ut I thought that was the status quo?)

[Ryosuke Niwa]

(In reply to comment #10)
> (From update of attachment 123099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123099&action=review
> 
> I think this is OK, after further discussion over IRC.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:452
> > +        Driver.__init__(self, port, worker_number, pixel_tests, no_timeout)
> 
> I thought we got the "options" element off the port object?  (I'm not saying that's necessarily a good thing,b ut I thought that was the status quo?)

It appears that port's options object is passed down from the command line argument so I didn't think sticking a random argument there was a good idea. Of course, we could make --no-timeout a command line argument and set it by default but there's not much use in run-perf-tests if no_timeout was False since DRT ends up timing out in a number of tests (2-3 on my MacPro but probably more on slower machines or on slower DRTs).

But I can change if you strongly feel about it. FWIW, it makes my patch smaller since I won't have to pass no_timeout argument everywhere.

[Dirk Pranke]

Comment on attachment 123099 [details]
fixes the bug

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

>> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:452
>> +        Driver.__init__(self, port, worker_number, pixel_tests, no_timeout)
> 
> I thought we got the "options" element off the port object?  (I'm not saying that's necessarily a good thing,b ut I thought that was the status quo?)

We do, normally, but unless I'm missing something there is no options.no_timeout, so this is correct. (Or we could add one, but I'd rather see us just fix DRTs to take arbitrarily long timeouts).

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 123099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123099&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:452
> >> +        Driver.__init__(self, port, worker_number, pixel_tests, no_timeout)
> > 
> > I thought we got the "options" element off the port object?  (I'm not saying that's necessarily a good thing,b ut I thought that was the status quo?)
> 
> We do, normally, but unless I'm missing something there is no options.no_timeout, so this is correct. (Or we could add one, but I'd rather see us just fix DRTs to take arbitrarily long timeouts).

DRT DOES take arbitrarily long timeouts. It's just that some DRTs have "watchdog" timer as well, and --no-timeout will disable that.

[Ryosuke Niwa]

Committed r105443: <http://trac.webkit.org/changeset/105443>

[Csaba Osztrogonác]

It broke NRWT on GTK and on Qt:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r105444%20%2816569%29/results.html

Could you fix it?

[Ryosuke Niwa]

Fixed the regression in http://trac.webkit.org/changeset/105450.