Right now, if we have a bunch of table cells that all share ComputedValues, their anonymous block boxes still won't share anything, though they could do so. This can use a noticeable amount of memory; see data in bug 1367862 comment 1 and bug 1367862 comment 2.

Moved the anonymous box stuff into bug 1368291, since it's the same as text. Lazy pseudos will require checking the rule node pointer, which we'll have soon.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1) > Moved the anonymous box stuff into bug 1368291 Err, the anonymous box stuff is bug 1368290.

Emilio is fixing bug 1368290, and this should be trivial to do on top of that. Assigning to him.

I ended up writing patches for this today while measuring bug 1367862 and bug 1369902. Ended up being much trickier than I thought, but that's how it goes.

So the patches seem to halve the number of pseudo element contexts on obama (bug 1367862 comment 21), but significantly hamper text sharing on github - possibly because it means that we can no longer share styles for text nodes that inherit lazy pseudo elements? Will need to investigate.

(The answer, in that case, may be to have two separate linked lists).

Just took a quick look... Why is it needed to check the rule node? Is it due to the pseudos with |CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE|? If so, I suspect that sharing isn't super-important for these (and that we mostly care about moz-list-{bullet,number}), and that avoiding that would make the patch much simpler, leaving all the complexity on the Gecko side of things, what do you think?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8) > Just took a quick look... Why is it needed to check the rule node? Is it due > to the pseudos with |CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE|? > > If so, I suspect that sharing isn't super-important for these (and that we > mostly care about moz-list-{bullet,number}), and that avoiding that would > make the patch much simpler, leaving all the complexity on the Gecko side of > things, what do you think? The argument being that, if the parent computedvalues is the same, we must either have the same originating element, or at least a similar-enough originating element that all the same lazy pseudos would match? That's a nice idea! Giving it a spin now.

With this patch, I get about 25% the OTHER PSEUDO COUNT on obama-noscript (716 instead of 2668), and a smaller (but still significant) win on the github testcase in bug 1369902 (2414 vs 2669).

MozReview-Commit-ID: 9u8FzDXFZcX

Comment on attachment 8894240 [details] [diff] [review] Style sharing for lazy pseudos. v1 Review of attachment 8894240 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I'm not sure if what's going on on the ::before/::after case is intentional. Seems to me like it isn't? ::: layout/style/ServoStyleContext.h @@ +36,5 @@ > > ServoStyleContext* GetCachedInheritingAnonBoxStyle(nsIAtom* aAnonBox) const; > > void SetCachedInheritedAnonBoxStyle(nsIAtom* aAnonBox, > + ServoStyleContext* aStyle) nit: This change seems mostly cosmetic (and I think everyone mostly agreed at [1] that passing non copy-constructible objects by reference instead of by pointers was fine). Fine to keep it the same as |SetCachedLazyPseudoStyle| for consistency, but I'd rather change the latter to take a reference... :P [1]: https://groups.google.com/d/msg/mozilla.dev.platform/_jfnwDvcvN4/Z4-m-03SCAAJ @@ +59,5 @@ > + ServoStyleContext* GetCachedLazyPseudoStyle(CSSPseudoElementType aPseudo) const; > + > + void SetCachedLazyPseudoStyle(ServoStyleContext* aStyle) > + { > + MOZ_ASSERT(aStyle->GetPseudoType() != CSSPseudoElementType::NotPseudo && nit: This can be more compact and fit on one line with |aStyle->GetPseudo() && !aStyle->IsAnonBox()|, but not a big deal. @@ +64,5 @@ > + !aStyle->IsAnonBox()); > + MOZ_ASSERT(!GetCachedLazyPseudoStyle(aStyle->GetPseudoType())); > + MOZ_ASSERT(!aStyle->mNextLazyPseudoStyle); > + > + if (nsCSSPseudoElements::PseudoElementSupportsUserActionState(aStyle->GetPseudoType())) { This probably deserves a comment. @@ +70,5 @@ > + } > + > + // NOTE(emilio): Since we use it to cache lazy pseudo styles in a linked > + // list, we can't use that cache if the style we're inheriting from is a > + // lazy pseudo style itself itself, since otherwise our parent would nit: itself itself @@ +76,5 @@ > + // > + // See the documentation of mNextLazyPseudoStyle. > + // > + // NB: We don't have a good way to differentiate lazy/eager pseudo-elements > + // on the Gecko side, so we just do a more conservative check. Well, can pseudos have other pseudos at all? I think they can't, so we could just try to assert (!IsPseudoElement()) instead? ::: layout/style/ServoStyleSet.cpp @@ +546,5 @@ > aPseudoTag, > mRawSet.get()).Consume(); > MOZ_ASSERT(style); > if (aParentContext) { > + aParentContext->SetCachedInheritedAnonBoxStyle(aPseudoTag, style); Yeah, these lines are mostly noise on this patch... but anyway, fine :P @@ +819,5 @@ > + if (!computedValues) { > + return nullptr; > + } > + > + aParentContext->SetCachedLazyPseudoStyle(computedValues); Should this move after the ::before and ::after check? (Should we even use it for those? Over all since they're not lazy pseudos themselves...) If you want to make it more generic and apply to eager pseudos as well, should we just call this {Set,Get}CachedPseudoElementStyle? But that seems somewhat pointless, at least for ::before and ::after, and at least for now, if we don't fix bug 1385154...

MozReview-Commit-ID: 9u8FzDXFZcX

(In reply to Emilio Cobos Álvarez [:emilio] from comment #13) > Comment on attachment 8894240 [details] [diff] [review] > Style sharing for lazy pseudos. v1 > > Review of attachment 8894240 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, but I'm not sure if what's going on on the ::before/::after case > is intentional. Seems to me like it isn't? > > ::: layout/style/ServoStyleContext.h > @@ +36,5 @@ > > > > ServoStyleContext* GetCachedInheritingAnonBoxStyle(nsIAtom* aAnonBox) const; > > > > void SetCachedInheritedAnonBoxStyle(nsIAtom* aAnonBox, > > + ServoStyleContext* aStyle) > > nit: This change seems mostly cosmetic (and I think everyone mostly agreed > at [1] that passing non copy-constructible objects by reference instead of > by pointers was fine). I think dbaron's point about per-type consistency is a fair one, and we use * for StyleContext types everywhere. > > Fine to keep it the same as |SetCachedLazyPseudoStyle| for consistency, but > I'd rather change the latter to take a reference... :P > > [1]: > https://groups.google.com/d/msg/mozilla.dev.platform/_jfnwDvcvN4/Z4-m-03SCAAJ > > @@ +59,5 @@ > > + ServoStyleContext* GetCachedLazyPseudoStyle(CSSPseudoElementType aPseudo) const; > > + > > + void SetCachedLazyPseudoStyle(ServoStyleContext* aStyle) > > + { > > + MOZ_ASSERT(aStyle->GetPseudoType() != CSSPseudoElementType::NotPseudo && > > nit: This can be more compact and fit on one line with |aStyle->GetPseudo() > && !aStyle->IsAnonBox()|, but not a big deal. Nice. > > @@ +64,5 @@ > > + !aStyle->IsAnonBox()); > > + MOZ_ASSERT(!GetCachedLazyPseudoStyle(aStyle->GetPseudoType())); > > + MOZ_ASSERT(!aStyle->mNextLazyPseudoStyle); > > + > > + if (nsCSSPseudoElements::PseudoElementSupportsUserActionState(aStyle->GetPseudoType())) { > > This probably deserves a comment. Good point. Done. > > @@ +70,5 @@ > > + } > > + > > + // NOTE(emilio): Since we use it to cache lazy pseudo styles in a linked > > + // list, we can't use that cache if the style we're inheriting from is a > > + // lazy pseudo style itself itself, since otherwise our parent would > > nit: itself itself Fixed. > > @@ +76,5 @@ > > + // > > + // See the documentation of mNextLazyPseudoStyle. > > + // > > + // NB: We don't have a good way to differentiate lazy/eager pseudo-elements > > + // on the Gecko side, so we just do a more conservative check. > > Well, can pseudos have other pseudos at all? I think they can't, so we could > just try to assert (!IsPseudoElement()) instead? Hm, I _think_ you're right, given the new model for inheritance. I'll assert and see what try says. > > ::: layout/style/ServoStyleSet.cpp > @@ +546,5 @@ > > aPseudoTag, > > mRawSet.get()).Consume(); > > MOZ_ASSERT(style); > > if (aParentContext) { > > + aParentContext->SetCachedInheritedAnonBoxStyle(aPseudoTag, style); > > Yeah, these lines are mostly noise on this patch... but anyway, fine :P :-) > > @@ +819,5 @@ > > + if (!computedValues) { > > + return nullptr; > > + } > > + > > + aParentContext->SetCachedLazyPseudoStyle(computedValues); > > Should this move after the ::before and ::after check? (Should we even use > it for those? Over all since they're not lazy pseudos themselves...) Ah, good point! I'll exclude them. > > If you want to make it more generic and apply to eager pseudos as well, > should we just call this {Set,Get}CachedPseudoElementStyle? But that seems > somewhat pointless, at least for ::before and ::after, and at least for now, > if we don't fix bug 1385154... Yeah, we already get eager pseudo sharing via the style sharing cache.

Comment on attachment 8895066 [details] [diff] [review] Style sharing for lazy pseudos. v2 Review of attachment 8895066 [details] [diff] [review]: ----------------------------------------------------------------- FWIW I don't agree with the "pointer or references per type" argument. I find it both hard to reason about if you don't know the codebase, and kind of a circular argument (you can't use references because we don't use references for this type because we never used references for this type). But whatever, r=me on this patch with the nits. ::: layout/style/ServoStyleContext.cpp @@ +64,5 @@ > + } > + > + // See the reasoning in SetCachedLazyPseudoStyle to understand why we > + // can't use the cache in this case. > + if (IsPseudoElement()) { Just say that pseudo-elements don't have other pseudo-elements. ::: layout/style/ServoStyleContext.h @@ +76,5 @@ > + if (nsCSSPseudoElements::PseudoElementSupportsUserActionState(aStyle->GetPseudoType())) { > + return; > + } > + > + MOZ_ASSERT(!IsPseudoElement()); nit: Comment that pseudos can't have other pseudos? And maybe move this to the top with the other assertions? ::: layout/style/nsCSSPseudoElements.h @@ +15,5 @@ > // Is this pseudo-element a CSS2 pseudo-element that can be specified > // with the single colon syntax (in addition to the double-colon syntax, > // which can be used for all pseudo-elements)? > +// > +// Note: We also rely on this for IsEagerlyCascadedInServo. hmm... I guess it's unlikely to change, but adding an extra flag would be cheap enough.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #17) > Comment on attachment 8895066 [details] [diff] [review] > Style sharing for lazy pseudos. v2 > > Review of attachment 8895066 [details] [diff] [review]: > ----------------------------------------------------------------- > > FWIW I don't agree with the "pointer or references per type" argument. > > I find it both hard to reason about if you don't know the codebase, and kind > of a circular argument (you can't use references because we don't use > references for this type because we never used references for this type). Such is the nature of style consistency arguments. Let's talk about it over a drink in Cancun. ;-) > > But whatever, r=me on this patch with the nits. > > ::: layout/style/ServoStyleContext.cpp > @@ +64,5 @@ > > + } > > + > > + // See the reasoning in SetCachedLazyPseudoStyle to understand why we > > + // can't use the cache in this case. > > + if (IsPseudoElement()) { > > Just say that pseudo-elements don't have other pseudo-elements. > > ::: layout/style/ServoStyleContext.h > @@ +76,5 @@ > > + if (nsCSSPseudoElements::PseudoElementSupportsUserActionState(aStyle->GetPseudoType())) { > > + return; > > + } > > + > > + MOZ_ASSERT(!IsPseudoElement()); > > nit: Comment that pseudos can't have other pseudos? And maybe move this to > the top with the other assertions? I had to fix these anyway because it turns out that pseudos _can_ have pseudos, but only eager ones (i.e. number lists inside before/after). I've added an IsLazilyCascadedPseudoElement() in ServoStyleContext. Thanks for the reviews! https://treeherder.mozilla.org/#/jobs?repo=try&revision=428f62c405aa84fe332223c4b53d5ff9e73e8021

Pushed by bholley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5cf7180bd57a Style sharing for lazy pseudos. r=emilio