1388623 - stylo: Continue cleaning up the traversal entry point machinery

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

Getting back to this. I've got a few patches in the queue that I want to get into the tree. More to come.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=451f84d900f648804b3df4f8fd1425d7aff4bf4f

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Switch to PreTraverseSync for new-element styling. v1

the PreTraverse stuff is all about ticking animations, which isn't something we
want to do when we're trying to get styles synchronously in the frame
constructor.

MozReview-Commit-ID: L6lw4ef4Jdk

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Move the document-root-cache-priming into PreTraverseSync. v1

I added this before PreTraverseSync existed, but that's really where it belongs.

MozReview-Commit-ID: DZlcH70QbEt

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add some logging to the traversal. v1

MozReview-Commit-ID: 3PzkUtEsa6p

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Be more careful about the flags we clear for forgetful traversals. v1

MozReview-Commit-ID: ETDL8KsInAn

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Use our new traversal flags to avoid doing post-traversal clearing of restyle state. v1

MozReview-Commit-ID: E4fqtsQtO9E

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8895220 [details] [diff] [review]
Part 1 - Switch to PreTraverseSync for new-element styling. v1

Review of attachment 8895220 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for cleaning up!

::: layout/style/ServoStyleSet.cpp
@@ +901,1 @@
>    PreTraverse();

If I understand correctly about StyleNewlyBoundElement(), we can also skip the first animation only restyle here, that means we can drop this PreTraverse() call. No?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8895223 [details] [diff] [review]
Part 4 - Be more careful about the flags we clear for forgetful traversals. v1

Review of attachment 8895223 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but I'd really really prefer the functions to be renamed. Please do if it's not too much hassle.

::: servo/components/style/data.rs
@@ +70,5 @@
>  
>      /// Clear all the restyle state associated with this element.
> +    ///
> +    /// FIXME(bholley): The only caller of this should probably just assert that
> +    /// the hint is empty and call clear_flags_and_damage().

How?

We use this to clear the data on the post-traversal, how should that assert that the hint is empty if you're clearing an explicit hint on a display: none subtree where we haven't processed it?

@@ +86,1 @@
>      fn clear_flags_and_damage(&mut self) {

Maybe rename the functions to |clear_restyle_state| and |clear_restyle_flags_and_damage|? Otherwise it's confusing.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8895224 [details] [diff] [review]
Part 5 - Use our new traversal flags to avoid doing post-traversal clearing of restyle state. v1

Review of attachment 8895224 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.cpp
@@ +383,1 @@
>      if (Servo_TraverseSubtree(aRoot, mRawSet.get(), &snapshots, aFlags)) {

I'd just write this as:

postTraversalRequired |= Servo_TraverseSubtree(..);
MOZ_ASSERT_IF(isInitial || forReconstruct, !postTraversalRequired);

Or something like that. But your call.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> If I understand correctly about StyleNewlyBoundElement(), we can also skip
> the first animation only restyle here, that means we can drop this
> PreTraverse() call. No?

Good catch!

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> We use this to clear the data on the post-traversal, how should that assert
> that the hint is empty if you're clearing an explicit hint on a display:
> none subtree where we haven't processed it?

Hm... I guess we shouldn't store the hint in the first place here. Did you just add the FIXME because you didn't try it? Or did you try it and it failed the assert?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> 
> @@ +86,1 @@
> >      fn clear_flags_and_damage(&mut self) {
> 
> Maybe rename the functions to |clear_restyle_state| and
> |clear_restyle_flags_and_damage|? Otherwise it's confusing.

Yeah good call. I'll do that.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> > We use this to clear the data on the post-traversal, how should that assert
> > that the hint is empty if you're clearing an explicit hint on a display:
> > none subtree where we haven't processed it?
> 
> Hm... I guess we shouldn't store the hint in the first place here. Did you
> just add the FIXME because you didn't try it? Or did you try it and it
> failed the assert?

Yeah I didn't have time to try it. Given that I need to do another try push anyway, I'll try it now.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> I'd just write this as:
> 
> postTraversalRequired |= Servo_TraverseSubtree(..);
> MOZ_ASSERT_IF(isInitial || forReconstruct, !postTraversalRequired);

Yeah that's nicer, I'll do that.

Try push without the change discussed in comment 11:

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

I'll try the change in comment 11 next.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove clear_restyle_state and assert against restyle hints left in the tree. v1

MozReview-Commit-ID: blYJzSxaRG

Comment 14

[Bobby Holley (:bholley)]

Try push with part 6: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c61b5a134c88bfb7ca11e59de1756a1f1e5b9b9

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8895441 [details] [diff] [review]
Part 6 - Remove clear_restyle_state and assert against restyle hints left in the tree. v1

Hm, this is orange. I may poke at it, but doesn't need to block this bug.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #15)
> Comment on attachment 8895441 [details] [diff] [review]
> Part 6 - Remove clear_restyle_state and assert against restyle hints left in
> the tree. v1
> 
> Hm, this is orange. I may poke at it, but doesn't need to block this bug.

Oh, sure, we don't consume non-animation hints during animation traversals, and vice-versa.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83319f6a69b2
Switch to PreTraverseSync for new-element styling. r=hiro
https://hg.mozilla.org/integration/autoland/rev/2cad3133b6a6
Move the document-root-cache-priming into PreTraverseSync. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b452402234b6
Use our new traversal flags to avoid doing post-traversal clearing of restyle state. r=emilio

Comment 18

[Bobby Holley (:bholley)]

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/83319f6a69b2
https://hg.mozilla.org/mozilla-central/rev/2cad3133b6a6
https://hg.mozilla.org/mozilla-central/rev/b452402234b6