Bug 64851 – Optimize AnimationController::updateAnimations

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[zeng huiqing]

For loading page w/ transition property(CSS3), FF4 ~5.5X faster than Chromium.

Through analysis, we find that the root cause is that w/ transition, when a dom element is attached, it will call RenderObject::setAnimatableStyle, then AnimationController::updateAnimations, and it will call AnimationController::updateAnimationTimer, which is the hotspot.

[zeng huiqing]

Created attachment 101436 [details]
Optimize AnimationController::updateAnimations

In my opinion, in fact, we may do not need to call updateAnimationTimer in the process of "Attach" when loading page.

Has submit a patch, the patch is to add flag in function setAnimatableStyle() and updateAnimations() to avoid calling updateAnimationTimer when doing "Attachment". The patch will reduce page(with transition property) loading time by 70%, improve the performance by 3.3X.

[WebKit Review Bot]

Comment on attachment 101436 [details]
Optimize AnimationController::updateAnimations

Attachment 101436 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9192186

New failing tests:
animations/animation-end-event-short-iterations.html

[zeng huiqing]

Created attachment 101557 [details]
newPatch

new patch
now pass the test animations/animation-end-event-short-iterations.html

[Dean Jackson]

Comment on attachment 101557 [details]
newPatch

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

In general I don't like that this adds the parameter to setAnimatableStyle. Is there way the RenderObject can know if it is attached to the tree so that we don't have to pass the flag around? From a brief investigation this path is only hit when the RenderObject is not in the render tree (it shouldn't have a parent) - is this correct?

> Source/WebCore/ChangeLog:3
> +        [Bug 64851][chromium]Optimize AnimationController::updateAnimations

You should give this a better title. There is no need for [Bug 64851] and this isn't specific to [chromium].

> Source/WebCore/ChangeLog:7
> +

Similarly, you should put a description here of what the problem was and what you fixed.

> Source/WebCore/page/animation/AnimationController.cpp:479
> +PassRefPtr<RenderStyle> AnimationController::updateAnimations(RenderObject* renderer, RenderStyle* newStyle, bool InAttach)

The parameter name is pretty unclear to me. It should describe what it is doing within the function itself, not what the caller is using it for. Does that make sense? I mean that it should be something like timerShouldUpdate because that's how the updateAnimations function is using it.

Also, a style issue, the parameter name should not start with an uppercase character.

[Simon Fraser (smfr)]

Comment on attachment 101557 [details]
newPatch

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

> Source/WebCore/dom/NodeRenderingContext.cpp:287
> +    newRenderer->setAnimatableStyle(m_context.releaseStyle(), true); // setAnimatableStyle() can depend on renderer() already being set.

We try to avoid boolean parameters because they are hard to read at the call site, like this. Enums are preferred.

[zeng huiqing]

Many thanks for helping review the patch and the good comments:)

> In general I don't like that this adds the parameter to setAnimatableStyle. Is there way the RenderObject can know if it is attached to the tree so that we don't have to pass the flag around? From a brief investigation this path is only hit when the RenderObject is not in the render tree (it shouldn't have a parent) - is this correct?


Through debugging, I found there exist another two call paths for RenderObject::setAnimatableStyle():
1)
WebKitClientImpl::DoTimeout()->ThreadTimers::sharedTimerFiredInternal()->Timer<AnimationControllerPrivate>::fired()->AnimationControllerPrivate::animationTimerFired()->AnimationControllerPrivate::updateAnimationTimer()[or AnimationControllerPrivate::fireEventsAndUpdateStyle]->Document::updateStyleIfNeeded()->Element::recalcStyle()->Node::setRenderStyle()->RenderObject::setAnimatableStyle()

2)
StyleElement::createSheet()->...->Document::styleSelectorChanged()->Element::recalcStyle()->Node::setRenderStyle()->RenderObject::setAnimatableStyle()

These two call paths both happen before "Attach", and I am sorry I am not sure whether can remove updateAnimationTimer() in these two paths when loading page w/ only transition property, so I add parameter to setAnimatableStyle().

And through profiling, we can find that the function AnimationController::updateAnimationTimer() doing in the process of "Attach" will consume a lot of time when loading page w/ transition property, especially when the page has many RenderObjects w/ transition property.

I will update the patch according to your good comments, thank you very much:).

[zeng huiqing]

> These two call paths both happen before "Attach", and I am sorry I am not sure whether can remove updateAnimationTimer() in these two paths when loading page w/ only transition property, so I add parameter to setAnimatableStyle().

Sorry for the wrong comments, these two call paths both happen after "Attach", like ':hover' or ':active' will invoke these two call path, so add parameter to setAnimatableStyle() to judge whether or not to invoke updateAnimationTimer().

[zeng huiqing]

Created attachment 101705 [details]
newPatch

use 'renderer->parent()' in function updateAnimations() to avoid calling updateAnimationTimer in the process of "attached" when loading page with only CSS3 transition property but no animation property.

[zeng huiqing]

Can anyone possibly help review the patch? Thanks very much:)

[zeng huiqing]

For page loading test, find that if load a page w/ CSS3 transition property, especially when page contains many RenderObjects w/ transition property, FF4 performs ~5.5X faster than Chromium&Safari.

Through analysis, find that the root cause is that w/ transition property, when a dom element is attached, it will call RenderObject::setAnimatableStyle, then AnimationController::updateAnimations, and it will call AnimationController::updateAnimationTimer, which is the hotspot.

And, in my opinion, we may do not need to call updateAnimationTimer() in the process of "Attach" when loading page w/ only CSS3 transition property actually.

Had submit a patch,use 'renderer->parent()' in function updateAnimations() to avoid calling updateAnimationTimer() in the process of "attached" when loading page with only CSS3 transition property but no animation property.
The patch will reduce page(with transition property) loading time by 70%, improve the performance by 3.3X.

Could someone possibly help review the patch when have a moment? Thanks very much:)

[Dean Jackson]

Comment on attachment 101705 [details]
newPatch

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

You'll have to wait until an official reviewer gives the r+. The two most familiar with the animation code are Simon and Chris, who are on vacation until Monday. I will try to find someone sooner.

Otherwise, this looks good. Thanks!

> Source/WebCore/ChangeLog:3
> +        Fix for bug 64851 - No need to call updateAnimationTimer() in the process of "Attach" when loading page with only CSS3 transition property but no animation property.

I think the title here should be "Avoid calling animation timer updates while the page is loading"

> Source/WebCore/ChangeLog:8
> +        No new tests. 

... and then here you should describe the change. For example:

There is no need to update the animation timer for a RenderObject that has not yet been inserted into the render tree, or that doesn't have animations. Our tests show that this improves page loading by up to 3x on pages with a large number of transitionable objects.

[Dean Jackson]

Also, no need for "Fix for bug 64851 -" on the first line in the change log.

[zeng huiqing]

(In reply to comment #12)
> Also, no need for "Fix for bug 64851 -" on the first line in the change log.

Dean, many thanks for helping review the patch and the good comments:)
I have submitted a new patch according to your comments:)

[zeng huiqing]

Created attachment 102341 [details]
newPatch

change the style of ChangeLog

[WebKit Review Bot]

Comment on attachment 102341 [details]
newPatch

Clearing flags on attachment: 102341

Committed r91999: <http://trac.webkit.org/changeset/91999>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[zeng huiqing]

Thanks very much for the review:)