Bug 76612 - Some perf tests time out when ran by run-perf-tests
Summary: Some perf tests time out when ran by run-perf-tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 74649
Blocks: 29229
  Show dependency treegraph
 
Reported: 2012-01-19 01:57 PST by Ryosuke Niwa
Modified: 2012-01-19 15:22 PST (History)
8 users (show)

See Also:


Attachments
fixes the bug (34.92 KB, patch)
2012-01-19 03:51 PST, Ryosuke Niwa
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-01-19 01:57:05 PST
Fix perf tests to be ran by run-perf-tests
Comment 1 Ryosuke Niwa 2012-01-19 03:51:22 PST
Created attachment 123099 [details]
fixes the bug
Comment 2 Dirk Pranke 2012-01-19 11:56:23 PST
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?
Comment 3 Eric Seidel (no email) 2012-01-19 11:58:35 PST
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...
Comment 4 Ryosuke Niwa 2012-01-19 11:59:02 PST
(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.
Comment 5 Eric Seidel (no email) 2012-01-19 12:00:24 PST
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?
Comment 6 Dirk Pranke 2012-01-19 12:01:42 PST
(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).
Comment 7 Eric Seidel (no email) 2012-01-19 12:02:17 PST
See also my attempt at this in bug 74649.
Comment 8 Eric Seidel (no email) 2012-01-19 12:02:46 PST
Comment on attachment 123099 [details]
fixes the bug

I dont' want to change html5.html unless we have to.
Comment 9 Ryosuke Niwa 2012-01-19 12:02:58 PST
(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://
Comment 10 Eric Seidel (no email) 2012-01-19 12:24:19 PST
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?)
Comment 11 Ryosuke Niwa 2012-01-19 12:29:47 PST
(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.
Comment 12 Dirk Pranke 2012-01-19 12:31:24 PST
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).
Comment 13 Ryosuke Niwa 2012-01-19 12:34:03 PST
(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.
Comment 14 Ryosuke Niwa 2012-01-19 13:50:58 PST
Committed r105443: <http://trac.webkit.org/changeset/105443>
Comment 15 Csaba Osztrogonác 2012-01-19 14:29:47 PST
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?
Comment 16 Ryosuke Niwa 2012-01-19 15:22:01 PST
Fixed the regression in http://trac.webkit.org/changeset/105450.