1356141 - stylo: Don't run normal traversal if only animation styles have changed

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Currently if only animation styles have changed we still set the regular dirty bit and run the regular traversal in additional to the animation restyle traversal. This is so that we end up generating a style context in the post traversal. We should find a way to avoid doing the regular (non-animation) restyle if we have only animation restyles.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

If we are OK with checking animation_only_dirty_descendants bit to process post traversal as well, setting the animation only bit during animation-only restyle is easiest way to solve this.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d61a1a2a863104e1516400e84d29f46c60992dc

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: a crash test case

An interim report:
I have patches to fix this bug and a try with the patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b69427df6fe4c51d0b3a63e01a7f4a6916674589

The try looks good but there is still another issue I have to solve apart from the patches.
Attaching file causes an assertion at the end of Servo_TraverseSubtree() on pure mozilla-central.
<div id=anonymous-div> seems to not have style data at the end of the function.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: more simplified test case

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I am going to handle the crash test case in another bug.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f52271ecabf7e6cf5e2f684144fea3b4fe818b8

In the try run, print-no-animations.html failed intermittently, this failure will be handled in bug 1374154.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1356141 - Use mutate_data() instead of get_data().map() with borrow_mut().

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1356141 - Don't traverse elements that have no style data in aniamtion-only restyle.

Animation-only restyle only works with elements that have already styled.

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1356141 - Check the child is unstyled without creating element data in preprocess_children.

If we check it and skip the child after ensure_element_data() call,
the child will have an empty element data, so we will succeed
element_data.is_some() check unexpectedly.

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1356141 - Don't traverse any elements that needed only for animation-only restyles in normal traversal.

Before this patch, we were setting the dirty descendants bit in animation-only
restyles and it triggered unnecessary traversal for elements that does not need
the traversal (i.e no need selector matching).

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1356141 - Skip print-no-animations.html.

reftest-print mode for animations does not work well yet.
We will handle it in bug 1374154.

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

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8879002 [details]
Bug 1356141 - Use mutate_data() instead of get_data().map() with borrow_mut().

https://reviewboard.mozilla.org/r/150272/#review154902

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8879003 [details]
Bug 1356141 - Don't traverse elements that have no style data in aniamtion-only restyle.

https://reviewboard.mozilla.org/r/150274/#review154904

::: commit-message-04ff8:1
(Diff revision 1)
> +Bug 1356141 - Don't traverse elements that have no style data in aniamtion-only restyle. r?heycam

animation

::: commit-message-04ff8:3
(Diff revision 1)
> +Bug 1356141 - Don't traverse elements that have no style data in aniamtion-only restyle. r?heycam
> +
> +Animation-only restyle only works with elements that have already styled.

"already been styled"

::: servo/components/style/traversal.rs:335
(Diff revision 1)
> +            // restyle is not necessary for the elements.
> +            let data = match el.borrow_data() {
> +                Some(d) => d,
> +                None => {
> +                    return false;
> +                },

Nit: I think the prevailing style is not to use a trailing comma after match statement arms that are a block.

(Although I do kind of like what it looked like before

  Node => return false,

as it's nice and compact.)

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8879004 [details]
Bug 1356141 - Check the child is unstyled without creating element data in preprocess_children.

https://reviewboard.mozilla.org/r/150276/#review154914

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8879006 [details]
Bug 1356141 - Skip print-no-animations.html.

https://reviewboard.mozilla.org/r/150280/#review154916

::: commit-message-53c08:3
(Diff revision 1)
> +reftest-print mode for animations does not work well yet.
> +We will handle it in bug 1374154.

Nit: Should this say "does not work well yet" or simply "does not work"? Does it sometimes/sort-of work for animations?

::: layout/reftests/css-animations/reftest.list:3
(Diff revision 1)
> -fails-if(!styloVsGecko) == print-no-animations.html print-no-animations-ref.html # reftest harness doesn't actually make pres context non-dynamic for reftest-print tests
> -fails-if(!styloVsGecko) != print-no-animations.html print-no-animations-notref.html # reftest harness doesn't actually make pres context non-dynamic for reftest-print tests
> +skip-if(styloVsGecko) == print-no-animations.html print-no-animations-ref.html # reftest harness doesn't actually make pres context non-dynamic for reftest-print tests. Bug 1356141 for stylo
> +skip-if(styloVsGecko) != print-no-animations.html print-no-animations-notref.html # reftest harness doesn't actually make pres context non-dynamic for reftest-print tests. Bug 1356141 for stylo

Wrong bug number here. Should be bug 1374154 I think.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8879005 [details]
Bug 1356141 - Don't traverse any elements that needed only for animation-only restyles in normal traversal.

https://reviewboard.mozilla.org/r/150278/#review154920

::: dom/base/Element.h:482
(Diff revision 1)
>      UnsetFlags(ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO);
>    }
>  
>    inline void NoteDirtyDescendantsForServo();
>  
> +  bool HasDirtyDescendantsForServoPostTraversal() const

Nit: it feels a bit strange to use the name "ForPostTraversal" here, since the bits can be used for both controlling the normal/animation traversal and for the post-traversal pass.

What do you think about having separate {Has,Unset}AnimationOnlyDirtyDescendantsForServo functions?  That would parallel the name used in the Rust code.

Up to you.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Cameron McCormack (:heycam) from comment #15)
> Comment on attachment 8879005 [details]
> Bug 1356141 - Don't traverse any elements that needed only for
> animation-only restyles in normal traversal.
> 
> https://reviewboard.mozilla.org/r/150278/#review154920
> 
> ::: dom/base/Element.h:482
> (Diff revision 1)
> >      UnsetFlags(ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO);
> >    }
> >  
> >    inline void NoteDirtyDescendantsForServo();
> >  
> > +  bool HasDirtyDescendantsForServoPostTraversal() const
> 
> Nit: it feels a bit strange to use the name "ForPostTraversal" here, since
> the bits can be used for both controlling the normal/animation traversal and
> for the post-traversal pass.

Ah, indeed.

> What do you think about having separate
> {Has,Unset}AnimationOnlyDirtyDescendantsForServo functions?  That would
> parallel the name used in the Rust code.
> 
> Up to you.

I will add those two functions.  Thank you!

Comment 17

[Hiroyuki Ikezoe (:hiro)]

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

A final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3176e71e5c976d9af1880d51596ab2afa289b362

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8879005 [details]
Bug 1356141 - Don't traverse any elements that needed only for animation-only restyles in normal traversal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150278/diff/1-2/

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8879006 [details]
Bug 1356141 - Skip print-no-animations.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150280/diff/1-2/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8cd3f64b530
Don't traverse any elements that needed only for animation-only restyles in normal traversal. r=heycam
https://hg.mozilla.org/integration/autoland/rev/228504fdc6e9
Skip print-no-animations.html. r=birtles

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8cd3f64b530
https://hg.mozilla.org/mozilla-central/rev/228504fdc6e9