1325734 - stylo: Give up on hoisting ElementData into the frame and eliminate the concept of consuming styles

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

In our architectural plan for stylo, one eventual goal was that ownership of computed style should be hoisted to the frame during frame construction such that we can drop ElementData and save memory.

Over time I've become convinced that this isn't the right approach:
* There's a lot of value in having the styles in the content tree, since it avoids the complexity of figuring out which frame has the style we want (i.e. nsFrame::CorrectStyleParentFrame).
* The Servo traversal is based on the content tree, and most of the style system algorithms need them. So it's really hard to avoid re-instantiating ElementData during the traversal, and trying to do so complicates a lot of the traversal in undesirable ways.
* We haven't really found a consistent way to think about the differences between the Initial(Some), Restyle, and Persistent variants of ElementData: they all have computed style data available, and most of the algorithms seem happiest just to use it.
* Reshaping the enum between its various forms is annoying and possibly inefficient.
* Hoisting computed style to the frame tree probably isn't feasible for Servo, and the divergence adds a lot of complexity.

I think a better architecture is to ditch the enum and make ElementData a regular struct like so:

struct ElementData {
  styles: Option<ElementStyles>,
  restyle: Option<Box<RestyleData>>
}

The RestyleData is a separate heap-allocated struct, which means that we can drop it when we're done with it, and in the steady state only pay the cost for the ElementStyles. This adds some memory overhead in the short-term, but as soon as we drop support for the old style system, we can shrink nsStyleContext significantly and claw this back (possibly even embedding it directly in the frame).

One consequence of this is that we can give up on the concept of "consuming" styles in general (which is a rather ambiguous concept right now), and instead only consume restyles (which can be pretty well defined as dropping the Box and applying the change hint). This should simplify a lot of the code around style resolution.

There are some tricky points in this change, in particular a few scattered callsites that depend on whether this the computed style is "initial" or not. But I think we can fix those, and this refactoring should make things cleaner.

Comment 1

[Bobby Holley (:bholley)]

Needinfoing myself to look at this after the holidays.

Comment 3

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47b3f11f3d2e2fda2cf9afb4582ef1fede0b6c9d

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Do a more specific check to determine whether an XBL binding has already been applied. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove Servo Layout's dependency on the initial-ness of the style. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 3 - Simplify ElementData and eliminate the concept of consuming styles. v1

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8824638 [details] [diff] [review]
Part 1 - Do a more specific check to determine whether an XBL binding has already been applied. v1

Review of attachment 8824638 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/traversal.rs
@@ +224,5 @@
> +        // drops Servo ElementData when the XBL insertion parent of an Element is
> +        // changed.
> +        if cfg!(feature = "gecko") &&
> +           parent_data.styles().primary.values.has_moz_binding() &&
> +           !parent.xbl_binding_is_applied()

What happens if there is a non-none -moz-binding property value, but it doesn't resolve to a valid <binding> element?  Will xbl_binding_is_applied() still return true in that case?  If not, then it seems like we would miss styling this subtree.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #8)
> Comment on attachment 8824638 [details] [diff] [review]
> Part 1 - Do a more specific check to determine whether an XBL binding has
> already been applied. v1
> 
> Review of attachment 8824638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/traversal.rs
> @@ +224,5 @@
> > +        // drops Servo ElementData when the XBL insertion parent of an Element is
> > +        // changed.
> > +        if cfg!(feature = "gecko") &&
> > +           parent_data.styles().primary.values.has_moz_binding() &&
> > +           !parent.xbl_binding_is_applied()
> 
> What happens if there is a non-none -moz-binding property value, but it
> doesn't resolve to a valid <binding> element?  Will xbl_binding_is_applied()
> still return true in that case?  If not, then it seems like we would miss
> styling this subtree.

Ah, interesting point! I guess we probably need something else here then.

I was originally going to just check where there was a primary frame, but it seemed like that could run into weird edge cases with display:contents and stuff. Do you think it would work?

Another thing we could do would be to track somehow if we've tried to apply the binding, even if it failed. We could use a bit (kinda wasteful), or some kind of tagged pointer in the DOM slot (complexity). Other ideas?

Comment 10

[Bobby Holley (:bholley)]

Oh and I guess we can always just fall back to tracking whether this is the initial styling or not (like we were doing before). It would just involve passing some state around, but shouldn't be that bad. The main downside is that it doesn't handle cases where we somehow traverse the tree again before trying to apply the binding. But maybe that's rare?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8824639 [details] [diff] [review]
Part 2 - Remove Servo Layout's dependency on the initial-ness of the style. v1

Review of attachment 8824639 [details] [diff] [review]:
-----------------------------------------------------------------

Not pretty, but no better ideas either :/

::: servo/components/layout/data.rs
@@ +47,5 @@
>      pub flags LayoutDataFlags: u8 {
>          #[doc = "Whether a flow has been newly constructed."]
> +        const HAS_NEWLY_CONSTRUCTED_FLOW = 0x01,
> +        #[doc = "Whether this node has been traversed by layout."]
> +        const HAS_BEEN_TRAVERSED = 0x02,

I'd probably use a IS_FIRST_REFLOW flag by default, then remove it, rather than inserting it all the time, but it's just personal preference so this is fine too.

::: servo/components/script_layout_interface/wrapper_traits.rs
@@ +248,5 @@
> +    /// barrier of ThreadSafeLayout wrapper layer, and can lead to races if not used
> +    /// carefully.
> +    ///
> +    /// We probably wouldn't need this if we didn't have this annoying trait separation
> +    /// between script and layout. :-(

Mention the exact reason too, which is that we need access to the layout data flags.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8824639 [details] [diff] [review]
Part 2 - Remove Servo Layout's dependency on the initial-ness of the style. v1

Review of attachment 8824639 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, I think I want a bit more explanation on the patch and its purpose...

If an element hasn't been restyled before, and it's not in a display: none subtree, I guess it'd be traversed by layout, and it'd have initial styles, so we can just return rebuild_and_reflow in that case.

I guess I can imagine you can force a restyle of an out-of-doc element, then inserting in the document, but we don't keep data either on out-of-doc elements.

May you give a clearer description on what situation can end up with layout traversing the node with a wrong restyle damage? Is it just for text nodes?

Sorry, I just want to make sure I understand it completely before granting r+

Also, I guess you could move those flags to the style data, right? It's a bit of bloat I suspect, but may be more elegant? (also, we probably need some styling-related flags at some point anyway?)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8824640 [details] [diff] [review]
Part 3 - Simplify ElementData and eliminate the concept of consuming styles. v1

Review of attachment 8824640 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/crashtests/crashtests.list
@@ +333,5 @@
>  load 522374-2.html
>  load 526378-1.xul
>  load 534367-1.xhtml
>  load 534368-1.xhtml
> +load 534768-1.html # bug 1324660

Delete the bug annotation if this is no longer an issue?

::: servo/components/style/data.rs
@@ +261,5 @@
>  /// Transient data used by the restyle algorithm. This structure is instantiated
>  /// either before or during restyle traversal, and is cleared at the end of node
>  /// processing.
>  ///
>  /// TODO(emilio): Tell bholley to document this more accurately. I can try (and

Remove this comment.

@@ +386,5 @@
> +        self.styles.as_mut().expect("Caling styles_mut() on unstyled ElementData")
> +    }
> +
> +    /// Sets the computed element styles.
> +    pub fn set_styles(&mut self, styles: ElementStyles) {

It'd be nice to assert here somehow that the element indeed needed to be restyled. Right now this would not be so straight-forward because we use the same RestyleData as an input and an output (for the restyle damage).

Should we split the input data and the damage, so we could clear the input (maybe except the snapshot) here, asserting that we had a proper restyle hint?

It might be fine to experiment with this as a followup though.

::: servo/components/style/matching.rs
@@ +660,5 @@
> +                        data.restyle_mut().damage |= d;
> +                    }
> +
> +                    // We never put elements with pseudo style into the style sharing cache,
> +                    // so we can just mint an ElementStyles directly here.

Reference the bug you filled yesterday for this?

::: servo/ports/geckolib/glue.rs
@@ +213,5 @@
> +        // Note that we check for has_restyle here rather than has_current_styles,
> +        // because we also want the traversal code to trigger if there's restyle
> +        // damage. We really only need the Gecko post-traversal in that case, so
> +        // the servo traversal will be a no-op, but it's cheap enough that we
> +        // don't bother distinguishing the two cases.

Maybe file a bug to measure how often this happens, and if it may be worth?

@@ +911,5 @@
>  {
>      let element = GeckoElement(element);
> +    let damage = if let Some(mut data) = element.mutate_data() {
> +        let d = data.get_restyle().map_or(GeckoRestyleDamage::empty(), |r| r.damage);
> +        data.clear_restyle();

We could rename this function to make clear that two calls to the function may not return the same change hint, how do you feel about it?

Comment 14

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Comment on attachment 8824639 [details] [diff] [review]
> Part 2 - Remove Servo Layout's dependency on the initial-ness of the style.
> v1
> 
> Review of attachment 8824639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, I think I want a bit more explanation on the patch and its
> purpose...
> 
> If an element hasn't been restyled before, and it's not in a display: none
> subtree, I guess it'd be traversed by layout, and it'd have initial styles,
> so we can just return rebuild_and_reflow in that case.

Right. That's the point of this patch. We need to distinguish between "just got initial styles", in which case the code expects rebuild_and_reflow(), or "already had styles and wasn't restyled", in which case the code expects none().
 
> I guess I can imagine you can force a restyle of an out-of-doc element, then
> inserting in the document, but we don't keep data either on out-of-doc
> elements.

Right, so that's not relevant here.

> May you give a clearer description on what situation can end up with layout
> traversing the node with a wrong restyle damage? Is it just for text nodes?

See above.

> Sorry, I just want to make sure I understand it completely before granting r+

No problem!
 
> Also, I guess you could move those flags to the style data, right?

They need to live in layout, because by the time flow construction happens, a HAS_BEEN_STYLED bit would already be true. Basically, we need a flag that gets cleared at the end of bottom-up processing, which must be in layout.

> It's a
> bit of bloat I suspect, but may be more elegant? (also, we probably need
> some styling-related flags at some point anyway?)

In general I'm hoping that we can just keep persistent style flags to a minimum, and use NodeFlags when we do need them.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #14)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Right. That's the point of this patch. We need to distinguish between "just
> got initial styles", in which case the code expects rebuild_and_reflow(), or
> "already had styles and wasn't restyled", in which case the code expects
> none().

Ok, I see, thanks for the explanation. So this is only for when we're reflowing nodes that haven't been restyled, makes sense.

I guess the comment could be more helpful if it enumerated the different cases we're handling in that function:

 (1) We're reflowing an element that just got a restyle and thus it has a proper calculated restyle damage.
 (2) We're reflowing a never previously node, in which case we need to rebuild and reflow.
 (3) We're reflowing a node that hasn't be restyled, but whose layout may have to change due to changes in its ancestors or descendants.

(but do as you wish, not blocking on that :))

> > Also, I guess you could move those flags to the style data, right?
> 
> They need to live in layout, because by the time flow construction happens,
> a HAS_BEEN_STYLED bit would already be true. Basically, we need a flag that
> gets cleared at the end of bottom-up processing, which must be in layout.

Sure, I meant the definition and storage, (no change needed on the usage), but fine if you want to keep them in the layout data :)

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8824639 [details] [diff] [review]
Part 2 - Remove Servo Layout's dependency on the initial-ness of the style. v1

Review of attachment 8824639 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the explanation Bobby, this looks good to me :)

Comment 17

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> Comment on attachment 8824639 [details] [diff] [review]
> Part 2 - Remove Servo Layout's dependency on the initial-ness of the style.
> v1
> 
> Review of attachment 8824639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not pretty, but no better ideas either :/
> 
> ::: servo/components/layout/data.rs
> @@ +47,5 @@
> >      pub flags LayoutDataFlags: u8 {
> >          #[doc = "Whether a flow has been newly constructed."]
> > +        const HAS_NEWLY_CONSTRUCTED_FLOW = 0x01,
> > +        #[doc = "Whether this node has been traversed by layout."]
> > +        const HAS_BEEN_TRAVERSED = 0x02,
> 
> I'd probably use a IS_FIRST_REFLOW flag by default, then remove it, rather
> than inserting it all the time, but it's just personal preference so this is
> fine too.

I agree that's a bit nicer, but the downside is that this means that the initial flag values would be non-zero, and I think it's generally better to keep the initial values zero where possible.

> Mention the exact reason too, which is that we need access to the layout
> data flags.

Added.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> I guess the comment could be more helpful if it enumerated the different
> cases we're handling in that function:

I've added this comment.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> ::: layout/base/crashtests/crashtests.list
> @@ +333,5 @@
> >  load 522374-2.html
> >  load 526378-1.xul
> >  load 534367-1.xhtml
> >  load 534368-1.xhtml
> > +load 534768-1.html # bug 1324660
> 
> Delete the bug annotation if this is no longer an issue?

Done.

> 
> ::: servo/components/style/data.rs
> @@ +261,5 @@
> >  /// Transient data used by the restyle algorithm. This structure is instantiated
> >  /// either before or during restyle traversal, and is cleared at the end of node
> >  /// processing.
> >  ///
> >  /// TODO(emilio): Tell bholley to document this more accurately. I can try (and
> 
> Remove this comment.

Done.

> 
> @@ +386,5 @@
> > +        self.styles.as_mut().expect("Caling styles_mut() on unstyled ElementData")
> > +    }
> > +
> > +    /// Sets the computed element styles.
> > +    pub fn set_styles(&mut self, styles: ElementStyles) {
> 
> It'd be nice to assert here somehow that the element indeed needed to be
> restyled. Right now this would not be so straight-forward because we use the
> same RestyleData as an input and an output (for the restyle damage).
> 
> Should we split the input data and the damage, so we could clear the input
> (maybe except the snapshot) here, asserting that we had a proper restyle
> hint?
> 
> It might be fine to experiment with this as a followup though.

Yeah, I'd rather experiment with these kinds of assertions once we have better CI coverage so we can see what breaks. I think we're not quite at that level of refinement yet.

> 
> ::: servo/components/style/matching.rs
> @@ +660,5 @@
> > +                        data.restyle_mut().damage |= d;
> > +                    }
> > +
> > +                    // We never put elements with pseudo style into the style sharing cache,
> > +                    // so we can just mint an ElementStyles directly here.
> 
> Reference the bug you filled yesterday for this?

Done.

> 
> ::: servo/ports/geckolib/glue.rs
> @@ +213,5 @@
> > +        // Note that we check for has_restyle here rather than has_current_styles,
> > +        // because we also want the traversal code to trigger if there's restyle
> > +        // damage. We really only need the Gecko post-traversal in that case, so
> > +        // the servo traversal will be a no-op, but it's cheap enough that we
> > +        // don't bother distinguishing the two cases.
> 
> Maybe file a bug to measure how often this happens, and if it may be worth?

Given how cheap a no-op traversal is, I think we'd only want to do that kind of micro-optimization if we notice it during performance testing - So I think we should let the profiler guide us there.
> 
> @@ +911,5 @@
> >  {
> >      let element = GeckoElement(element);
> > +    let damage = if let Some(mut data) = element.mutate_data() {
> > +        let d = data.get_restyle().map_or(GeckoRestyleDamage::empty(), |r| r.damage);
> > +        data.clear_restyle();
> 
> We could rename this function to make clear that two calls to the function
> may not return the same change hint, how do you feel about it?

Yeah good idea - I've renamed it to Servo_TakeChangeHint().

Comment 19

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> Oh and I guess we can always just fall back to tracking whether this is the
> initial styling or not (like we were doing before). It would just involve
> passing some state around, but shouldn't be that bad. The main downside is
> that it doesn't handle cases where we somehow traverse the tree again before
> trying to apply the binding. But maybe that's rare?

I ended up deciding to do this, and tracking it in the TLS. But this took me down some other rabbit holes which I'm still unwinding. More tomorrow.

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove Servo Layout's dependency on the initial-ness of the style. r=emilio

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 2 - Parameterize the style traversal on TElement instead of TNode. v1

This works around the issue described in https://github.com/rust-lang/rust/issues/38917

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 3 - Pass the thread-local context into should_traverse_children. v1

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 4 - Explicitly track whether we're performing the initial style. v1

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 5 - Simplify ElementData and eliminate the concept of consuming styles. r=emilio

Comment 25

[Bobby Holley (:bholley)]

Attachment: Part 4 - Explicitly track whether we're performing the initial style. v2

Comment 26

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=927ea372d9c9986c89a845b0cc09bf085c3bd542

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8824800 [details] [diff] [review]
Part 2 - Parameterize the style traversal on TElement instead of TNode. v1

Review of attachment 8824800 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine :)

::: servo/components/style/traversal.rs
@@ +85,5 @@
>  
>      /// Process `node` on the way up, after its children have been processed.
>      ///
>      /// This is only executed if `needs_postorder_traversal` returns true.
> +    fn process_postorder(&self, thread_local: &mut Self::ThreadLocalContext,

nit: Put the thread_local argument in its own line.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8824802 [details] [diff] [review]
Part 3 - Pass the thread-local context into should_traverse_children. v1

Review of attachment 8824802 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/sequential.rs
@@ +29,5 @@
>                  *depth += 1;
>              }
>  
> +            traversal.traverse_children(thread_local, el,
> +                                        |tlc, kid| doit(traversal, traversal_data, tlc, kid));

nit: maybe it's a bit more legible using braces like:

traversal.traverse_children(thread_local, el, |tlc, kid| {
    doit(traversal, traversal_data, tlc, kid));
});

::: servo/components/style/traversal.rs
@@ +193,5 @@
>      /// this to cull traversal of various subtrees.
>      ///
>      /// This may be called multiple times when processing an element, so we pass
>      /// a parameter to keep the logs tidy.
> +    fn should_traverse_children(&self, _thread_local: &mut ThreadLocalStyleContext<E>,

nit: each argument in its own line.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8824805 [details] [diff] [review]
Part 4 - Explicitly track whether we're performing the initial style. v2

Review of attachment 8824805 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing the review flag for now, I want your opinion on the following.

::: servo/components/style/traversal.rs
@@ +245,5 @@
>          // Check if we're allowed to traverse past this element.
> +        let should_traverse =
> +            self.should_traverse_children(thread_local.borrow_mut(), parent,
> +                                          &parent.borrow_data().unwrap(), MayLog);
> +        thread_local.borrow_mut().end_element(parent);

I believe is kind of counter-intuitive to have this call here (and in general, to have the transient state in the local style context). Feels like a hack.

Could we instead make something like:

 * Add a new type parameter to the dom traversal: PreorderProcessingResult. Make process_preorder return that, and traverse_children get that as a parameter (and should_traverse_children too).
 * Make recalc_style_at return a StyleCalculationResult { was_initial_style: bool, }
 * Make process_preorder return an Option<StyleCalculationResult>, which will be our PreorderProcessingResult.
 * Check that result in should_traverse_children.

I think that'd be a bit cleaner and easier to maintain/extend than stashing the data in the local style context just for this purpose, what do you think?

@@ +251,4 @@
>              return;
>          }
>  
> +

nit: Spurious newline?

Comment 30

[Bobby Holley (:bholley)]

Comment on attachment 8824805 [details] [diff] [review]
Part 4 - Explicitly track whether we're performing the initial style. v2

(In reply to Emilio Cobos Álvarez [:emilio] from comment #29)
> Comment on attachment 8824805 [details] [diff] [review]
> Part 4 - Explicitly track whether we're performing the initial style. v2
> 
> Review of attachment 8824805 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Clearing the review flag for now, I want your opinion on the following.
> 
> ::: servo/components/style/traversal.rs
> @@ +245,5 @@
> >          // Check if we're allowed to traverse past this element.
> > +        let should_traverse =
> > +            self.should_traverse_children(thread_local.borrow_mut(), parent,
> > +                                          &parent.borrow_data().unwrap(), MayLog);
> > +        thread_local.borrow_mut().end_element(parent);
> 
> I believe is kind of counter-intuitive to have this call here (and in
> general, to have the transient state in the local style context). Feels like
> a hack.
> 
> Could we instead make something like:
> 
>  * Add a new type parameter to the dom traversal: PreorderProcessingResult.
> Make process_preorder return that, and traverse_children get that as a
> parameter (and should_traverse_children too).
>  * Make recalc_style_at return a StyleCalculationResult { was_initial_style:
> bool, }
>  * Make process_preorder return an Option<StyleCalculationResult>, which
> will be our PreorderProcessingResult.
>  * Check that result in should_traverse_children.
> 
> I think that'd be a bit cleaner and easier to maintain/extend than stashing
> the data in the local style context just for this purpose, what do you think?

So, the reason I did things this way (and went down all the rabbit holes in parts 2 and 3) is that I think we're currently suffering from an overload of data container types.

We've got SharedStyleContext (closely associated with DomTraversal). Then we've got ThreadLocalStyleContext. We've got StyleContext to hook them together. We've also got PerLevelTraversalData. And ElementData. And then there's a long tail of little things like PreTraverseToken. They all have slightly-different lifetime characteristics that prevents us from merging them together, and so we need to pass lots of things around.

All of this is a lot to grok when reading the code (evidenced by the fact that bz filed [1]). I think most of the complexity of the existing setup is necessary and warranted, but it made me reluctant to add yet _another_ data structure to ferry this information from the beginning of recalc_style_at to the traversal code at the end of top_down_dom, and pass yet another argument to all the functions it needs to reach.

Moreover, I think this usage of ThreadLocalStyleContext actually makes sense here. ThreadLocalStyleContext exists because we want some kind of ambient context that is lock-free mutable (which SharedStyleContext is not). We store the context in TLS so that we can persist cache data across different elements, but I don't think that means that means that we should restrict usage of the structure to _only_ things that must be passed from one traversed element to the next. We already pass this thing in most of the right places, so I think it's totally reasonable to use it for data that we want to ferry around within the scope of processing a single element as well.

The one concern with this approach is that data could accidentally be persisted longer than we intended, but the machinery I added in this patch gives us pretty good assertions against that happening.

[1] https://github.com/servo/servo/issues/14765

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8824805 [details] [diff] [review]
Part 4 - Explicitly track whether we're performing the initial style. v2

Review of attachment 8824805 [details] [diff] [review]:
-----------------------------------------------------------------

I sincerely don't believe this patch makes the logic or the data flow simpler to understand in any sense, and that something like what I proposed (call it |RestyleResult|, or whatever), makes sense in this context.

Also, I don't believe it's the issue Boris filled is incompatible at all with what I'm proposing here FWIW :P.

I guess feedback from other people would be helpful (flagging cam here for that), but I guess it's ok to land this for now and refactor it later if we change our mind.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8824805 [details] [diff] [review]
Part 4 - Explicitly track whether we're performing the initial style. v2

(I forgot to flag cam of course)

This feedback request shouldn't block landing IMO.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8824805 [details] [diff] [review]
Part 4 - Explicitly track whether we're performing the initial style. v2

Review of attachment 8824805 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/context.rs
@@ +89,5 @@
>      /// applying to them.
>      pub default_computed_values: Arc<ComputedValues>,
>  }
>  
> +struct CurrentElementInfo {

Oh, one last request, could you mention here how this is used? i.e:

Information for the current element being restyled. This is stored temporarily in the thread-local context during restyle and dropped before children processing.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f650d9f42ff5
Simplify ElementData and eliminate the concept of consuming styles. r=emilio

Comment 35

[Carsten Book [:Tomcat]]

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