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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bholley, Assigned: heycam)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
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•8 years 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.
Reporter | ||
Comment 2•8 years ago
|
||
(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.
Reporter | ||
Updated•8 years ago
|
Priority: -- → P2
Updated•8 years ago
|
Summary: Use the result of CalcStyleDifference to cull parallel DOM traversal → stylo: Use the result of CalcStyleDifference to cull parallel DOM traversal
Reporter | ||
Comment 3•8 years ago
|
||
Over to heycam as part of his incremental restyle work.
Assignee: nobody → cam
Priority: P2 → P1
Assignee | ||
Comment 4•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Attachment #8868481 -
Attachment is obsolete: true
Assignee | ||
Comment 18•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•7 years 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•7 years 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
Comment 24•7 years ago
|
||
bugherder |
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
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•