[Qt][WTR] TestController::platformRunUntil should not do busy waiting

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Balazs Kelemen]

My problem is that WebKitTestRunner use too much CPU so it really overloads my system. I believe one reason is that the current implementation of platformRunUntil is actually busy waiting for events. Until an event triggers it is continously calling into processEvents (without the WaitForMoreEvents flag). Instead we should allow waiting for events.

[Balazs Kelemen]

Created attachment 172552 [details]
Patch

[Balazs Kelemen]

Here is some data about the effect.

test: time run-webkit-tests -2 --no-show-results --no-retry-failures --no-new-test-results LayoutTests/compositing

ref:
real    9m33.580s
user    4m40.298s
sys     3m55.207s

patched:
real    0m27.776s
user    0m7.044s
sys     0m6.040s

Note that this is an extreme situation, somehow the compositing tests shows the affect much more than others (currently), the affect is not always that much. Btw I repeated this test once again and the results were the same so I'm quite sure 
it's not about some variance of the workload of my system.

[Balazs Kelemen]

I found a single example where the difference is huge: LayoutTests/compositing/overflow/overflow-positioning.html. Honestly I don't why this test is so slow with the current runUntil implementations while others are not.

[Balazs Kelemen]

Created attachment 173599 [details]
Patch

[Simon Hausmann]

Comment on attachment 173599 [details]
Patch

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

> Tools/WebKitTestRunner/qt/TestControllerQt.cpp:62
> +    QTimer timer;

This could also be implemented using a QBasicTimer, removing the need for Q_OBJECT and moc inclusion.

> Tools/WebKitTestRunner/qt/TestControllerQt.cpp:67
> +Q_GLOBAL_STATIC(RunLoopContext, g_loopContext);

I still think that it would be cleaner if this was a member variable of TestController instead.

[Balazs Kelemen]

(In reply to comment #5)
> (From update of attachment 173599 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173599&action=review
> 
> > Tools/WebKitTestRunner/qt/TestControllerQt.cpp:62
> > +    QTimer timer;
> 
> This could also be implemented using a QBasicTimer, removing the need for Q_OBJECT and moc inclusion.

How? I still need a QObject with a custom timerEvent override if I use a QBasicTimer, no?

> 
> > Tools/WebKitTestRunner/qt/TestControllerQt.cpp:67
> > +Q_GLOBAL_STATIC(RunLoopContext, g_loopContext);
> 
> I still think that it would be cleaner if this was a member variable of TestController instead.

Ok, than let's do another round.

[Balazs Kelemen]

Created attachment 174138 [details]
Patch

[Balazs Kelemen]

Created attachment 174914 [details]
Patch

fixed changelog

[Build Bot]

Comment on attachment 174914 [details]
Patch

Attachment 174914 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14908160

New failing tests:
inspector/profiler/heap-snapshot.html

[Simon Hausmann]

Comment on attachment 174914 [details]
Patch

I like this approach, but the call to platformDestroy() is missing, as briefly mentioned on IRC :)

[Balazs Kelemen]

Created attachment 175526 [details]
Patch

[Balazs Kelemen]

Comment on attachment 175526 [details]
Patch

Clearing flags on attachment: 175526

Committed r135496: <http://trac.webkit.org/changeset/135496>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.