Bug 119479 – Inserting a JS generated keyframe animation shouldn't trigger a whole document style recalc

• Component: CSS
• Status: RESOLVED FIXED

[Ralph T]

Created attachment 208099 [details]
Example of adding an animation via JS which triggers a whole document style recalc

Inserting a keyframe animation created in JS causes a full style recalc which can be very expensive on large documents. WebKit doesn't start an animation if it was referenced by a node prior to inserting it, so hopefully it's possible to add a custom keyframe animation for only the time it takes to parse it.

The attached example causes a longer style recalc (on L26) when the keyframe rule is added.

[Ralph T]

Created attachment 213191 [details]
Better/slower test using CSSStyleSheet.insertRule.

This test inserts lots of elements with a style that's expensive to calculate (as seen in the timeline view), then when you click the red box it inserts a @-webkit-keyframes rule into an existing stylesheet and sets the box's webkitAnimationName to use the new rule. Currently inserting the new rule causes an invalidation of the style of every node.

[Ralph T]

Created attachment 213194 [details]
Patch

[Ralph T]

Comment on attachment 213194 [details]
Patch

Clear review flag.

[Andreas Kling]

Comment on attachment 213194 [details]
Patch

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

> Source/WebCore/css/CSSStyleSheet.cpp:317
> +        if (owner)
> +            owner->ensureStyleResolver().addKeyframeStyle(static_cast<StyleRuleKeyframes*>(rule.get()));

If there is no StyleResolver here, I don't think we need to create one.
Use Document::styleResolverIfExists() instead, the keyframes rule will get picked up during the inevitable StyleResolver construction.

[Antti Koivisto]

Comment on attachment 213194 [details]
Patch

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

> Source/WebCore/css/CSSStyleSheet.cpp:318
> +    if (rule->type() == CSSRule::WEBKIT_KEYFRAMES_RULE) {
> +        Document* owner = ownerDocument();
> +        if (owner)
> +            owner->ensureStyleResolver().addKeyframeStyle(static_cast<StyleRuleKeyframes*>(rule.get()));
> +    }

I think this can go to CSSStyleSheet::didMutateRules.

[Antti Koivisto]

The idea is right assuming there is no way for keyframe insertion to trigger animation or generally affect anything that would require style recalc.

[Ralph T]

The CSS3 Animations spec says about the "animation-name" property:

"If the name does not match any keyframe at-rule, there are no properties to be animated and the animation will not execute."

So if you have an element that already matches the animation name you're inserting then nothing will happen.

[Build Bot]

Comment on attachment 213194 [details]
Patch

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

[Ralph T]

Created attachment 213201 [details]
Patch

[Ralph T]

The latest patch uses styleResolverIfExists() and moves the insertion into CSSStyleSheet::didMutateRules. Next I will see what we have so far as tests for dynamic animation insertion and locally run LayoutTests on a debug build (of EFL) in case the change causes new asserts.

[Darin Adler]

Comment on attachment 213201 [details]
Patch

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

This seems like a great optimization. I think there is a slightly tighter way to code it.

> Source/WebCore/css/CSSStyleSheet.cpp:175
> +        ASSERT(insertedRule->type() == StyleRuleBase::Keyframes);
> +        StyleResolver* resolver = 0;
> +        if (owner && (resolver = owner->styleResolverIfExists()))
> +            resolver->addKeyframeStyle(static_cast<StyleRuleKeyframes*>(insertedRule));

It’s not helpful to initialize resolver to 0 since that value is never used. And also, this coding style is a bit tortured. It doesn’t really make sense to null-check owner here since we already checked that earlier. I think it should end up like this:

    if (StyleResolver* resolver = owner->styleResolverIfExists())
        return resolver->addKeyframeStyle(insertedRule);

If we did need a null check for owner, I would have suggested an early return.

> Source/WebCore/css/CSSStyleSheet.cpp:427
> +    , m_insertedRule(0)

Please use nullptr instead of 0 in new code.

> Source/WebCore/css/CSSStyleSheet.h:88
> +    enum RuleMutationType { OtherMutation, RuleInsertion, KeyframesRuleInsertion };

I don’t think a specific type of rule should be treated as a separate mutation type, especially if we have to pass the actual rule in, I suggest just using RuleInsertion combined with an inserted keyframes rule. Or eliminate the concept of RuleMutationType entirely (see below).

> Source/WebCore/css/CSSStyleSheet.h:94
> +        RuleMutationScope(CSSStyleSheet*, RuleMutationType = OtherMutation, StyleRuleBase* insertedRule = 0);

Please use nullptr instead of 0 in new code.

I suggest we use the type StyleRuleKeyframes* here instead of StyleRuleBase*. If we decide to use this in a more general way later, we could change that, but no reason to be needlessly general.

Another way to go would be to get rid of RuleMutationType entirely, and only pass in the inserted rule. Then a rule of nullptr would mean “other mutation”. Either way would be OK with me.

[Antti Koivisto]

Comment on attachment 213201 [details]
Patch

Yeah, looks good except for the now-unnecessary KeyframesRuleInsertion value. We may still want to keep the RuleMutationType enum for clarity.

[Ralph T]

Created attachment 213285 [details]
Patch

[Ralph T]

(In reply to comment #11)

Thanks for the review, Darin!

> > Source/WebCore/css/CSSStyleSheet.cpp:175
> > +        ASSERT(insertedRule->type() == StyleRuleBase::Keyframes);
> > +        StyleResolver* resolver = 0;
> > +        if (owner && (resolver = owner->styleResolverIfExists()))
> > +            resolver->addKeyframeStyle(static_cast<StyleRuleKeyframes*>(insertedRule));
> 
> It’s not helpful to initialize resolver to 0 since that value is never used. And also, this coding style is a bit tortured. It doesn’t really make sense to null-check owner here since we already checked that earlier. I think it should end up like this:
> 
>     if (StyleResolver* resolver = owner->styleResolverIfExists())
>         return resolver->addKeyframeStyle(insertedRule);
> 
> If we did need a null check for owner, I would have suggested an early return.

I reworked this and folded into the existing RuleInsertion branch since they're now share the same conditions. It's a lot better now :).

> 
> > Source/WebCore/css/CSSStyleSheet.cpp:427
> > +    , m_insertedRule(0)
> 
> Please use nullptr instead of 0 in new code.

Done.

> > Source/WebCore/css/CSSStyleSheet.h:94
> > +        RuleMutationScope(CSSStyleSheet*, RuleMutationType = OtherMutation, StyleRuleBase* insertedRule = 0);
> 
> Please use nullptr instead of 0 in new code.
> 

Done.

> I suggest we use the type StyleRuleKeyframes* here instead of StyleRuleBase*. If we decide to use this in a more general way later, we could change that, but no reason to be needlessly general.

I did this, and changed the name to m_insertedKeyframesRule.

> Another way to go would be to get rid of RuleMutationType entirely, and only pass in the inserted rule. Then a rule of nullptr would mean “other mutation”. Either way would be OK with me.

I kept the enum because maybe there will be other optimized cases in the future (like deleting keyframes?).

I still plan on making a new test for dynamically creating animations (I haven't done this before; it's going to look a bit like animations/change-keyframes.html).

[Antti Koivisto]

Comment on attachment 213285 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:9
> +        https://bugs.webkit.org/show_bug.cgi?id=119479
> +
> +        Change CSSStyleSheet::didMutateRules to not invalidate all node's styles when inserting a
> +        @-webkit-keyframes rule, and to instead insert the rule directly into the StyleResolver.
> +
> +        Reviewed by NOBODY (OOPS!).

Reviewed-by line usually goes after the URL, before the descriptive text.

> Source/WebCore/ChangeLog:11
> +        No new tests (OOPS!).

Some tests for dynamic insertions (and deletions too) would be nice.

[Ralph T]

Created attachment 213316 [details]
Patch

[Ralph T]

Most recent patch includes a test case which adds some keyframes rules and removes  them before and after the animation has started.

Unfortunately I was unable to produce a working build of DumpRenderTree or WKTR, so I haven't been able to generate expected results. Antti, would you be able to do this for me? (I'm not sure of the process here, this is the first time I've added a test). I will keep trying to make a working WKTR in the meantime.

[Ralph T]

Created attachment 213382 [details]
Patch

[Ralph T]

Ok, I figured out my WKTR woes and built expected results for the test. Antti -- is this what you had in mind for tests? If so could you set r+ and cq+ for me (I'm not a committer).

[WebKit Commit Bot]

Comment on attachment 213382 [details]
Patch

Clearing flags on attachment: 213382

Committed r156912: <http://trac.webkit.org/changeset/156912>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

I'm confused about the behavior with this patch. If an element has: -webkit-animation-name: foo and you insert @-webkit-keyframes foo {...}, does the animation start?

[Ralph T]

(In reply to comment #22)
> I'm confused about the behavior with this patch. If an element has: -webkit-animation-name: foo and you insert @-webkit-keyframes foo {...}, does the animation start?

It does not, and it didn't before this patch either. I think the relevant part of the CSS 3 Animations Spec is "If the name does not match any keyframe at-rule, there are no properties to be animated and the animation will not execute."

I read that to mean that at the time the webkitAnimationName property is set if there's no keyframes at-rule with that name then it's ignored, but it's not explicitly spelled out. I don't believe the patch changed any WebKit behavior, however.

I can definitely add some more test cases around this, or raise the issue on www-style.

[Simon Fraser (smfr)]

(In reply to comment #23)
> I can definitely add some more test cases around this, or raise the issue on www-style.

Please do raise it on www-style so the behavior is specified.

[Ralph T]

(In reply to comment #24)
> (In reply to comment #23)
> > I can definitely add some more test cases around this, or raise the issue on www-style.
> 
> Please do raise it on www-style so the behavior is specified.

Ok, raised: http://lists.w3.org/Archives/Public/www-style/2013Oct/0248.html

As I mentioned in the email, I also tested in Firefox and that does start the animation when just adding the @keyframes rule (also they're very quick starting the animation, so I'd like to learn what's going on there!).

[Ralph T]

Ok, so I was mistaken and an animation should start in the case Simon described -- and there's even a bug for it: https://bugs.webkit.org/show_bug.cgi?id=27881 .. Oops! I'll follow up on that bug.