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

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: birtles, Assigned: hiro)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

6 months ago
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.
(Assignee)

Comment 1

6 months ago
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.
(Assignee)

Updated

4 months ago
Blocks: 1372642
(Assignee)

Comment 2

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d61a1a2a863104e1516400e84d29f46c60992dc
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 months ago
Created attachment 8878277 [details]
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.
(Assignee)

Comment 4

4 months ago
Created attachment 8878311 [details]
more simplified test case
Attachment #8878277 - Attachment is obsolete: true
(Assignee)

Comment 5

4 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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
Attachment #8879002 - Flags: review?(cam) → review+
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.)
Attachment #8879003 - Flags: review?(cam) → review+
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
Attachment #8879004 - Flags: review?(cam) → review+
(Reporter)

Comment 14

4 months ago
mozreview-review
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.
Attachment #8879006 - Flags: review?(bbirtles) → review+
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.
Attachment #8879005 - Flags: review?(cam) → review+
(Assignee)

Comment 16

4 months ago
(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!
(Assignee)

Comment 17

4 months ago
https://github.com/servo/servo/pull/17400

A final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3176e71e5c976d9af1880d51596ab2afa289b362
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8879002 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8879003 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8879004 - Attachment is obsolete: true

Comment 20

4 months ago
Pushed by hikezoe@mozilla.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8cd3f64b530
https://hg.mozilla.org/mozilla-central/rev/228504fdc6e9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
status-firefox55: affected → ---
Blocks: 1385173
You need to log in before you can comment on or make changes to this bug.