Bug 137583 – Introduce an isCSSAnimated flag on RenderElement for performance

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Chris Dumez]

Introduce a lower-level AnimationController::animationForRenderer() API for performance and drop higher level isRunningAnimationOnRenderer() / isRunningAcceleratedAnimationOnRenderer() API.

It is frequent for callers to call isRunningAnimationOnRenderer() or isRunningAcceleratedAnimationOnRenderer() repeatedly on the same renderer to check for different properties. As a result, we end up making a lot of unnecessary m_compositeAnimations HashMap lookups to retrieve the CompositeAnimation for a given renderer. As an example, we do ~4.7 millions HashMap lookups when loading bay.com (spending ~300ms doing that). Providing a lower-level API allows the callers to write more efficient code, reducing the number of lookups to 2.1 millions (2.2x less). As a result, we now spend ~137ms doing lookups when loading ebay.com.

2.1 millions is still too much and this will need further investigation / improvements. However, I believe this API change will remain useful as we really want to avoid repetitive lookups for the same renderer, even if we end up doing those checks less often.

[Chris Dumez]

<rdar://problem/18602906>

[Chris Dumez]

Created attachment 239581 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 239581 [details]
Patch

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

> Source/WebCore/page/animation/AnimationControllerPrivate.h:89
> +    const CompositeAnimation* animationForRenderer(const RenderElement&) const;

I don't think CompositeAnimation should leak out of AnimationController.

> Source/WebCore/rendering/RenderLayerBacking.cpp:35
> +#include "CompositeAnimation.h"

I don't like this. CompositeAnimation is internal to AnimationController..

[Chris Dumez]

(In reply to comment #3)
> (From update of attachment 239581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239581&action=review
> 
> > Source/WebCore/page/animation/AnimationControllerPrivate.h:89
> > +    const CompositeAnimation* animationForRenderer(const RenderElement&) const;
> 
> I don't think CompositeAnimation should leak out of AnimationController.
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:35
> > +#include "CompositeAnimation.h"
> 
> I don't like this. CompositeAnimation is internal to AnimationController..

I see, I did not know that, thanks. I will try to deal with this some other way then.

[Chris Dumez]

Will submit another proposal.

[Chris Dumez]

Created attachment 239690 [details]
Patch

[Build Bot]

Comment on attachment 239690 [details]
Patch

Attachment 239690 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6128418127085568

New failing tests:
compositing/layer-creation/animation-overlap-with-children.html
compositing/layer-creation/overlap-animation-container.html
compositing/layer-creation/overlap-animation-clipping.html
compositing/layer-creation/overlap-animation.html

[Build Bot]

Created attachment 239691 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 239690 [details]
Patch

Attachment 239690 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6206560762068992

New failing tests:
compositing/layer-creation/animation-overlap-with-children.html
compositing/layer-creation/overlap-animation-container.html
compositing/layer-creation/overlap-animation-clipping.html
compositing/layer-creation/overlap-animation.html

[Build Bot]

Created attachment 239694 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 239695 [details]
Patch

[Chris Dumez]

Simon, do you have any thought on my latest iteration?

[Simon Fraser (smfr)]

Comment on attachment 239695 [details]
Patch

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

> Source/WebCore/page/animation/AnimationBase.h:42
> +enum class AnimationQueryFilter { Any, AcceleratedOnly };

We often use the term "FooOrNot" for thee kinds of enums. I find that a bit more readable than "queryfilter".

> Source/WebCore/page/animation/AnimationBase.h:44
> +enum AnimatedCSSProperties {

These are really properties that can have accelerated animations. So I'd call them AcceleratedAnimationPropertyFlags or something.

> Source/WebCore/page/animation/AnimationController.cpp:254
> +    AnimatedCSSPropertyMask animatedProperties = 0;
> +    if (animation.isAnimatingProperty(CSSPropertyOpacity, filter, runningState))
> +        animatedProperties |= AnimatingOpacityProperty;
> +    if (animation.isAnimatingProperty(CSSPropertyWebkitFilter, filter, runningState))
> +        animatedProperties |= AnimatingWebKitFilterProperty;
> +    if (animation.isAnimatingProperty(CSSPropertyWebkitTransform, filter, runningState))
> +        animatedProperties |= AnimatingWebKitTransformProperty;
> +    return animatedProperties;

This is a bit deceptive. animatedProperties makes it sound like a set of all properties that are animating, but it's really animating properties that happen to be acceleratable.

> Source/WebCore/rendering/RenderElement.h:164
> +    bool hasCompositeAnimation() const { return m_hasCompositeAnimation; }
> +    void setHasCompositeAnimation(bool b) { m_hasCompositeAnimation = b; }

I think this should be "isCSSAnimating" or something. The fact that it's a composite animation (crappy name) is just an imp. detail of AnimationController.

[Chris Dumez]

Created attachment 239778 [details]
Patch

[Chris Dumez]

This latest patch includes only the flag on RenderElement. Getting the AnimationController API right is difficult, especially while maintaining the performance. Introducing the flag on RenderElement is sufficient to greatly improve the performance when loading ebay.com.

[Chris Dumez]

Simon, is it any better now? :)

[Simon Fraser (smfr)]

Comment on attachment 239778 [details]
Patch

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

> Source/WebCore/page/animation/AnimationController.cpp:246
> +    const CompositeAnimation& animation = *m_compositeAnimations.get(renderer);

I sure hope we always get non-null here.

[WebKit Commit Bot]

Comment on attachment 239778 [details]
Patch

Clearing flags on attachment: 239778

Committed r174703: <http://trac.webkit.org/changeset/174703>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.