Closed Bug 1289868 Opened 8 years ago Closed 7 years ago

stylo: Use the result of CalcStyleDifference to cull parallel DOM traversal

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bholley, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1289868 - Part 1: stylo: Add an outparam to Gecko_CalcStyleDifference that returns whether any style data changed.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1289868 - Part 2: stylo: Compare all structs in CalcStyleDifference so that Servo can accurately determine whether to stop traversal.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
CalcStyleDifference gives us a change hint as well as a bitmap of style structs that are value-equal and pointer-equal. There are various optimizations we can perform with this, like stopping recursion on the subtree if no inherited data was changed. Right now we don't do any of this in Gecko_CalcAndStoreStyleDifference, and we should.
A related optimization is stopping restyling once we generate nsChangeHint_ReconstructFrame.

A comment in RestyleManager.cpp claims that not producing new style contexts for elements that will be reframed is also important:

  http://searchfox.org/mozilla-central/rev/389a3e0817b873a64376ec2b65f6807afbba9d8f/layout/base/RestyleManager.cpp#3057-3059

although I am not sure if things will break if we don't do that, or if it is just to keep some assertions happy.
(In reply to Cameron McCormack (:heycam) from comment #1)
> A related optimization is stopping restyling once we generate
> nsChangeHint_ReconstructFrame.

Presumably you mean "stopping style context fixup", right? We still want to perform the cascade during the parallel traversal, since frame construction will need the computed styles.

I filed bug 1293478 about this.
Priority: -- → P2
Summary: Use the result of CalcStyleDifference to cull parallel DOM traversal → stylo: Use the result of CalcStyleDifference to cull parallel DOM traversal
Over to heycam as part of his incremental restyle work.
Assignee: nobody → cam
Priority: P2 → P1
Comment on attachment 8868479 [details]
Bug 1289868 - Part 1: stylo: Add an outparam to Gecko_CalcStyleDifference that returns whether any style data changed.

https://reviewboard.mozilla.org/r/140092/#review143448

::: layout/style/ServoBindings.cpp:355
(Diff revision 1)
> +  // when there is no other change hint generated.  (This isn't quite
> +  // the same as just skipping the NeutralChange removal that happens
> +  // at the end of CalcStyleDifference, since here we'll only ever
> +  // return NeutralChange by itself.)
> +  if (equalStructs != NS_STYLE_INHERIT_MASK) {
> +    return nsChangeHint_NeutralChange;

This is fine, though perhaps it's cleaner to return whether all the structs were equal as a `bool*` outparam or something? No big deal.
Attachment #8868479 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8868480 [details]
Bug 1289868 - Part 2: stylo: Compare all structs in CalcStyleDifference so that Servo can accurately determine whether to stop traversal.

https://reviewboard.mozilla.org/r/140094/#review143450

::: layout/style/nsStyleContext.cpp:1045
(Diff revision 1)
> +  bool checkUnrequestedServoStructs = mSource.IsServoComputedValues();
> +
>  #define DO_STRUCT_DIFFERENCE(struct_)                                         \
>    PR_BEGIN_MACRO                                                              \
>      const nsStyle##struct_* this##struct_ = PeekStyle##struct_();             \
> +    bool unrequstedStruct;                                                    \

nit: spelling: `unrequested`.

I'd probably just initialize it to false, and set it to true in the `checkUnrequestedServoStructs` branch.
Attachment #8868480 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8868481 [details]
style: Use RestyleDamage to determine whether we must continue cascading style changes to children.

https://reviewboard.mozilla.org/r/140096/#review143456

Looks good, though I'm unsure that using neutral change hints to represent "no style changed" is the right decission. Could I hear about why did you do it that way, and whether you considered other kind of approach?

I'd find it more straight-forward in general if we did something like renaming `accumulate_damage` to `diff_styles`, and making that return a

```
StyleDifference {
    style_change: StyleChange,
    damage: RestyleDamage,
}
```

with something like:


```
enum StyleChange {
    None,
    Changed,
}
```

(I'd expect us to extend `StyleChange` in the future, maybe, but if it's not, just a `bool` may do too).

::: servo/components/style/gecko/restyle_damage.rs:68
(Diff revision 1)
>          GeckoRestyleDamage(hint)
>      }
>  
> +    /// Returns a copy of this `GeckoRestyleDamage` without any neutral change
> +    /// hint.
> +    pub fn without_neutral_change(self) -> Self {

I'd sort of prefer to track whether the structs didn't change explicitly than backing it in `RestyleDamage`, what do you think?

::: servo/components/style/matching.rs:815
(Diff revision 1)
> +
> -            let new_damage = self.compute_restyle_damage(&old_values,
> +        let new_damage = self.compute_restyle_damage(&old_values,
> -                                                         &new_values,
> +                                                     &new_values,
> -                                                         pseudo);
> +                                                     pseudo);
> -            if !restyle.damage_handled.contains(new_damage) {
> -                restyle.damage |= new_damage;
> +
> +        let child_cascade_requirement = match new_damage.is_empty() {

just use `if`? If you think as a conditional it's a bit too verbose, you can do:

```
use ChildCascadeRequirement::*;
```

at the top of the function, and then:

```
let child_cascade_requirement =
    if new_damage.is_empty() { CanSkipCascade } else { MustCascade };
```

::: servo/components/style/matching.rs:835
(Diff revision 1)
>      fn accumulate_damage(&self,
>                           _shared_context: &SharedStyleContext,
>                           restyle: &mut RestyleData,
>                           old_values: &Arc<ComputedValues>,
>                           new_values: &Arc<ComputedValues>,
> -                         pseudo: Option<&PseudoElement>) {
> +                         pseudo: Option<&PseudoElement>) -> ChildCascadeRequirement {

nit: put the return value in its own line, here and below.

::: servo/components/style/matching.rs:839
(Diff revision 1)
>                           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;
> +                         pseudo: Option<&PseudoElement>) -> ChildCascadeRequirement {
> +        let new_damage = self.compute_restyle_damage(&old_values, &new_values, pseudo);
> +        restyle.damage |= new_damage;
> +
> +        match new_damage.is_empty() {

ditto here.

Though I don't remember from memory what are the semantics of servo's restyle damage, you might need to add a neutral change too in order for this to be correct, or return `MustCascade` unconditionally.

::: servo/components/style/matching.rs:1349
(Diff revision 1)
> -                        self.accumulate_damage(&context.shared,
> +                            self.accumulate_damage(&context.shared,
> -                                               data.restyle_mut(), &old,
> +                                                   data.restyle_mut(), &old,
> -                                               shared_style.values(), None);
> +                                                   shared_style.values(), None)
> -                    }
> +                        }
> +                        None => ChildCascadeRequirement::MustCascade,
> +                    };

So I've seen this block before... ;)

Do you think it makes sense to make accumulate_damage get an `old_values: Option<&ComputedValues>` instead?

::: servo/components/style/traversal.rs:615
(Diff revision 1)
>             element, compute_self, element.has_dirty_descendants(), data);
>  
>      // Compute style for this element if necessary.
>      if compute_self {
> -        compute_style(traversal, traversal_data, context, element, &mut data);
> +        match compute_style(traversal, traversal_data, context, element, &mut data) {
> +            ChildCascadeRequirement::MustCascade => inherited_style_changed = true,

nit: I find this assignment somewhat easy to miss, perhaps it'd be a bit more readable using braces and moving it to its own line.
Comment on attachment 8868481 [details]
style: Use RestyleDamage to determine whether we must continue cascading style changes to children.

https://reviewboard.mozilla.org/r/140096/#review143456

It was just the simplest way to funnel through the information without changing too much.  But I like the StyleDifference idea, so I'll do that (tomorrow).

> just use `if`? If you think as a conditional it's a bit too verbose, you can do:
> 
> ```
> use ChildCascadeRequirement::*;
> ```
> 
> at the top of the function, and then:
> 
> ```
> let child_cascade_requirement =
>     if new_damage.is_empty() { CanSkipCascade } else { MustCascade };
> ```

I've sort of come to feel that |if { ... } else { ...}| expressions aren't as nice to read as match statements.  Especially when it's split over three lines, although as you point out, I could fit it in one line with the use statement.  Happy to switch it back to an if statement though.

> So I've seen this block before... ;)
> 
> Do you think it makes sense to make accumulate_damage get an `old_values: Option<&ComputedValues>` instead?

I'll try that.
Comment on attachment 8868481 [details]
style: Use RestyleDamage to determine whether we must continue cascading style changes to children.

https://reviewboard.mozilla.org/r/140096/#review144038

Huh, I thought I had already reviewed this, but somehow it was still in my queue...

r=me, thanks!
Attachment #8868481 - Flags: review?(emilio+bugs) → review+
Attachment #8868481 - Attachment is obsolete: true
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/008cb503f35e
Part 1: stylo: Add an outparam to Gecko_CalcStyleDifference that returns whether any style data changed. r=emilio
https://hg.mozilla.org/integration/autoland/rev/da166dedd42e
Part 2: stylo: Compare all structs in CalcStyleDifference so that Servo can accurately determine whether to stop traversal. r=emilio
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/81eff766b8c3
Whitelist outparam in heap write hazard analysis on a CLOSED TREE. r=me
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: