1338382 - stylo: refactor and clean up style computation

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

I finally did some long-overdue refactoring to avoid various bits of unnecessary work in the current design. This makes it simpler to do some other work I have queued up, as well as various optimizations that we've planned but haven't yet implemented (like skipping the cascade if the rule node doesn't change).

This will bitrot quickly so I'm hoping to land it soon.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=343ee2a6a55b8c656bcbc4b141e01803330791f5https://treeherder.mozilla.org/#/jobs?repo=try&revision=343ee2a6a55b8c656bcbc4b141e01803330791f5

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - hoist some work out of the tail of compute_style into the caller. v1

This is really part of the "recalc" rather than the "compute" work. But more to
the point, it lets us early-return on the style-sharing stuff (see the next patch).

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hoist style sharing cache handling to the top of compute_style. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Allow the ComputedValues in ComputedStyle to be null. v1

This is necessary to start synthesizing the styles in match_element and avoid
round-tripping them through the caller.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Stop returning rule nodes from match_element and overhaul style calculation logic. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 5 - Clean up and simplify the accumulation of restyle damage. v1

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8835826 [details] [diff] [review]
Part 3 - Allow the ComputedValues in ComputedStyle to be null. v1

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

These changes standalone don't make much sense, but I guess I'll see it used later right?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8835827 [details] [diff] [review]
Part 4 - Stop returning rule nodes from match_element and overhaul style calculation logic. v1

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

::: servo/components/style/matching.rs
@@ +74,5 @@
>  impl MatchResults {
>      /// Returns true if the primary rule node is shareable with other nodes.
>      pub fn primary_is_shareable(&self) -> bool {
> +        self.primary_relations.as_ref()
> +            .map_or(false, |r| relations_are_shareable(r))

You might be able to eta-reduce this, just sayin' ;)

@@ +463,5 @@
> +        let rule_node = &pseudo_style.as_ref().map_or(primary_style, |p| &*p.1).rules;
> +
> +        // Grab the inherited values.
> +        let parent_el;
> +        let parent_data;

These two are only used inside the if branch right? Let's move them inside?

@@ +516,5 @@
> +                                     possibly_expired_animations: &mut Vec<PropertyAnimation>,
> +                                     booleans: CascadeBooleans) {
> +        // Collect some values.
> +        let shared_context = context.shared;
> +        let animate = booleans.animate;

let's just use if booleans.animate below, maybe making cascade_internal getting booleans by reference?

::: servo/components/style/traversal.rs
@@ +524,5 @@
>          }
>          CascadeWithReplacements(hint) => {
> +            let rule_nodes_changed =
> +                element.cascade_with_replacements(hint, context, &mut data);
> +            MatchResults {

If you're refactoring this I wouldn't call this MatchResults anymore (since there's no matching in two of the three branches), maybe CascadeInput? Or CascadeChange?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8835828 [details] [diff] [review]
Part 5 - Clean up and simplify the accumulation of restyle damage. v1

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

::: servo/components/style/matching.rs
@@ +570,5 @@
>          }
>      }
>  
> +    /// Computes and applies restyle damage unless we've already maxed it out.
> +    fn accumulate_damage(&self, restyle: &mut RestyleData,

nit: restyle in its own line.

@@ +811,5 @@
>              match sharing_result {
>                  Ok(shared_style) => {
>                      // Yay, cache hit. Share the style.
>  
> +                    // Accumulate restyle damage.

we should probably be able to just cache the restyle damage here I guess. Anyway we could do it as a followup.

@@ +812,5 @@
>                  Ok(shared_style) => {
>                      // Yay, cache hit. Share the style.
>  
> +                    // Accumulate restyle damage.
> +                    debug_assert!(data.has_styles() == data.has_restyle());

not that it matters a lot, but probably debug_assert_eq!

@@ +817,5 @@
> +                    let old_values = data.get_styles_mut()
> +                                         .and_then(|s| s.primary.values.take());
> +                    if let Some(old) = old_values {
> +                        self.accumulate_damage(data.restyle_mut(), &old,
> +                                               shared_style.values(), None);

What makes us generate rebuild_and_reflow if old_values is None?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> Comment on attachment 8835826 [details] [diff] [review]
> Part 3 - Allow the ComputedValues in ComputedStyle to be null. v1
> 
> Review of attachment 8835826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These changes standalone don't make much sense, but I guess I'll see it used
> later right?

Yeah. In order for match_element to avoid returning rule nodes to the caller, it needs to be able to set the rule node on the ElementData before cascade_element is run, which means that we need to allow the ComputedValues to be temporarily None.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Comment on attachment 8835827 [details] [diff] [review]
> Part 4 - Stop returning rule nodes from match_element and overhaul style
> calculation logic. v1
> 
> Review of attachment 8835827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/matching.rs
> @@ +74,5 @@
> >  impl MatchResults {
> >      /// Returns true if the primary rule node is shareable with other nodes.
> >      pub fn primary_is_shareable(&self) -> bool {
> > +        self.primary_relations.as_ref()
> > +            .map_or(false, |r| relations_are_shareable(r))
> 
> You might be able to eta-reduce this, just sayin' ;)

;-)

> 
> @@ +463,5 @@
> > +        let rule_node = &pseudo_style.as_ref().map_or(primary_style, |p| &*p.1).rules;
> > +
> > +        // Grab the inherited values.
> > +        let parent_el;
> > +        let parent_data;
> 
> These two are only used inside the if branch right? Let's move them inside?

They need to be outside so that the borrow that gets returned from the following block can live long enough.

> 
> @@ +516,5 @@
> > +                                     possibly_expired_animations: &mut Vec<PropertyAnimation>,
> > +                                     booleans: CascadeBooleans) {
> > +        // Collect some values.
> > +        let shared_context = context.shared;
> > +        let animate = booleans.animate;
> 
> let's just use if booleans.animate below, maybe making cascade_internal
> getting booleans by reference?

ok.

> 
> ::: servo/components/style/traversal.rs
> @@ +524,5 @@
> >          }
> >          CascadeWithReplacements(hint) => {
> > +            let rule_nodes_changed =
> > +                element.cascade_with_replacements(hint, context, &mut data);
> > +            MatchResults {
> 
> If you're refactoring this I wouldn't call this MatchResults anymore (since
> there's no matching in two of the three branches), maybe CascadeInput? Or
> CascadeChange?

Per IRC discussion, we decided to keep this as MatchResults.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Comment on attachment 8835828 [details] [diff] [review]
> Part 5 - Clean up and simplify the accumulation of restyle damage. v1
> 
> Review of attachment 8835828 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/matching.rs
> @@ +570,5 @@
> >          }
> >      }
> >  
> > +    /// Computes and applies restyle damage unless we've already maxed it out.
> > +    fn accumulate_damage(&self, restyle: &mut RestyleData,
> 
> nit: restyle in its own line.

Fixed.

> 
> @@ +811,5 @@
> >              match sharing_result {
> >                  Ok(shared_style) => {
> >                      // Yay, cache hit. Share the style.
> >  
> > +                    // Accumulate restyle damage.
> 
> we should probably be able to just cache the restyle damage here I guess.
> Anyway we could do it as a followup.

I don't think that would be valid. Consider the case where we have 50 sibling divs that all share the same style, except that two of them have "id" attributes and then do not match the cache. Those "id" attributes are then stripped, and they match the cached style in the next round. They can have arbitrary RestyleDamage (that might differ from each other, depending on what the id selectors had previously matched).

> @@ +812,5 @@
> >                  Ok(shared_style) => {
> >                      // Yay, cache hit. Share the style.
> >  
> > +                    // Accumulate restyle damage.
> > +                    debug_assert!(data.has_styles() == data.has_restyle());
> 
> not that it matters a lot, but probably debug_assert_eq!

sure.

> 
> @@ +817,5 @@
> > +                    let old_values = data.get_styles_mut()
> > +                                         .and_then(|s| s.primary.values.take());
> > +                    if let Some(old) = old_values {
> > +                        self.accumulate_damage(data.restyle_mut(), &old,
> > +                                               shared_style.values(), None);
> 
> What makes us generate rebuild_and_reflow if old_values is None?

We don't need to do that anymore. I fixed up Servo not to depend on that, and to track whether layout has visited a node at all.

Comment 14

[Bobby Holley (:bholley)]

Landed in https://github.com/servo/servo/pull/15480