Summary: | [Qt][WTR] TestController::platformRunUntil should not do busy waiting | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Balazs Kelemen <kbalazs> | ||||||||||||
Component: | Tools / Tests | Assignee: | Balazs Kelemen <kbalazs> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | hausmann | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Balazs Kelemen
2012-11-06 02:43:05 PST
Created attachment 172552 [details]
Patch
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. 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. Created attachment 173599 [details]
Patch
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. (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. Created attachment 174138 [details]
Patch
Created attachment 174914 [details]
Patch
fixed changelog
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 on attachment 174914 [details]
Patch
I like this approach, but the call to platformDestroy() is missing, as briefly mentioned on IRC :)
Created attachment 175526 [details]
Patch
Comment on attachment 175526 [details] Patch Clearing flags on attachment: 175526 Committed r135496: <http://trac.webkit.org/changeset/135496> All reviewed patches have been landed. Closing bug. |