Closed Bug 1338382 Opened 5 years ago Closed 5 years ago

stylo: refactor and clean up style computation

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bholley, Unassigned)

References

Details

Attachments

(5 files)

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.
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).
Attachment #8835824 - Flags: review?(emilio+bugs)
This is necessary to start synthesizing the styles in match_element and avoid
round-tripping them through the caller.
Attachment #8835826 - Flags: review?(emilio+bugs)
Attachment #8835824 - Flags: review?(emilio+bugs) → review+
Attachment #8835825 - Flags: review?(emilio+bugs) → review+
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?
Attachment #8835826 - Flags: review?(emilio+bugs) → review+
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?
Attachment #8835827 - Flags: review?(emilio+bugs) → review+
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?
Attachment #8835828 - Flags: review?(emilio+bugs) → review+
(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.
(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.
(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.
Landed in https://github.com/servo/servo/pull/15480
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.