1368291 - stylo: Enable ComputedValues sharing for lazy pseudos

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Bobby Holley (:bholley)]

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Bobby Holley (:bholley)]

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

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Bobby Holley (:bholley)]

Attachment: Style sharing for lazy pseudos. v1

MozReview-Commit-ID: 9u8FzDXFZcX

Comment 13

[Emilio Cobos Álvarez (:emilio)]

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

Comment 14

[Bobby Holley (:bholley)]

Attachment: Style sharing for lazy pseudos. v2

MozReview-Commit-ID: 9u8FzDXFZcX

Comment 15

[Bobby Holley (:bholley)]

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

[Bobby Holley (:bholley)]

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

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 18

[Bobby Holley (:bholley)]

(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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cf7180bd57a
Style sharing for lazy pseudos. r=emilio

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5cf7180bd57a