Bug 112345

Summary: [CoordGfx] requestAnimationFrame performance issues
Product: WebKit Reporter: Milian Wolff <milian.wolff>
Component: WebKit QtAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cmarcelo, commit-queue, helder.correia, jturcotte, luiz, noam, rafael.lobo, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 116055    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Milian Wolff
Reported 2013-03-14 07:19:39 PDT
See also: https://lists.webkit.org/pipermail/webkit-qt/2013-March/003546.html Short version is: window.requestAnimationFrame has performance issues on QtWebKit and performs worse than a simple window.setTimeout approach. Code to reproduce the issue: ``` test.qml ``` import QtQuick 2.0 import QtWebKit 3.0 import QtWebKit.experimental 1.0 WebView { id: webView height: 500 width: 500 url: "test.html" experimental { preferences.developerExtrasEnabled: true } } `````` ``` test.html ``` <DOCTYPE html> <html> <head> <title>rAF test</title> <script type="text/javascript"> function animateRAF() { window.requestAnimationFrame(animateRAF); } function animateTimer() { window.setTimeout(17, animateTimer) } // abysmal performance window.onload = animateRAF; // nice performance // window.onload = animateTimer; </script> <body> <h1>rAF test</h1> </body> </html> `````` The animateRAF version above makes my system noticeably sluggish and I see a high CPU load: PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND 25016 milian 20 0 2578m 55m 36m S 54.9 0.7 0:10.93 qmlscene 312 root 20 0 206m 92m 65m S 48.2 1.2 8:41.84 X 25024 milian 30 10 1827m 48m 38m S 12.0 0.6 0:01.89 QtWebProcess Using animateTimer instead I do not see any of the above processed in the top 20 of top at all. I am using a git checkout of WebKit master with bd8ce398ed4dd2ce92f8db61d7e97b43a534297a, i.e. https://bugs.webkit.org/show_bug.cgi?id=112095 applied. That helped a bit but rAF is still unusable for me. perf top -G, while running the rAF version of the above, shows this: - 46.09% [kernel] [k] ioread32 ◆ - ioread32 ▒ - 53.13% 0xffffffffa05e80f6 ▒ - 68.79% nouveau_gem_ioctl_pushbuf ▒ 0xffffffffa02ee3f3 ▒ do_vfs_ioctl ▒ sys_ioctl ▒ system_call_fastpath ▒ __GI___ioctl ▒ - 31.21% nv84_fence_sync ▒ nouveau_fence_sync ▒ validate_sync.isra.3 ▒ validate_list ▒ nouveau_gem_ioctl_pushbuf ▒ drm_ioctl ▒ do_vfs_ioctl ▒ sys_ioctl ▒ system_call_fastpath ▒ __GI___ioctl ▒ 34.81% _nouveau_gpuobj_rd32 ▒ nv84_fence_read ▒ nouveau_fence_done ▒ 9.60% nouveau_dma_wait ▒ 1.17% nv50_crtc_cursor_set ▒ drm_mode_cursor_ioctl ▒ drm_ioctl ▒ do_vfs_ioctl ▒ sys_ioctl ▒ system_call_fastpath ▒ __GI___ioctl ▒ 0.88% nv50_vm_flush I.e. the painting seems to be an issue. So apparently rAF triggers excessive repaints. Is this maybe also a driver problem of noveau?
Attachments
Patch (11.70 KB, patch)
2013-05-07 14:52 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (13.86 KB, patch)
2013-05-09 14:10 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (13.97 KB, patch)
2013-05-09 15:17 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (13.97 KB, patch)
2013-05-13 07:05 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Noam Rosenthal
Comment 1 2013-05-07 14:52:00 PDT
Created attachment 200980 [details] Patch
Noam Rosenthal
Comment 2 2013-05-09 14:10:55 PDT
Created attachment 201276 [details] Patch
Rafael Brandao
Comment 3 2013-05-09 14:21:25 PDT
Comment on attachment 201276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201276&action=review Very nice patch! > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:436 > + m_lastAnimationServiceTime = WTF::monotonicallyIncreasingTime(); I think you could pass m_lastAnimationServiceTime as parameter to serviceScriptedAnimations, since in practice we are doing the same work twice.
Noam Rosenthal
Comment 4 2013-05-09 14:31:02 PDT
(In reply to comment #3) > (From update of attachment 201276 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201276&action=review > > Very nice patch! > > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:436 > > + m_lastAnimationServiceTime = WTF::monotonicallyIncreasingTime(); > > I think you could pass m_lastAnimationServiceTime as parameter to serviceScriptedAnimations, since in practice we are doing the same work twice. Right, of course :)
Noam Rosenthal
Comment 5 2013-05-09 15:17:34 PDT
Created attachment 201288 [details] Patch
Jocelyn Turcotte
Comment 6 2013-05-13 04:53:28 PDT
Comment on attachment 201288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201288&action=review > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:664 > + m_layerFlushTimer.startOneShot(std::max<double>(0., WTF::monotonicallyIncreasingTime() + MinimalTimeoutForAnimations - m_lastAnimationServiceTime)); If we're 5ms into the frame, I think the timer should be scheduled for 11ms to reach the target, but that's not what we'll get here. Example: WTF::monotonicallyIncreasingTime() = 37, MinimalTimeoutForAnimations = 16, m_lastAnimationServiceTime = 32. WTF::monotonicallyIncreasingTime() + MinimalTimeoutForAnimations - m_lastAnimationServiceTime = 37 + 16 - 32 = 21 So maybe it should be: MinimalTimeoutForAnimations - (WTF::monotonicallyIncreasingTime() - m_lastAnimationServiceTime) = 16 - (37 - 32) = 11 I'm also not sure if this can be lower than zero assuming that (WTF::monotonicallyIncreasingTime() >= m_lastAnimationServiceTime) always holds.
Noam Rosenthal
Comment 7 2013-05-13 05:13:27 PDT
Comment on attachment 201288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201288&action=review >> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:664 >> + m_layerFlushTimer.startOneShot(std::max<double>(0., WTF::monotonicallyIncreasingTime() + MinimalTimeoutForAnimations - m_lastAnimationServiceTime)); > > If we're 5ms into the frame, I think the timer should be scheduled for 11ms to reach the target, but that's not what we'll get here. > Example: > WTF::monotonicallyIncreasingTime() = 37, MinimalTimeoutForAnimations = 16, m_lastAnimationServiceTime = 32. > WTF::monotonicallyIncreasingTime() + MinimalTimeoutForAnimations - m_lastAnimationServiceTime = 37 + 16 - 32 = 21 > So maybe it should be: MinimalTimeoutForAnimations - (WTF::monotonicallyIncreasingTime() - m_lastAnimationServiceTime) = 16 - (37 - 32) = 11 > > I'm also not sure if this can be lower than zero assuming that (WTF::monotonicallyIncreasingTime() >= m_lastAnimationServiceTime) always holds. Oops, it should be + MinimalTimeoutForAnimations + m_lastAnimationServiceTime - WTF::monotonicallyIncreasingTime()
Noam Rosenthal
Comment 8 2013-05-13 07:05:50 PDT
Created attachment 201559 [details] Patch
Jocelyn Turcotte
Comment 9 2013-05-13 07:36:29 PDT
Comment on attachment 201559 [details] Patch LGTM
Noam Rosenthal
Comment 10 2013-05-13 07:43:17 PDT
(In reply to comment #9) > (From update of attachment 201559 [details]) > LGTM Thanks! Based on the webkit summit we're allowed to r+ patches that touch only port-specific directories (including CoordinatedGraphics) without WK2 owner review. Guess we're still waiting for the email about it from Sam...
Jocelyn Turcotte
Comment 11 2013-05-13 07:59:17 PDT
Comment on attachment 201559 [details] Patch (In reply to comment #10) > > LGTM > Thanks! Based on the webkit summit we're allowed to r+ patches that touch only port-specific directories (including CoordinatedGraphics) without WK2 owner review. Guess we're still waiting for the email about it from Sam... Ok, that's good to know :)
WebKit Commit Bot
Comment 12 2013-05-13 08:28:27 PDT
Comment on attachment 201559 [details] Patch Clearing flags on attachment: 201559 Committed r150015: <http://trac.webkit.org/changeset/150015>
WebKit Commit Bot
Comment 13 2013-05-13 08:28:30 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.