Bug 137583 - Introduce an isCSSAnimated flag on RenderElement for performance
Summary: Introduce an isCSSAnimated flag on RenderElement for performance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 137786
  Show dependency treegraph
 
Reported: 2014-10-09 16:44 PDT by Chris Dumez
Modified: 2014-10-16 14:31 PDT (History)
12 users (show)

See Also:


Attachments
Patch (15.71 KB, patch)
2014-10-09 16:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (19.53 KB, patch)
2014-10-11 16:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (615.80 KB, application/zip)
2014-10-11 17:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (628.46 KB, application/zip)
2014-10-11 19:24 PDT, Build Bot
no flags Details
Patch (19.59 KB, patch)
2014-10-11 20:01 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.34 KB, patch)
2014-10-13 22:39 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-10-09 16:44:36 PDT
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.
Comment 1 Chris Dumez 2014-10-09 16:47:13 PDT
<rdar://problem/18602906>
Comment 2 Chris Dumez 2014-10-09 16:53:21 PDT
Created attachment 239581 [details]
Patch
Comment 3 Simon Fraser (smfr) 2014-10-09 17:16:03 PDT
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..
Comment 4 Chris Dumez 2014-10-09 17:41:29 PDT
(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.
Comment 5 Chris Dumez 2014-10-11 15:29:49 PDT
Will submit another proposal.
Comment 6 Chris Dumez 2014-10-11 16:26:14 PDT
Created attachment 239690 [details]
Patch
Comment 7 Build Bot 2014-10-11 17:45:48 PDT
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
Comment 8 Build Bot 2014-10-11 17:45:53 PDT
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
Comment 9 Build Bot 2014-10-11 19:24:15 PDT
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
Comment 10 Build Bot 2014-10-11 19:24:22 PDT
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
Comment 11 Chris Dumez 2014-10-11 20:01:09 PDT
Created attachment 239695 [details]
Patch
Comment 12 Chris Dumez 2014-10-13 15:51:05 PDT
Simon, do you have any thought on my latest iteration?
Comment 13 Simon Fraser (smfr) 2014-10-13 15:57:32 PDT
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.
Comment 14 Chris Dumez 2014-10-13 22:39:23 PDT
Created attachment 239778 [details]
Patch
Comment 15 Chris Dumez 2014-10-13 22:41:29 PDT
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.
Comment 16 Chris Dumez 2014-10-14 13:28:02 PDT
Simon, is it any better now? :)
Comment 17 Simon Fraser (smfr) 2014-10-14 14:09:52 PDT
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.
Comment 18 WebKit Commit Bot 2014-10-14 14:50:34 PDT
Comment on attachment 239778 [details]
Patch

Clearing flags on attachment: 239778

Committed r174703: <http://trac.webkit.org/changeset/174703>
Comment 19 WebKit Commit Bot 2014-10-14 14:50:41 PDT
All reviewed patches have been landed.  Closing bug.