1354426 - stylo: Animations generate a UpdateAnimationsTasks::EffectProperties task each tick

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Brian Birtles (:birtles)]

Attachment: Test case

The attached test case includes a script-generated animation where the context font-size is changed mid-way through the animation. (I've slowed it down significantly to allow time to check the behavior in the debugger before and after the change.)

Normally I would expect EffectCompositor::UpdateEffectProperties to be called roughly twice:

1) When we create the animation (there might be a couple of extra calls here, who knows)
2) When we update the context in the middle of the animation

However, with a fairly recent debug stylo build if I put a breakpoint in EffectCompositor::UpdateEffectProperties I see it being called every tick. I can confirm this by setting the breakpoint condition to only fire on multiples of 50 calls and then it gets hit every 1s or so.

The stack I see in Visual Studio is:

> >	xul.dll!mozilla::EffectCompositor::UpdateEffectProperties<mozilla::ServoComputedValuesWithParent const & __ptr64>(const mozilla::ServoComputedValuesWithParent & aStyleType, mozilla::dom::Element * aElement, mozilla::CSSPseudoElementType aPseudoType) Line 370	C++
>  	xul.dll!Gecko_UpdateAnimations(const mozilla::dom::Element * aElement, nsIAtom * aPseudoTagOrNull, const ServoComputedValues * aComputedValues, const ServoComputedValues * aParentComputedValues, mozilla::UpdateAnimationsTasks aTaskBits) Line 481	C++
>  	xul.dll!style::gecko::wrapper::{{impl}}::update_animations(style::gecko::wrapper::GeckoElement * self, core::option::Option<&style::gecko::selector_parser::PseudoElement> pseudo, style::context::UpdateAnimationsTasks tasks) Line 599	Unknown
>  	xul.dll!style::context::SequentialTask<style::gecko::wrapper::GeckoElement>::execute<style::gecko::wrapper::GeckoElement>() Line 233	Unknown
>  	xul.dll!style::context::{{impl}}::drop<style::gecko::wrapper::GeckoElement>(style::context::ThreadLocalStyleContext<style::gecko::wrapper::GeckoElement> * self) Line 329	Unknown
>  	[External Code]	
>  	xul.dll!style::parallel::traverse_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly>(style::gecko::traversal::RecalcStyleOnly * traversal, style::gecko::wrapper::GeckoElement root, core::option::Option<usize> known_root_dom_depth, style::traversal::PreTraverseToken token, rayon::api::ThreadPool * queue) Line 92	Unknown
>  	xul.dll!geckoservo::glue::traverse_subtree(style::gecko::wrapper::GeckoElement element, style::gecko_bindings::bindings::RawServoStyleSet * raw_data, style::traversal::TraversalFlags traversal_flags) Line 196	Unknown
>  	xul.dll!geckoservo::glue::Servo_TraverseSubtree(style::gecko_bindings::structs::root::mozilla::dom::Element * root, style::gecko_bindings::bindings::RawServoStyleSet * raw_data, style::gecko_bindings::structs::root::mozilla::TraversalRootBehavior behavior) Line 219	Unknown
>  	[External Code]	
>  	xul.dll!mozilla::detail::AtomicBaseIncDec<unsigned int,2>::operator--(int __formal) Line 608	C++
>  	xul.dll!Checker::EndReadOp() Line 125	C++


Stepping through I see we actually do mark the cascade as needing an update and do the work in KeyframeEffectReadOnly::BuildProperties so this seems like a fair bit of wasted work.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Don't call process_animation during animation-only restyles

UpdateEffectProperties() in called during animation-only restyles.
This patch fixes it.

Comment 2

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Created attachment 8855642 [details] [diff] [review]
> Don't call process_animation during animation-only restyles
> 
> UpdateEffectProperties() in called during animation-only restyles.
> This patch fixes it.

CSS Transition is also dependent on this patch. (I use another way to fix this problem, and I can rebase my patch after this merged.)

Comment 3

[Hiroyuki Ikezoe (:hiro)]

A big mistake what I made is that I thought cascade_with_replacements() does equivalent stuff what cascade_element() does because the name implies doing it. But actually it doesn't. I guess we should rename cascade_with_replacements() first.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aec74fa484c6013bf68f81f73eead731b3ffb8b

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1354426 - Rename cascade_with_replacements to replace_rules.

That's because the function does not invoke cascade() at all.

Review commit: https://reviewboard.mozilla.org/r/128302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128302/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1354426 - Don't call process_animations during animation-only restyles.

Review commit: https://reviewboard.mozilla.org/r/128304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128304/

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I did confirm that with these patches we don't call UpdateEffectProperties(). But if we move mouse cursor on content window, we call UpdateEffectProperties() a bunch of times because of bug 1353212.  We should fix that bug at some point.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8856381 [details]
Bug 1354426 - Rename cascade_with_replacements to replace_rules.

https://reviewboard.mozilla.org/r/128302/#review131228

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8856382 [details]
Bug 1354426 - Don't call process_animations during animation-only restyles.

https://reviewboard.mozilla.org/r/128304/#review131242

::: servo/components/style/matching.rs:1237
(Diff revision 1)
> -        self.cascade_primary_or_pseudo(context, &mut data, None, /* animate = */ true);
> +        let animate = !context.shared.animation_only_restyle;
> +        self.cascade_primary_or_pseudo(context, &mut data, None, animate);

Maybe it makes more sense to check context.shared.animation_only_restyle inside cascade_primary_or_pseudo, instead of passing in this boolean argument?  Up to you.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Thank you! I decided to check it in cascade_primary_or_pseudo().

https://github.com/servo/servo/pull/16344

Comment 11

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/autoland/rev/425b938551df0b3e6a8e7446276af1428758e01d