1340022 - stylo: Implement the "change hints handled for descendants" optimization

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

This is that fun one we spent forever figuring out in Taipei. The logic is simple enough, but it's easy to screw up the bits here so I'd like a careful review from both heycam and emilio.

I wasn't planning to do this until later, but bug 1331047 needs the tracking of whether an ancestor has ReconstructFrame.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Cameron McCormack (:heycam)]

I forgot about bug 1301258, but given I wasn't able to convert this into just a simple bitmasking operation, I think it's fine to work on top of the existing NS_HintsNotHandledForDescendantsIn function, and we can update the handling on the Servo side once I fix up bug 1301258.

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

Attachment: Bug 1340022 - Implement "handled for descendants" tracking for RestyleDamage.

Review commit: https://reviewboard.mozilla.org/r/112978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112978/

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8837972 [details]
Bug 1340022 - Implement "handled for descendants" tracking for RestyleDamage.

https://reviewboard.mozilla.org/r/112978/#review114464

The rest looks OK to me.

::: servo/components/style/matching.rs:585
(Diff revision 1)
> +            let new_damage = self.compute_restyle_damage(&old_values, &new_values, pseudo) &
> +                             !restyle.damage_handled;

In Gecko right now, without my bug 1301258 patches, we don't skip accumulating damage unless all of the change hints generated for the current element have been handled by ancestors.  IIRC, I found that masking off just the ones that are handled for descendants was not correct, since not all of the bits are independent.  (OTOH I believe that ORing more bits in is always OK.)  I'll have to page this stuff back in to remind myself of specifics, if you want to see an example.

With the bug 1301258 patches, I added a function which does a correct "subtraction" of the hints handled for descendants by ancestors (NS_RemoveSubsumedHints).  I think we need to do something similar here, and I don't think you can construct it just by using Gecko_HintsHandledForDescendants.

If you want to match what Gecko is doing now, you can do:

  let new_damage = self.compute_restyle_damage(...);
  if !restyle.damage_handled.contains(new_damage) {
      restyle.damage |= new_damage;
  }

but if you want to do the more optimized thing here, I should finish off bug 1301258 to work on.

::: servo/components/style/traversal.rs:563
(Diff revision 1)
>  }
>  
>  fn preprocess_children<E, D>(traversal: &D,
>                               element: E,
>                               mut propagated_hint: StoredRestyleHint,
> +                             damage_handled:  RestyleDamage,

Nit: just one space after ":".

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8837972 [details]
Bug 1340022 - Implement "handled for descendants" tracking for RestyleDamage.

https://reviewboard.mozilla.org/r/112978/#review114476

I don't have a lot more to add than what heycam already did.

However, I think this optimization maps way more cleanly inside the RecreateStyleContexts pass, and it'd be probably easier to understand since we wouldn't have all the restyling logic around.

Also, the FFI call problem goes automatically away if we do this.

What do you think bobby, cam?

::: servo/components/style/servo/restyle_damage.rs:127
(Diff revision 1)
>      }
> +
> +    /// Assuming |self| is applied to an element, returns the set of damage that
> +    /// would be superfluous to apply for descendants.
> +    pub fn handled_for_descendants(self) -> Self {
> +        /// NB: Servo doesn't implement this optimization yet.

Note: no need for this to be a doc comment.

::: servo/components/style/traversal.rs:587
(Diff revision 1)
>  
>          // If the child doesn't have pre-existing RestyleData and we don't have
>          // any reason to create one, avoid the useless allocation and move on to
>          // the next child.
>          if propagated_hint.is_empty() && !parent_inherited_style_changed &&
> -           !child_data.has_restyle()
> +           damage_handled.is_empty() && !child_data.has_restyle()

nit: (preexisting) brace on the same line.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> Comment on attachment 8837972 [details]
> Bug 1340022 - Implement "handled for descendants" tracking for RestyleDamage.
> 
> https://reviewboard.mozilla.org/r/112978/#review114476
> 
> I don't have a lot more to add than what heycam already did.
> 
> However, I think this optimization maps way more cleanly inside the
> RecreateStyleContexts pass, and it'd be probably easier to understand since
> we wouldn't have all the restyling logic around.
> 
> Also, the FFI call problem goes automatically away if we do this.
> 
> What do you think bobby, cam?

It needs to be done during restyle because there are several places where the restyle algorithm needs to know whether any ancestor has ReconstructFrame:
(1) To avoid calling CalcStyleDifference in that case (the optimization in this patch).
(2) To avoid restyling NAC that is going to disappear.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8837972 [details]
Bug 1340022 - Implement "handled for descendants" tracking for RestyleDamage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112978/diff/1-2/

Comment 10

[Bobby Holley (:bholley)]

This is green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=248c7f64189454055682802f036be810747ef0dc

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8837972 [details]
Bug 1340022 - Implement "handled for descendants" tracking for RestyleDamage.

https://reviewboard.mozilla.org/r/112978/#review114626

I guess you're right in that you can skip calling CalcStyleDifference.

I believe that's minor compared to the work of actually skipping the processing of the style change, but given it's still worth, I agree it's better to do it here.

r=me

::: servo/components/style/matching.rs:581
(Diff revision 2)
>                           new_values: &Arc<ComputedValues>,
>                           pseudo: Option<&PseudoElement>) {
> -        if restyle.damage != RestyleDamage::rebuild_and_reflow() {
> -            let d = self.compute_restyle_damage(&old_values, &new_values, pseudo);
> -            restyle.damage |= d;
> +        if restyle.damage_handled.contains(RestyleDamage::reconstruct()) {
> +            // If an ancestor is already getting reconstructed, no need to apply
> +            // damage.
> +            restyle.damage = RestyleDamage::empty();

I believe this would be slightly more readable if structured in the following way. No need to take any action if you think otherwise, this is mostly opinion.

```
// If an ancestor is already getting reconstructed, no need to apply
// damage.
if restyle.damage_handled.contains(RestyleDamage::reconstruct()) {
    restyle.damage = RestyleDamage::empty();
    return;
}

// Add restyle damage, but only the bits that aren't redundant with
// respect to damage applied on our ancestors.
if !restyle.damage.contains(RestyleDamage::reconstruct()) {
    let new_damage = self.compute_restyle_damage(&old_values, &new_values, pseudo);
    // ...
}
```

Though actually I'm not sure if the first condition holds for servo, given it looks at the reconstruct flags for the bottom.

Presumably the second condition applies to repaint and similar. Worth checking.

::: servo/components/style/matching.rs:726
(Diff revision 2)
>          if matches_different_pseudos {
>              rule_nodes_changed = true;
>              if let Some(r) = data.get_restyle_mut() {
>                  // Any changes to the matched pseudo-elements trigger
>                  // reconstruction.
> -                r.damage |= RestyleDamage::rebuild_and_reflow();
> +                r.damage |= RestyleDamage::reconstruct();

Are you sure this is ok for servo? I don't see anything that makes the reconstruct bit imply any of the rest. Worth trying, I guess, then we'd be able to get rid of rebuild_and_reflow in servo too, though that seems unlikely to me.

::: servo/components/style/servo/restyle_damage.rs:73
(Diff revision 2)
>      /// Returns a bitmask that represents a flow that needs to be rebuilt and
>      /// reflowed.
> +    ///
> +    /// FIXME(bholley): Do we ever actually need this? Shouldn't RECONSTRUCT_FLOW
> +    /// imply everything else?
>      pub fn rebuild_and_reflow() -> ServoRestyleDamage {

It seems like you got rid of rebuild_and_reflow for gecko, you don't remove this because there are callers in layout right?

Just to keep track of the IRC conversation we had about RECONSTRUCT: http://logs.glob.uno/?c=mozilla%23servo&s=16+Feb+2017&e=16+Feb+2017#c613239

(tl;dr; what we're doing seems quite inefficient, at the moment one fragment fails to be reconstructed, that causes `process` in the parent to be called, which causes the `HAS_NEWLY_CONSTRUCTED_FLOW` flag to be set, which causes this to go up to the top).

Though as said I guess it depends on the cost of actually calling the different construct_flow_for_xxx. 

They seem quite expensive though, requiring at least one iteration for every children.

Comment 12

[Bobby Holley (:bholley)]

Servo PR: https://github.com/servo/servo/pull/15599

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8837972 [details]
Bug 1340022 - Implement "handled for descendants" tracking for RestyleDamage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112978/diff/2-3/

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8837972 [details]
Bug 1340022 - Implement "handled for descendants" tracking for RestyleDamage.

https://reviewboard.mozilla.org/r/112978/#review115016

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd6b20156127
Implement "handled for descendants" tracking for RestyleDamage. r=emilio

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd6b20156127