|Summary:||[Qt][WTR] TestController::platformRunUntil should not do busy waiting|
|Product:||WebKit||Reporter:||Balazs Kelemen <kbalazs>|
|Component:||Tools / Tests||Assignee:||Balazs Kelemen <kbalazs>|
|Version:||528+ (Nightly build)|
Description Balazs Kelemen 2012-11-06 02:43:05 PST
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.
Comment 2 Balazs Kelemen 2012-11-06 04:51:25 PST
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.
Comment 3 Balazs Kelemen 2012-11-08 07:36:57 PST
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.
Comment 5 Simon Hausmann 2012-11-13 23:53:40 PST
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.
Comment 6 Balazs Kelemen 2012-11-14 03:08:03 PST
(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.
Comment 8 Balazs Kelemen 2012-11-19 01:40:04 PST
Created attachment 174914 [details] Patch fixed changelog
Comment 9 Build Bot 2012-11-19 04:37:06 PST
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
Comment 10 Simon Hausmann 2012-11-21 08:25:21 PST
Comment on attachment 174914 [details] Patch I like this approach, but the call to platformDestroy() is missing, as briefly mentioned on IRC :)
Comment 12 Balazs Kelemen 2012-11-22 01:45:26 PST
Comment on attachment 175526 [details] Patch Clearing flags on attachment: 175526 Committed r135496: <http://trac.webkit.org/changeset/135496>
Comment 13 Balazs Kelemen 2012-11-22 01:45:30 PST
All reviewed patches have been landed. Closing bug.