1385154 - stylo: Consider rethinking how EagerPseudoStyles for ::before and ::after work.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez [:emilio]]

There's probably no need to do this right now, but I believe we should really think a bit harder about all the EagerPseudoStyles stuff.

Here's something that makes me nervous. Using a single rule like:

  *, *::before, *::after { box-sizing: border-box }

(which people do use, and also I think it's pretty common in modern pages)

Will make stylo allocate a whole lot of extra memory for every single element.

There are other multiple reasons that make EagerPseudoStyles not great.

 * It's only semi-useful for ::first-letter, due to the weird inheritance.
 * The benefit of getting it done in parallel the traversal is only useful for frame construction. We're restyling them unnecessarily afterwards if we don't end up constructing a pseudo. We'd still need to query them during MatchAndCascade restyles though, if there's no pre-existing pseudo, to trigger the proper reframe if needed...

One semi-obvious solution would be something like only store ::before and ::after when there's an actual non-empty "content" property.

This is maybe the most reasonable way, but then we need to fix a bunch of stuff that depends on "them being present == they matched" for getComputedStyle and similar callers. That's probably not too hard.

Other one would be to just remove them, of course, which would remove complexity from a few places, but also potentially add it to others...

Comment 1

[Emilio Cobos Álvarez [:emilio]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> One semi-obvious solution would be something like only store ::before and
> ::after when there's an actual non-empty "content" property.
> 
> This is maybe the most reasonable way, but then we need to fix a bunch of
> stuff that depends on "them being present == they matched" for
> getComputedStyle and similar callers. That's probably not too hard.

There's a non-trivial part of this, which would be how it interacts with our restyling optimizations to avoid selector-matching.

In particular, I'd expect that a naive implementation of this would fail to handle something like:

<style>
div::before { content: inherit }
</style>
<div id="target"></div>
<script>
document.body.offsetTop;
target.style.content = '"foo"';
</script>

So we'd really need something like "was there a declaration for the content property"? Instead of "is content not none"? Or maybe we can just explicitly check for "content: inherit" too.

I can't think of other similar nasty cases of doing it that way, thought there could be.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

I talked with Cam and I think we're probably underestimating the impact of this.

I'll take a look, I think I have an idea.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

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

If it works, it wasn't really that hard as I expected :)

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: style: Only resolve applicable ::before / ::after pseudo styles during the traversal.

Signed-off-by: Emilio Cobos Álvarez <[e-mail]>

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

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1385154: Test-cases for the nasty edge cases.

Chromium and Epiphany fail these, whoops...

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

Comment 6

[Cameron McCormack (:heycam)]

I'll take a look at these tomorrow morning, but for now, do you have some numbers on how this affects memory usage?

Comment 7

[Emilio Cobos Álvarez [:emilio]]

I don't. I'll get some later today.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

I opened https://github.com/servo/servo/pull/18215 with the servo bits (the first commit).

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Also filed https://bugs.chromium.org/p/chromium/issues/detail?id=758520, and https://bugs.webkit.org/show_bug.cgi?id=18587 already existed in WebKit's bug tracker.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

So, I took some numbers, and it seems a slight win in the pages I tried, but I couldn't find pages that used rules like the one I posted in the bug description (though it's a thing, see[1]).

But, over all... This literally can't be a memory loss, and the worst case for this would be pretty evident, and I believe it's not ultra-hard to find it on the web, so...

[1]: https://www.paulirish.com/2012/box-sizing-border-box-ftw/

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8900656 [details]
style: Only resolve applicable ::before / ::after pseudo styles during the traversal.

https://reviewboard.mozilla.org/r/172052/#review177788

Looks good, thanks!

::: servo/components/style/traversal.rs:445
(Diff revision 1)
>  
>      for ancestor in ancestors_requiring_style_resolution.iter().rev() {
>          context.thread_local.bloom_filter.assert_complete(*ancestor);
>  
>          let primary_style =
> -            StyleResolverForElement::new(*ancestor, context, rule_inclusion)
> +            StyleResolverForElement::new(*ancestor, context, rule_inclusion, PseudoElementResolution::None)

Is this because we can't have an ancestor that is a pseudo?  (Maybe comment?)

::: servo/ports/geckolib/glue.rs:3001
(Diff revision 1)
> +        rule_inclusion,
> +        ignore_existing_styles,
> +        pseudo.as_ref()
> +    );
> +
> +    finish(&styles, false)

`/* is_probe = */ false` just to be clear.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8900657 [details]
Bug 1385154: Test-cases for the nasty edge cases.

https://reviewboard.mozilla.org/r/172054/#review177790

Nice tests. :-)

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8900656 [details]
style: Only resolve applicable ::before / ::after pseudo styles during the traversal.

https://reviewboard.mozilla.org/r/172052/#review177796

::: servo/components/style/traversal.rs:445
(Diff revision 1)
>  
>      for ancestor in ancestors_requiring_style_resolution.iter().rev() {
>          context.thread_local.bloom_filter.assert_complete(*ancestor);
>  
>          let primary_style =
> -            StyleResolverForElement::new(*ancestor, context, rule_inclusion)
> +            StyleResolverForElement::new(*ancestor, context, rule_inclusion, PseudoElementResolution::None)

Well we can have an ancestor that is a pseudo, but those have no eager pseudo-elements anyway. 

Actually, I thought we computed all the `EagerPseudoStyles` here, but since we don't I can just remove `PseudoElementResolution::None` entirely. I was misremembering from my patches from bug 1381071.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8900656 [details]
style: Only resolve applicable ::before / ::after pseudo styles during the traversal.

https://reviewboard.mozilla.org/r/172052/#review177788

> `/* is_probe = */ false` just to be clear.

Sure :)

Comment 15

[Bobby Holley (:bholley)]

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

https://hg.mozilla.org/integration/autoland/rev/7a6ee708390920125291fd0790fce9e8d1c25068

Comment 16

[Emilio Cobos Álvarez [:emilio]]

I still want to land the test. I'm waiting to see if there's a way to get the spec issue resolved, otherwise I'll land in layout/reftests and upstream when possible.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> I still want to land the test. I'm waiting to see if there's a way to get
> the spec issue resolved, otherwise I'll land in layout/reftests and upstream
> when possible.

Ok. Do you plan to do that within the next day or two? If not, I'd prefer to file a followup, non-P2 issue about landing the test, to get this off our radar.

Comment 18

[Emilio Cobos Álvarez [:emilio]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #17)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> > I still want to land the test. I'm waiting to see if there's a way to get
> > the spec issue resolved, otherwise I'll land in layout/reftests and upstream
> > when possible.
> 
> Ok. Do you plan to do that within the next day or two? If not, I'd prefer to
> file a followup, non-P2 issue about landing the test, to get this off our
> radar.

Yeah, I'll land tomorrow if there's no update from the CSS working group.

Comment 19

[Cameron McCormack (:heycam)]

Emilio, would it be incorrect to do this?  Or would it cause us to keep re-resolving ::before/::after style?

diff --git a/servo/components/style/style_resolver.rs b/servo/components/style/style_resolver.rs
index e2e9624..a9a684c 100644
--- a/servo/components/style/style_resolver.rs
+++ b/servo/components/style/style_resolver.rs
@@ -195,7 +195,9 @@ where
                     layout_parent_style_for_pseudo
                 );
                 if let Some(style) = pseudo_style {
-                    pseudo_styles.set(&pseudo, style);
+                    if !eager_pseudo_is_definitely_not_generated(&pseudo, &style) {
+                        pseudo_styles.set(&pseudo, style);
+                    }
                 }
             })
         }

I'm finding that even after this bug's patch, we have a lot of ineffective ::before styles stored in the tree for the HTML spec.  Just dropping the styles if they turn out to be ineffective, after computing them, lets us save another 3 MB of style structs.

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Err, yes, that should've been in the initial patch, I missed that and only added the callsite at the cascade_* method, whoops.

r=me on that diff, with a pseudo_resolution check.

Comment 21

[Emilio Cobos Álvarez [:emilio]]

I just submitted it in https://github.com/servo/servo/pull/18261

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8900657 [details]
Bug 1385154: Test-cases for the nasty edge cases.

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/909cff254692
Test-cases for the nasty edge cases. r=heycam

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/909cff254692