Closed Bug 1368236 Opened 7 years ago Closed 7 years ago

stylo: Plan for killing RestyleData.

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 3 obsolete files)

Bug 1368236: Implement the "hints handled for descendants" optimization.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
RestyleData is this heap allocation where we store whatever transient restyle state we need.

Right now it contains a restyle hint, a change hint, the "damage handled for descendants", and a boolean that tells us we have to recascade.

I want to change a bit how we do restyle hints in order for it to be slimmer and more performant, but I'll file another bug to discuss that.

In the meantime, I think we could kill RestyleData quite easily.

In particular, there's no benefit now in having the "damage handled" optimization there instead of the post-traversal. Cameron made it so that we unconditionally call CalcStyleDifference to optimize the cascade anyway.

Without that, the remaining bits are:

 * RestyleDamage (32 bits)
 * The "Recascade" boolean (8 bits)
 * The "RestyleHint" (32 bits)

Merging the recascade boolean into RestyleHint (something that Cam was going to do I think) will make all the stuff under that less than a pointer, at which time we can just kill it.

My plan to handle restyle hints will make it just a bunch of bitflags, which also have this property.

WDYT Bobby?
Flags: needinfo?(bobbyholley)
Where will we store the information for more precise re-selector matching of descendants (be it Gecko-like storage of right-most selectors, or your suggestion of something based on the dependencies)?
See Also: → 1368240
(In reply to Cameron McCormack (:heycam) from comment #1)
> Where will we store the information for more precise re-selector matching of
> descendants (be it Gecko-like storage of right-most selectors, or your
> suggestion of something based on the dependencies)?

I just filed bug 1368240 for that :)
(In reply to Cameron McCormack (:heycam) from comment #1)
> Where will we store the information for more precise re-selector matching of
> descendants (be it Gecko-like storage of right-most selectors, or your
> suggestion of something based on the dependencies)?

In particular, given we have access to all the DOM we care about, we could do it while computing hints instead of propagating it down the tree copying it.
In general. this sounds great. (In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> In particular, there's no benefit now in having the "damage handled"
> optimization there instead of the post-traversal. Cameron made it so that we
> unconditionally call CalcStyleDifference to optimize the cascade anyway.

Yes, but it _is_ important because that's what allows us to avoid styling doomed NAC. So I think we'll want some other solution there, though we really only need a single bit to track whether we've got a reconstruct or not.

> WDYT Bobby?

In general this sounds great. I'm definitely of the opinion at this point that a heap-allocation is overkill and hurting us.
Flags: needinfo?(bobbyholley)
Blocks: 1371975
I thought the point of the "damage handled" bit was to avoid bloating the changelist and doing extra work invalidating or queueing reflows or whatnot.  Are we not managing to do any of that?
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #5)
> I thought the point of the "damage handled" bit was to avoid bloating the
> changelist and doing extra work invalidating or queueing reflows or whatnot.
> Are we not managing to do any of that?

We're not, but I plan to do that as part of this bug.
We might have to move it back to the Servo traversal, if per bug 1367904 comment 15 we get rid of the post traversal.
Blocks: 1372056
Priority: -- → P1
Last two commits have some orange on them, I'm investigating, so feel free to hold off review of those. I don't think the approach would change significantly though.
Comment on attachment 8877159 [details]
Bug 1368236: Kill StoredRestyleHint.

https://reviewboard.mozilla.org/r/148524/#review153070
Attachment #8877159 - Flags: review?(bobbyholley) → review+
Comment on attachment 8877160 [details]
Bug 1368236: Remove damage_handled, and use a reconstructed_ancestor bit instead.

https://reviewboard.mozilla.org/r/148526/#review153072
Attachment #8877160 - Flags: review?(bobbyholley) → review+
Comment on attachment 8877161 [details]
Bug 1368236: Inline RestyleData.

https://reviewboard.mozilla.org/r/148528/#review153076

Canceling review here for now given the orange.

It seems to me that RestyleData is still going to be bigger than a word. What happens if you run the size_of tests via |./mach test-stylo|? We should add a size_of test for RestyleData and try to pack it as small as possible.
Attachment #8877161 - Flags: review?(bobbyholley)
Comment on attachment 8877162 [details]
Bug 1368236: Implement the "hints handled for descendants" optimization.

https://reviewboard.mozilla.org/r/148530/#review153302
Attachment #8877162 - Flags: review?(cam) → review+
Attachment #8877159 - Attachment is obsolete: true
Attachment #8877160 - Attachment is obsolete: true
Comment on attachment 8877161 [details]
Bug 1368236: Inline RestyleData.

https://reviewboard.mozilla.org/r/148528/#review153564

r=me with those things addressed.

::: servo/components/style/data.rs:405
(Diff revision 4)
> +        // TODO(emilio): We should be able to assert this, except for
> +        // animation-only traversals.
> +        // debug_assert!(!self.reconstructed_ancestor(),
> +        //               "How did we knew this?");

The assertion message here doesn't seem helpful (and should be "know"). I'd just say:

// If it weren't for animation-only traversals, we
// could assert |!self.reconstructed_ancestor()| here.

::: servo/components/style/data.rs:414
(Diff revision 4)
> +    /// Mark this element as restyled, which is useful to know whether we need
> +    /// to do a post-traversal.
> +    pub fn set_restyled(&mut self, restyled: bool) {
> +        if restyled {
> +            self.flags.insert(WAS_RESTYLED);

This API seems rather footgun-y given that it doesn't clear the bit in the |false| case. How about just removing the boolean param and having the caller do an |if|?

::: servo/components/style/data.rs:422
(Diff revision 4)
> +    fn is_empty(&self) -> bool {
> +        self.hint.is_empty() && self.damage.is_empty() && self.flags.is_empty()
> +    }
> +
> +    fn contains_restyle_data(&self) -> bool {
> +        !self.is_empty()

The semantics of these two functions (is_empty and contains_restyle_data) seem kinda wishy-washy - for example, why does the caller care whether ANCESTOR_WAS_RECONSTRUCTED was set or not?

Why not just expose WAS_RESTYLED, which is the only thing the single callsite actually cares about?

::: servo/components/style/data.rs:468
(Diff revision 4)
> +    /// Whether this element was restyled, or has any other restyle state that
> +    /// may make us do a post-traversal.
> +    pub fn contains_restyle_data(&self) -> bool {
> +        self.restyle.contains_restyle_data()
> +    }

Given that RestyleData is pub, this forwarding accessor seems unnecessary.

::: servo/components/style/matching.rs:699
(Diff revision 4)
>          let skip_applying_damage =
> -            restyle.damage.contains(RestyleDamage::reconstruct()) ||
> +            restyle.reconstructed_self_or_ancestor();

This is a behavior change, I think, but seems like a harmless one.

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

This will cause us to add restyle damage even on initial styling, right? We need to guard against that (that's what the old code was doing).
Attachment #8877161 - Flags: review?(bobbyholley) → review+
Comment on attachment 8877161 [details]
Bug 1368236: Inline RestyleData.

https://reviewboard.mozilla.org/r/148528/#review154146

::: servo/components/style/data.rs:405
(Diff revision 4)
> +        // TODO(emilio): We should be able to assert this, except for
> +        // animation-only traversals.
> +        // debug_assert!(!self.reconstructed_ancestor(),
> +        //               "How did we knew this?");

Done.

::: servo/components/style/data.rs:414
(Diff revision 4)
> +    /// Mark this element as restyled, which is useful to know whether we need
> +    /// to do a post-traversal.
> +    pub fn set_restyled(&mut self, restyled: bool) {
> +        if restyled {
> +            self.flags.insert(WAS_RESTYLED);

Done.

::: servo/components/style/data.rs:422
(Diff revision 4)
> +    fn is_empty(&self) -> bool {
> +        self.hint.is_empty() && self.damage.is_empty() && self.flags.is_empty()
> +    }
> +
> +    fn contains_restyle_data(&self) -> bool {
> +        !self.is_empty()

I've reworded it, but you actually care about explicit change hints on the root.

::: servo/components/style/data.rs:468
(Diff revision 4)
> +    /// Whether this element was restyled, or has any other restyle state that
> +    /// may make us do a post-traversal.
> +    pub fn contains_restyle_data(&self) -> bool {
> +        self.restyle.contains_restyle_data()
> +    }

done

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

good catch :)
See Also: → 1373412
Attachment #8877161 - Attachment is obsolete: true
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/de9ea489717f
Implement the "hints handled for descendants" optimization. r=heycam
https://hg.mozilla.org/mozilla-central/rev/de9ea489717f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: