Summary: | [CoordGfx] requestAnimationFrame performance issues | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Milian Wolff <milian.wolff> | ||||||||||
Component: | WebKit Qt | Assignee: | 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
Milian Wolff
2013-03-14 07:19:39 PDT
Created attachment 200980 [details]
Patch
Created attachment 201276 [details]
Patch
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. (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 :) Created attachment 201288 [details]
Patch
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. 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() Created attachment 201559 [details]
Patch
Comment on attachment 201559 [details]
Patch
LGTM
(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... 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 :) Comment on attachment 201559 [details] Patch Clearing flags on attachment: 201559 Committed r150015: <http://trac.webkit.org/changeset/150015> All reviewed patches have been landed. Closing bug. This patch appears to have caused the following regression on EFL: http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r150018%20(12196)/fast/animation/request-animation-frame-too-rapid-pretty-diff.html |