1380877 - stylo: Trim some fat from the traversal

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (On Leave Through ~December)]

I have some patches to reduce overhead in the traversal. A major piece of to avoid enumerating the children twice (once during preprocessing, and once from the traversal driver).

Comment 1

[Bobby Holley (On Leave Through ~December)]

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

Comment 2

[Bobby Holley (On Leave Through ~December)]

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

Comment 3

[Bobby Holley (On Leave Through ~December)]

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

Comment 4

[Bobby Holley (On Leave Through ~December)]

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

Comment 5

[Bobby Holley (On Leave Through ~December)]

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

Finally green. This took way longer than I thought to get right.

Comment 6

[Bobby Holley (On Leave Through ~December)]

Attachment: Part 1 - Use Drains in parallel.rs. v1

Contrary to the comment around its declaration, we do move the SmallVec in
the replace call. This fixes that.

MozReview-Commit-ID: Bc1qFPpHQ8t

Comment 7

[Bobby Holley (On Leave Through ~December)]

Attachment: Part 2 - Move clearing of dirty descendants bit closer to the last place it's needed. v1

In the next patch, we'll move logic to identify the children for traversal into
preprocess_children (which will be renamed), and the set_dirty_descendants logic
will move along with it. So left as-is, the code here will clobber the flags.

MozReview-Commit-ID: 7ZskKWD4QC3

Comment 8

[Bobby Holley (On Leave Through ~December)]

Attachment: Part 3 - Pass a callback to recalc_style_at to avoid traversing children twice. v1

MozReview-Commit-ID: DIHXaVNzbFM

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886738 [details] [diff] [review]
Part 1 - Use Drains in parallel.rs. v1

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

I guess, though the code was nicer before...

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886740 [details] [diff] [review]
Part 3 - Pass a callback to recalc_style_at to avoid traversing children twice. v1

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

::: servo/components/style/gecko/traversal.rs
@@ +38,3 @@
>      {
>          if node.is_element() {
>              let el = node.as_element().unwrap();

While you're here, can you change this to:

if let Some(el) = node.as_element() {

instead?

::: servo/components/style/traversal.rs
@@ +117,5 @@
>  
>  /// A DOM Traversal trait, that is used to generically implement styling for
>  /// Gecko and Servo.
>  pub trait DomTraversal<E: TElement> : Sync {
>      /// Process `node` on the way down, before its children have been processed.

Please document the new function.

@@ +257,5 @@
>          debug_assert!(node.is_text_node());
>          false
>      }
>  
> +    /// Returns true if traversal is needed for the given element and subtree.

Document that the only place parent_data can be None is if there's no parent?

@@ +617,5 @@
> +    // * This is a reconstruct traversal.
> +    // * This is a servo non-incremental traversal.
> +    //
> +    // Additionally, there are a few scenarios where we avoid traversing the
> +    // subtree even if the element styles are out of date. These cases are

I don't think "the element styles are out of date" is a great description... Maybe just "the children styles"?

@@ +778,5 @@
>          important_rules_changed
>      )
>  }
>  
> +fn note_children<E, D, F>(

I find "note" a very generic name, and it's doing very concrete stuff... Don't have an immediate better suggestion, though I think the previous name is clearer than this (but still not great!)

Comment 11

[Bobby Holley (On Leave Through ~December)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Comment on attachment 8886738 [details] [diff] [review]
> Part 1 - Use Drains in parallel.rs. v1
> 
> Review of attachment 8886738 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess, though the code was nicer before...

Yeah, fair. I realize now that I could have just kept the old code and emptied the array after borrowing it (rather than doing the replace). But I'd probably still want a try push, and otherwise this is land-ready, so I'm just going to land it.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> I find "note" a very generic name, and it's doing very concrete stuff...
> Don't have an immediate better suggestion, though I think the previous name
> is clearer than this (but still not great!)

Well, pre-process isn't really accurate here, because it both preprocesses and passes them to the caller. We can rename it if we think of a better name.

Thanks for the reviews!

Comment 12

[Bobby Holley (On Leave Through ~December)]

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

Comment 13

[Bobby Holley (On Leave Through ~December)]

Attachment: Part 1 - Avoid memmoving the large smallvec in Parallel.rs.

I had to rebase anyway, so I made this more minimal patch for part 1. I'm
going to assume emilio prefers this. :-)

MozReview-Commit-ID: 7nzjMwOmszZ