Bug 80042 – [WK2] run-perf-tests should be able to run with WTR

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

We should be able to run the performance tests with WebKitTestRunner (WebKit 2) and, therefore, WKTR should  support the --no-timeout parameter.

[Zan Dobersek]

Created attachment 129741 [details]
Patch

[Zan Dobersek]

(In reply to comment #1)
> Created an attachment (id=129741) [details]
> Patch

This patch adds a necessary option to PerfTestRunner and does some small refactoring in WebKitTestRunner to avoid timing out, as required by perf tests.

I've tested these changes on Gtk port with success. I've applied the similar logic to the Qt port - it should work, but I haven't tested it.

I'm unfamiliar with the ways Mac and Win ports set up the loops, so I've left a FIXME note in those places. The code should build fine, though.

[Jesus Sanchez-Palencia]

(In reply to comment #2)
> I've tested these changes on Gtk port with success. I've applied the similar logic to the Qt port - it should work, but I haven't tested it.

I'm testing your patch with the Qt port and it works just fine. Thanks!

[Martin Robinson]

Comment on attachment 129741 [details]
Patch

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

Looks fine, in general, but I have a few nits.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:129
> +    bool m_useTimeoutWatchdog;

nit: m_useWaitToDumpWatchdogTimer?

> Tools/WebKitTestRunner/TestController.cpp:546
> -void TestController::runUntil(bool& done, TimeoutDuration timeoutDuration)
> +void TestController::runUntil(bool& done, TimeoutDuration timeoutDuration, bool shouldTimeout)
>  {
> -    platformRunUntil(done, timeoutDuration == ShortTimeout ? m_shortTimeout : m_longTimeout);
> +    platformRunUntil(done, timeoutDuration == ShortTimeout ? m_shortTimeout : m_longTimeout, shouldTimeout);

Instead of adding an extra parameter, perhaps it would be better to add another value to the enum so there is ShortTimeout, LongTimeout and NoTimeout.

> Tools/WebKitTestRunner/TestController.h:74
> +    void platformRunUntil(bool& done, double timeout, bool shouldTimeout);

Instead of adding another parameter, perhaps you could just have a constant == -1 that signifies no timeout. Another option would be to add another calld platformRun that didn't timeout.

> Tools/WebKitTestRunner/TestInvocation.cpp:153
> +    WKRetainPtr<WKStringRef> useTimeoutWatchdogKey = adoptWK(WKStringCreateWithUTF8CString("UseTimeoutWatchdog"));
> +    WKRetainPtr<WKBooleanRef> useTimeoutWatchdogValue = adoptWK(WKBooleanCreate(TestController::shared().shouldTimeout()));
> +    WKDictionaryAddItem(beginTestMessageBody.get(), useTimeoutWatchdogKey.get(), useTimeoutWatchdogValue.get());

Probably better to refer to this thing consistently ala useWaitToDumpWatchdogTimer.

> Tools/WebKitTestRunner/mac/TestControllerMac.mm:56
> +    // FIXME: No timeout should occur if shouldTimeout is false (necessary when running performance tests).

It seems quite possible to add support here.

> Tools/WebKitTestRunner/win/TestControllerWin.cpp:175
> +    // FIXME: No timeout should occur if shouldTimeout is false (necessary when running performance tests).

This one seems a bit more complicated, so if you don't add Windows support, be sure to open a bug for it and CC the appropriate people.

[Zan Dobersek]

Comment on attachment 129741 [details]
Patch

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

>> Tools/WebKitTestRunner/TestController.cpp:546
>> +    platformRunUntil(done, timeoutDuration == ShortTimeout ? m_shortTimeout : m_longTimeout, shouldTimeout);
> 
> Instead of adding an extra parameter, perhaps it would be better to add another value to the enum so there is ShortTimeout, LongTimeout and NoTimeout.

Noted.

>> Tools/WebKitTestRunner/TestController.h:74
>> +    void platformRunUntil(bool& done, double timeout, bool shouldTimeout);
> 
> Instead of adding another parameter, perhaps you could just have a constant == -1 that signifies no timeout. Another option would be to add another calld platformRun that didn't timeout.

platformRun would probably introduce some code duplication, so I'll just use a negative constant to indicate no timeout and check for that.

>> Tools/WebKitTestRunner/TestInvocation.cpp:153
>> +    WKDictionaryAddItem(beginTestMessageBody.get(), useTimeoutWatchdogKey.get(), useTimeoutWatchdogValue.get());
> 
> Probably better to refer to this thing consistently ala useWaitToDumpWatchdogTimer.

I'll just put (u/U)seWaitToDumpWatchdogTimer everywhere - consistency at its best.

>> Tools/WebKitTestRunner/mac/TestControllerMac.mm:56
>> +    // FIXME: No timeout should occur if shouldTimeout is false (necessary when running performance tests).
> 
> It seems quite possible to add support here.

I'm not familiar enough with Objective C or Cocoa to be comfortable making these changes. I can give it a shot, but I would rather open a bug for this problem, as you suggested doing for the Windows' TestController.

[Zan Dobersek]

Created attachment 130183 [details]
Patch

[Martin Robinson]

Comment on attachment 130183 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:558
> +        timeout = -1.0;
> +        break;
> +    }

Sorry, I should have been a bit clearer in my review comment. I think it makes sense to have a constant, perhaps a static member variable like:

TestController:s_noTimeoutDuration = -1;

I can change this and land it.

[Martin Robinson]

Committed r110382: <http://trac.webkit.org/changeset/110382>

[Zan Dobersek]

(In reply to comment #8)
> Committed r110382: <http://trac.webkit.org/changeset/110382>

Thanks for the small changes and landing.

Posted two new bugs for Mac and Win ports, #80780 and #80781.