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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: bholley, Assigned: heycam)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

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.
(Assignee)

Comment 1

a year ago
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
Blocks: 1243581
Over to heycam as part of his incremental restyle work.
Assignee: nobody → cam
Priority: P2 → P1
(Assignee)

Comment 4

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=343c3140211de121c7f9f79ef501285bf99b2120
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 months ago
mozreview-review
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 9

2 months ago
mozreview-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 10

2 months ago
mozreview-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.
(Assignee)

Comment 11

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 months ago
mozreview-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/#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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8868481 - Attachment is obsolete: true
(Assignee)

Comment 18

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73e544fdd4693b0f7b54b3a9d9392b278f38bee3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 months ago
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

Comment 23

2 months ago
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
https://hg.mozilla.org/mozilla-central/rev/008cb503f35e
https://hg.mozilla.org/mozilla-central/rev/da166dedd42e
https://hg.mozilla.org/mozilla-central/rev/81eff766b8c3
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1372056
You need to log in before you can comment on or make changes to this bug.