1371049 - stylo: Make EagerPseudoStyles use an Arc, not a Box

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

That way when styles with pseudo-elements are shared (per bug 1329361) we just refcount instead of copying.

Comment 1

[Bobby Holley (:bholley)]

Filed this for the piranhas: https://github.com/servo/servo/issues/17300

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop thrashing when adding and removing pseudos. v1

MozReview-Commit-ID: 9hY1rP5KXGn

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use a newtype within EagerPseudoValues. v1

MozReview-Commit-ID: IIDxBJ8mqvJ

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use an Arc for eager pseudo styles. v1

MozReview-Commit-ID: BdYkXxYvUQ3

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

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

Comment on attachment 8880951 [details] [diff] [review]
Part 3 - Use an Arc for eager pseudo styles. v1

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

Thanks, this looks good to me! :)

Comment 7

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

Comment on attachment 8880950 [details] [diff] [review]
Part 2 - Use a newtype within EagerPseudoValues. v1

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

Thanks! :) Please change the commit message to say `EagerPseudoStyles` instead of `Values`.

Comment 8

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

Comment on attachment 8880949 [details] [diff] [review]
Part 1 - Stop thrashing when adding and removing pseudos. v1

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

Is this thrashing really a common thing?  You'd have to remove the last pseudo and add another different one during the same matching run, right?

::: servo/components/style/data.rs
@@ +167,5 @@
> +    /// Checks whether the EagerPseudoStyles is allocated but empty (due to take()
> +    /// calls), and if so, drops the allocation. We do this as a separate step from
> +    /// take() so that we don't thrash when adding/removing pseudos.
> +    pub fn compact(&mut self) {
> +        if self.0.is_some() && self.0.as_ref().unwrap().iter().all(|x| x.is_none()) {

With this delayed compaction, there's a window where `is_empty` gives the wrong answer until we compact.  Not a _big_ deal, but also a little unfortunate...

If this thrashing is an issue, what if we instead build a list of the pseudos to remove, and then do all the take()s back to back?

::: servo/components/style/matching.rs
@@ +1535,5 @@
>          }
> +
> +        // If we previously had pseudos but no longer do, this is the time to
> +        // drop the now-empty pseudo array.
> +        data.styles.pseudos.compact();

The set of active pseudos is up to date by the end of `match_pseudos`.  Perhaps this makes more sense inside there, so it's closer to where the add / remove bits happen?

Comment 9

[Bobby Holley (:bholley)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> Comment on attachment 8880949 [details] [diff] [review]
> Part 1 - Stop thrashing when adding and removing pseudos. v1
> 
> Review of attachment 8880949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this thrashing really a common thing?

No idea - this wasn't profile-driven, just a drive-by fix that seemed worth doing.

>  You'd have to remove the last
> pseudo and add another different one during the same matching run, right?

Yeah. I could imagine a bunch of children flipping style from ::before to ::after given some style change in the parent. But probably not super likely.

> ::: servo/components/style/data.rs
> @@ +167,5 @@
> > +    /// Checks whether the EagerPseudoStyles is allocated but empty (due to take()
> > +    /// calls), and if so, drops the allocation. We do this as a separate step from
> > +    /// take() so that we don't thrash when adding/removing pseudos.
> > +    pub fn compact(&mut self) {
> > +        if self.0.is_some() && self.0.as_ref().unwrap().iter().all(|x| x.is_none()) {
> 
> With this delayed compaction, there's a window where `is_empty` gives the
> wrong answer until we compact.  Not a _big_ deal, but also a little
> unfortunate...

Yeah, fair. Probably not worth spending any time on, I'll just nix this patch.

Comment 10

[Bobby Holley (:bholley)]

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

Comment 11

[Bobby Holley (:bholley)]

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