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

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

MozReview-Commit-ID: 9hY1rP5KXGn

MozReview-Commit-ID: IIDxBJ8mqvJ

MozReview-Commit-ID: BdYkXxYvUQ3

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 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 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?

(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.