1370719 - stylo: Move the Rule node pointer into ComputedValues

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

This will save us two words on ElementData, on top of what we save in bug 1370711.

It's possible that there might be some reason this is more complicated than we think, and if so we should re-evaluate. But so far it seems like a reasonable thing to do.

Comment 1

[Bobby Holley (:bholley)]

jryans, can you look into this?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> jryans, can you look into this?

Sure, I'll investigate!

Comment 3

[Bobby Holley (:bholley)]

Note that you can probably avoid a lot of refactoring by temporarily storing the rule node in the CurrentElementInfo between the point when it's computed and the point where we have a ComputedValue to stash in the ElementData.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

In ComputedStyle today, we have 2 StrongRuleNodes (1 wrapped in Option) and 2 Option<Arc<ComputedValues>>, which add up to 32 bytes in ElementData.  In this bug, we're talking about pushing the rule nodes inside each ComputedValues.  For extra savings, the visited values in ComputedStyle is a temporary value to ease data flow, so it could be removed.

The potential benefit of this work is shrinking ComputedStyle by 24 bytes down to 8 bytes (just a single Arc<ComputedValues>).  Each ComputedValues would grow by 8 bytes (each element may have 1 or 2 of those depending on visited).  Possibly the smaller ElementData would improve perf here by fitting better in a cache line, but I don't think we have clear evidence that this matters yet.

After attempting this for a bit, I am getting worried that it makes the data flow a lot harder to reason about.

There are various paths through matching / cascading, such as: (a) full match and cascade, (b) replace rules, re-cascade, (c) re-cascade without rule changes.  At the moment, they can all look to ComputedStyle for rules and values.  This change means that rules are stored permanently inside ComputedValues after the cascade, but sometimes referenced temporarily off CurrentElementInfo before the cascade.  The different paths through matching / cascading feed through common functions that can no longer assume where the "newest" rules are located since it's unclear what work has already happened at each step.

So, we could certainly sprinkle various "ensure you've got the latest rules" calls around, but it definitely feels more complex to me while I am working on it, and surely when trying to make future changes as well.  Should I continue to push forward here?  My feeling is we have higher priority issues for now, but we could return to this in the future.

Comment 5

[Bobby Holley (:bholley)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> In ComputedStyle today, we have 2 StrongRuleNodes (1 wrapped in Option) and
> 2 Option<Arc<ComputedValues>>, which add up to 32 bytes in ElementData.  In
> this bug, we're talking about pushing the rule nodes inside each
> ComputedValues.  For extra savings, the visited values in ComputedStyle is a
> temporary value to ease data flow, so it could be removed.
> 
> The potential benefit of this work is shrinking ComputedStyle by 24 bytes
> down to 8 bytes (just a single Arc<ComputedValues>).  Each ComputedValues
> would grow by 8 bytes (each element may have 1 or 2 of those depending on
> visited).  Possibly the smaller ElementData would improve perf here by
> fitting better in a cache line, but I don't think we have clear evidence
> that this matters yet.
> 
> After attempting this for a bit, I am getting worried that it makes the data
> flow a lot harder to reason about.
> 
> There are various paths through matching / cascading, such as: (a) full
> match and cascade, (b) replace rules, re-cascade, (c) re-cascade without
> rule changes.  At the moment, they can all look to ComputedStyle for rules
> and values.  This change means that rules are stored permanently inside
> ComputedValues after the cascade, but sometimes referenced temporarily off
> CurrentElementInfo before the cascade.  The different paths through matching
> / cascading feed through common functions that can no longer assume where
> the "newest" rules are located since it's unclear what work has already
> happened at each step.
> 
> So, we could certainly sprinkle various "ensure you've got the latest rules"
> calls around, but it definitely feels more complex to me while I am working
> on it, and surely when trying to make future changes as well.  Should I
> continue to push forward here?  My feeling is we have higher priority issues
> for now, but we could return to this in the future.

So, I certainly agree that we shouldn't make the code significantly more complex or fragile on account of this. But I don't totally follow why this would actually happen.

In my mental model, we'd just expose a method on StyleContext called current_rule_node, which checks whether we have one cached in the CurrentElementInfo, and if not, forwards to the ComputedStyles (which would presumably need to be passed in as an arg). Is there some reason that wouldn't be easy?

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Chatted a bit more on IRC, I'll keep going with this.

I think I was losing confidence that there was actually a benefit overall.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:bholley noted that this will also help with bug 1367635, which would need a rule node in the ComputedValues.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1370719 - Copy ComputedStyles to context.

Extract the general collection operations of `EagerPseudoStyles` to a generic
`EagerPseudoMap` type that can hold arbitrary values.

This will be used in the next patch to hold the permanent `ComputedValues` for
pseudo-elements.

Review commit: https://reviewboard.mozilla.org/r/149986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149986/

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

Before this change, the `ComputedStyle` struct that is part of permanent style
data per element holds 2 `StrongRuleNode`s (unvisited and visited) and 2
`Arc<ComputedValues>` (unvisited and visited).

Both rule nodes and the visited values don't actually need to be here.  This
patch moves these 3 as part of `ComputedStyle` to temporary storage on
`CurrentElementInfo` during the match and cascade process.  Rule nodes are
pushed down inside the `ComputedValues` for later access after the cascade.
(Visited values were already available there.)

The permanent style data per element now has just the `Arc<ComputedValues>` for
itself and eager pseudo-elements (plus the `RestyleHint`).

Review commit: https://reviewboard.mozilla.org/r/149988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149988/

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8878683 [details]
Bug 1370719 - Copy ComputedStyles to context.

https://reviewboard.mozilla.org/r/149986/#review154726

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8878684 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/149988/#review154732

I've only skimmed the patch, but a few comments:
* ElementValues is too Option-happy. We have an Option on the outer struct and and Option on both of the inner ones. This means that the NonZero optimization has nowhere to stick the discriminant, so it needs to add an extra word. We should remove one of those Options (presumably the outer one), at which point ElementData should shrink. Really, ElementData should be 24 bytes, because there are 3 words (the primary computed values, the lazily-allocated pseudo array, and the RestyleData). What are we missing?
* The way you've reused the PseudoStyles stuff for the CurrentElementInfo causes us to do a transient heap-allocation, which isn't good. We should just store it inline.
* I don't think it really makes sense to use the old data structure names, specifically ComputedStyles, to store things that just have the rule nodes, because the actual computed values haven't been generated yet. Perhaps StylePrerequisites?
* In general I'd rather the temporary stuff moved to context.rs, and keep data.rs for the persistent pieces.
* We probably don't need size_of tests for the ephemeral structs, since they're stack-allocated and temporary.

Comment 13

[Bobby Holley (:bholley)]

It's probably easier to address the above and then have me look at it again. Thanks for working on this!

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8878683 [details]
Bug 1370719 - Copy ComputedStyles to context.

Canceling review on this for now since it's not clear to me that we need this given my comments on part 2.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12)
> Comment on attachment 8878684 [details]
> Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.
> 
> https://reviewboard.mozilla.org/r/149988/#review154732
> 
> I've only skimmed the patch, but a few comments:
> * ElementValues is too Option-happy. We have an Option on the outer struct
> and and Option on both of the inner ones. This means that the NonZero
> optimization has nowhere to stick the discriminant, so it needs to add an
> extra word. We should remove one of those Options (presumably the outer
> one), at which point ElementData should shrink. Really, ElementData should
> be 24 bytes, because there are 3 words (the primary computed values, the
> lazily-allocated pseudo array, and the RestyleData). What are we missing?

Good points!  I removed the outer option, since that seems liked the simpler one to remove, so that gets us down to 32 bytes from ElementData.

The other issue was that the type holding the eager pseudo values was declared as a slice, so it was being stored as (ptr, len) and taking up 16 bytes.  Since the length is constant, I changed it to an array instead, and now we're at the expected 24 bytes for ElementData.

> * The way you've reused the PseudoStyles stuff for the CurrentElementInfo
> causes us to do a transient heap-allocation, which isn't good. We should
> just store it inline.

Good catch, I've changed this to use an array inline now.

> * I don't think it really makes sense to use the old data structure names,
> specifically ComputedStyles, to store things that just have the rule nodes,
> because the actual computed values haven't been generated yet. Perhaps
> StylePrerequisites?

I went with `CascadeInputs`, which seemed pretty clear and also short.

> * In general I'd rather the temporary stuff moved to context.rs, and keep
> data.rs for the persistent pieces.

Makes sense, I've moved it.  I'll attach a "prequel" patch that just copies `ComputedStyle` there first, to make it more obvious what is new code.

> * We probably don't need size_of tests for the ephemeral structs, since
> they're stack-allocated and temporary.

Okay, removed.

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1370719 - Shrink ElementData by moving pseudo count to type.

`ElementValues` holds an optional list of values for each eager pseudo-element.
However, the type was declared as a slice instead of a fixed size array, so an
extra 8 bytes were being allocated to hold the size, even though it never
changes.

Moving the constant size into the type reduces `ElementValues` and `ElementData`
by 8 bytes.

Review commit: https://reviewboard.mozilla.org/r/151090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151090/

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8878683 [details]
Bug 1370719 - Copy ComputedStyles to context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149986/diff/1-2/

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8878684 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149988/diff/1-2/

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1370719 - Copy ComputedStyle to context.

In the patch after this, the existing `ComputedStyles` type is renamed and
repurposed as temporary storage of inputs for the cascade.  To make it a bit
easier to follow what code is new, this patch starts by copying `ComputedStyles`
to context.rs without changes.

Review commit: https://reviewboard.mozilla.org/r/151490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151490/

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

Before this change, the `ComputedStyle` struct that is part of permanent style
data per element holds 2 `StrongRuleNode`s (unvisited and visited) and 2
`Arc<ComputedValues>` (unvisited and visited).

Both rule nodes and the visited values don't actually need to be here.  This
patch moves these 3 to new temporary storage in `CascadeInputs` on
`CurrentElementInfo` during the match and cascade process.  Rule nodes are
pushed down inside the `ComputedValues` for later access after the cascade.
(Visited values were already available there.)

The permanent style data per element now has just the `Arc<ComputedValues>` for
itself and eager pseudo-elements (plus the `RestyleHint`).

Review commit: https://reviewboard.mozilla.org/r/151492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151492/

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1370719 - Shrink ElementData by moving pseudo count to type.

`ElementValues` holds an optional list of values for each eager pseudo-element.
However, the type was declared as a slice instead of a fixed size array, so an
extra 8 bytes were being allocated to hold the size, even though it never
changes.

Moving the constant size into the type reduces `ElementValues` and `ElementData`
by 8 bytes.

Review commit: https://reviewboard.mozilla.org/r/151494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151494/

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 8880110 [details]
Bug 1370719 - Copy ComputedStyle to context.

https://reviewboard.mozilla.org/r/151490/#review156410

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 8880110 [details]
Bug 1370719 - Copy ComputedStyle to context.

https://reviewboard.mozilla.org/r/151490/#review156412

::: commit-message-85070:1
(Diff revision 1)
> +Bug 1370719 - Copy ComputedStyles to context. r=bholley
> +
> +In the patch after this, the existing `ComputedStyles` type is renamed and
> +repurposed as temporary storage of inputs for the cascade.  To make it a bit
> +easier to follow what code is new, this patch starts by copying `ComputedStyles`
> +to context.rs without changes.

Oh, nit: The struct name is ComputedStyle, not ComputedStyles. Please fix up the title and commit message.

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8880110 [details]
Bug 1370719 - Copy ComputedStyle to context.

https://reviewboard.mozilla.org/r/151490/#review156412

> Oh, nit: The struct name is ComputedStyle, not ComputedStyles. Please fix up the title and commit message.

Ah, good catch!

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 8880112 [details]
Bug 1370719 - Shrink ElementData by moving pseudo count to type.

https://reviewboard.mozilla.org/r/151494/#review156414

Nice find. :-)

::: commit-message-ce61e:3
(Diff revision 1)
> +Bug 1370719 - Shrink ElementData by moving pseudo count to type. r=bholley
> +
> +`ElementValues` holds an optional list of values for each eager pseudo-element.

Nit - this should be ElementStyles given the rename you're doing now.

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8880110 [details]
Bug 1370719 - Copy ComputedStyle to context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151490/diff/1-2/

Comment 28

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151492/diff/1-2/

Comment 29

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8880112 [details]
Bug 1370719 - Shrink ElementData by moving pseudo count to type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151494/diff/1-2/

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151492/diff/2-3/

Comment 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1370719 - Shrink ElementData by moving pseudo count to type.

`ElementStyles` holds an optional list of values for each eager pseudo-element.
However, the type was declared as a slice instead of a fixed size array, so an
extra 8 bytes were being allocated to hold the size, even though it never
changes.

Moving the constant size into the type reduces `ElementStyles` and `ElementData`
by 8 bytes.

Review commit: https://reviewboard.mozilla.org/r/151556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151556/

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151492/diff/3-4/

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8880188 [details]
Bug 1370719 - Shrink ElementData by moving pseudo count to type.

https://reviewboard.mozilla.org/r/151556/#review156506

I addressed the nit about values vs. styles.  Not sure why this went back to a review request...

Comment 34

[Emilio Cobos Álvarez (:emilio)]

So, few things, from poking at the patch a bit for the lifetime issue:

The lifetime issue seems unavoidable as things stand. But the root cause is that still we have that apply_declarations call from Servo's animation code, which instead of creating a rule node, it goes and applies them directly. Seems like we should really fix that and use a rule node there instead. Then all the iterator madness is moot and can go away, and the cascade() function can just take rule nodes.

That being said, there are quite a few things that look fishy in this patch. In particular:

 pub fn for_inheritance(parent: &'a ComputedValues, default: &'a ComputedValues) -> Self {
      Self::new(parent.rules.clone(), ...)

That bit is plain wrong. Something that inherits from a style doesn't have any rule applied to it. It should use the rule tree root instead.

Third, has anybody measured this is an actual memory win?

Not that we shouldn't do it (seems beneficial for code health in general), but just that the memory win (assuming that's the real reason for this patch) seems less obvious when you take into account that this patch is growing text styles...

Anyway, I think we should really fix the apply_declarations fix instead of hacking around it though.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #34)
> So, few things, from poking at the patch a bit for the lifetime issue:
> 
> The lifetime issue seems unavoidable as things stand. But the root cause is
> that still we have that apply_declarations call from Servo's animation code,
> which instead of creating a rule node, it goes and applies them directly.
> Seems like we should really fix that and use a rule node there instead. Then
> all the iterator madness is moot and can go away, and the cascade() function
> can just take rule nodes.
> 
> That being said, there are quite a few things that look fishy in this patch.
> In particular:
> 
>  pub fn for_inheritance(parent: &'a ComputedValues, default: &'a
> ComputedValues) -> Self {
>       Self::new(parent.rules.clone(), ...)
> 
> That bit is plain wrong. Something that inherits from a style doesn't have
> any rule applied to it. It should use the rule tree root instead.

BTW, The most elegant way to fix this at a glance seems making `rules` non optional (just holding the rule tree root instead), and using default.rules.clone() instead, btw. Why would it be optional anyway?

Comment 36

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #34)
> So, few things, from poking at the patch a bit for the lifetime issue:
> 
> The lifetime issue seems unavoidable as things stand. But the root cause is
> that still we have that apply_declarations call from Servo's animation code,
> which instead of creating a rule node, it goes and applies them directly.
> Seems like we should really fix that and use a rule node there instead. Then
> all the iterator madness is moot and can go away, and the cascade() function
> can just take rule nodes.
> 
> That being said, there are quite a few things that look fishy in this patch.
> In particular:
> 
>  pub fn for_inheritance(parent: &'a ComputedValues, default: &'a
> ComputedValues) -> Self {
>       Self::new(parent.rules.clone(), ...)
> 
> That bit is plain wrong. Something that inherits from a style doesn't have
> any rule applied to it. It should use the rule tree root instead.

I agree that we shouldn't use the parent rule node here, but don't agree with using the root node as a sentinel. Using the root node means highly-contended atomic refcount traffic, whereas using None is free. And since we have the nonzero optimization, they take the same amount of space.

> 
> Third, has anybody measured this is an actual memory win?
> 
> Not that we shouldn't do it (seems beneficial for code health in general),
> but just that the memory win (assuming that's the real reason for this
> patch) seems less obvious when you take into account that this patch is
> growing text styles...

The memory win is a partial motivation, but not the only one - we're going to need it for bug 1368291.

That said, I would be very surprised if the impact on ComputedValues size ended up outweighing here the benefit here. Our style sharing is getting quite good - on wikipedia, we're sharing styles for about 85% of the elements, and with bug 1368290 those wins will translate to text nodes and anonymous boxes.

Finally, with bug 1367635, the rulenode-in-computedvalues will save us even more memory by allowing us to cache the reset structs in the rule tree.
 
> Anyway, I think we should really fix the apply_declarations fix instead of
> hacking around it though.

I think at this point that should be a followup. jryans, can you file?

Comment 37

[Bobby Holley (:bholley)]

Comment on attachment 8880188 [details]
Bug 1370719 - Shrink ElementData by moving pseudo count to type.

https://reviewboard.mozilla.org/r/151556/#review156540

Comment 38

[Bobby Holley (:bholley)]

Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/151492/#review156562

::: servo/components/style/animation.rs:505
(Diff revision 4)
>  
>              // This currently ignores visited styles, which seems acceptable,
>              // as existing browsers don't appear to animate visited styles.
>              let computed =
>                  properties::apply_declarations(context.stylist.device(),
> +                                               previous_style.rules(),

Please file a followup for fixing this up?

::: servo/components/style/data.rs:107
(Diff revision 4)
> -    /// Returns whether there are any pseudo styles.
> +    /// Returns whether there are any pseudo values.
>      pub fn is_empty(&self) -> bool {
>          self.0.is_none()
>      }
>  
> -    /// Returns a reference to the style for a given eager pseudo, if it exists.
> -    pub fn get(&self, pseudo: &PseudoElement) -> Option<&ComputedStyle> {
> +    /// Returns a reference to the values for a given eager pseudo, if it exists.
> +    pub fn get(&self, pseudo: &PseudoElement) -> Option<&Arc<ComputedValues>> {
>          debug_assert!(pseudo.is_eager());
>          self.0.as_ref().and_then(|p| p[pseudo.eager_index()].as_ref())
>      }
>  
> -    /// Returns a mutable reference to the style for a given eager pseudo, if it exists.
> -    pub fn get_mut(&mut self, pseudo: &PseudoElement) -> Option<&mut ComputedStyle> {
> +    /// Returns a mutable reference to the values for a given eager pseudo, if it exists.
> +    pub fn get_mut(&mut self, pseudo: &PseudoElement) -> Option<&mut Arc<ComputedValues>> {
>          debug_assert!(pseudo.is_eager());
>          self.0.as_mut().and_then(|p| p[pseudo.eager_index()].as_mut())
>      }
>  
> -    /// Returns true if the EagerPseudoStyles has a ComputedStyle for |pseudo|.
> +    /// Returns true if the EagerPseudoStyles has the values for |pseudo|.
>      pub fn has(&self, pseudo: &PseudoElement) -> bool {
>          self.get(pseudo).is_some()
>      }
>  
> -    /// Inserts a pseudo-element. The pseudo-element must not already exist.
> -    pub fn insert(&mut self, pseudo: &PseudoElement, style: ComputedStyle) {
> +    /// Sets the values for the eager pseudo.
> +    pub fn set(&mut self, pseudo: &PseudoElement, value: Arc<ComputedValues>) {

Nit: all this "values" renaming really isn't necessary.

::: servo/components/style/matching.rs:1388
(Diff revision 4)
> +        let mut primary_inputs = context.thread_local.current_element_info
> +                                        .as_mut().unwrap()
> +                                        .cascade_inputs.primary_mut();

Seems like we do this incantation a lot. How about making a helper cascade_inputs() method on context?

::: servo/components/style/properties/gecko.mako.rs:165
(Diff revision 4)
> +    /// Gets a reference to the rule node. Panic if the element does not have a
> +    /// rule node.

What Element? This should probably say Panics if the ComputedValues does not have a rule node.

::: servo/components/style/properties/properties.mako.rs:1886
(Diff revision 4)
> +    /// Gets a reference to the rule node. Panic if the element does not have a
> +    /// rule node.

Similar.

::: servo/components/style/properties/properties.mako.rs:2351
(Diff revision 4)
>      }
>  
>      /// Inherits style from the parent element, accounting for the default
>      /// computed values that need to be provided as well.
>      pub fn for_inheritance(parent: &'a ComputedValues, default: &'a ComputedValues) -> Self {
> -        Self::new(parent.custom_properties(),
> +        Self::new(parent.rules.clone(),

As emilio mentioned, this should pass None.

::: servo/components/style/properties/properties.mako.rs:2637
(Diff revision 4)
>      let custom_properties =
>          ::custom_properties::finish_cascade(
>              custom_properties, &inherited_custom_properties);
>  
>      let builder = if !flags.contains(INHERIT_ALL) {
> -        StyleBuilder::new(custom_properties,
> +        StyleBuilder::new(Some(rules.clone()),

Let's make sure we have a bug on file to eliminate this clone().

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/151492/#review156618

::: servo/components/style/matching.rs:301
(Diff revision 4)
>  
>      fn cascade_with_rules(&self,
>                            shared_context: &SharedStyleContext,
>                            font_metrics_provider: &FontMetricsProvider,
>                            rule_node: &StrongRuleNode,
> -                          primary_style: &ComputedStyle,
> +                          primary_style: Option<&Arc<ComputedValues>>,

Please document when this can or cannot be `None`.

::: servo/components/style/properties/properties.mako.rs:2637
(Diff revision 4)
>      let custom_properties =
>          ::custom_properties::finish_cascade(
>              custom_properties, &inherited_custom_properties);
>  
>      let builder = if !flags.contains(INHERIT_ALL) {
> -        StyleBuilder::new(custom_properties,
> +        StyleBuilder::new(Some(rules.clone()),

So, if we're not keeping the root as the only way of saying "no rules matched", the has_rules function is kind of a lie, because you can arrive here without any matching rule, and has_rules will happily return true.

Should we check the rule node too?

::: servo/ports/geckolib/glue.rs:2605
(Diff revision 4)
>      // In the common case we already have the style. Check that before setting
>      // up all the computation machinery. (Don't use it when we're getting
>      // default styles, though.)
>      if rule_inclusion == RuleInclusion::All {
> -        if let Some(result) = element.mutate_data()
> -                                     .and_then(|d| d.get_styles().map(&finish)) {
> +        let styles = element.mutate_data().and_then(|d| {
> +            match d.has_styles() {

nit: I usually use `if` instead of matching with `Some` and `None`.

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/151492/#review156618

> nit: I usually use `if` instead of matching with `Some` and `None`.

Err, matching with `true` and `false`.

Comment 41

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/151492/#review156562

> Please file a followup for fixing this up?

I filed bug 1375525.  I'll mention it in a comment as well.

> Nit: all this "values" renaming really isn't necessary.

Good catch, I've changed these back to "style".

> Seems like we do this incantation a lot. How about making a helper cascade_inputs() method on context?

Okay, I added helpers and was able to use them in most places.  In some spots, I had to keep the long form because the helpers borrow all of context and some code paths need to simultaneously use other bits of context with different borrowing.

> What Element? This should probably say Panics if the ComputedValues does not have a rule node.

Okay, updated.

> As emilio mentioned, this should pass None.

Okay, fixed.

> Let's make sure we have a bug on file to eliminate this clone().

I mentioned this in same bug filed for the animation issue, and I'll add a comment here.

Comment 42

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/151492/#review156618

> Please document when this can or cannot be `None`.

Good idea, added docs for this.

> Err, matching with `true` and `false`.

Yes, that's probably more natural!  Fixed.

Comment 43

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #39)
> ::: servo/components/style/properties/properties.mako.rs:2637
> (Diff revision 4)
> >      let custom_properties =
> >          ::custom_properties::finish_cascade(
> >              custom_properties, &inherited_custom_properties);
> >  
> >      let builder = if !flags.contains(INHERIT_ALL) {
> > -        StyleBuilder::new(custom_properties,
> > +        StyleBuilder::new(Some(rules.clone()),
> 
> So, if we're not keeping the root as the only way of saying "no rules
> matched", the has_rules function is kind of a lie, because you can arrive
> here without any matching rule, and has_rules will happily return true.
> 
> Should we check the rule node too?

I don't quite follow your concern here, can you elaborate?  Or let's discuss on IRC if that's simpler.

Comment 44

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #43)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #39)
> > ::: servo/components/style/properties/properties.mako.rs:2637
> > (Diff revision 4)
> > >      let custom_properties =
> > >          ::custom_properties::finish_cascade(
> > >              custom_properties, &inherited_custom_properties);
> > >  
> > >      let builder = if !flags.contains(INHERIT_ALL) {
> > > -        StyleBuilder::new(custom_properties,
> > > +        StyleBuilder::new(Some(rules.clone()),
> > 
> > So, if we're not keeping the root as the only way of saying "no rules
> > matched", the has_rules function is kind of a lie, because you can arrive
> > here without any matching rule, and has_rules will happily return true.
> > 
> > Should we check the rule node too?
> 
> I don't quite follow your concern here, can you elaborate?  Or let's discuss
> on IRC if that's simpler.

We dicsussed this on IRC.  The issue was about whether `has_rules` is still correct for the visited case.  It is because we only have Some(rules) that type of matching was done now or in the past.

I added some comments to clarify this.

Comment 45

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 46

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://github.com/servo/servo/pull/17481

Comment 47

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://hg.mozilla.org/integration/autoland/rev/f620402d752e