1409672 - stylo: consider handling :-moz-window-inactive changes using invalidations

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

In bug 1390694 (and in the old Gecko style system), whenever a document state flag changes, and we have some rule in style sheets that uses the correspond pseudo-class, we restyle the entire document.  It would be nice if we could use the invalidation machinery to limit the elements that need restyling.

Bug 1390694 comment 13 explains some difficulties in doing that.  The right way to do that, if we thought it was worth it, is probably to add a field to MatchingContext that indicates some document state flags to always consider as matched.

Comment 1

[Chris Peterson [:cpeterson]]

Boosting priority from P4 to P2 because Emilio says this bug is related to or a dupe of Xidorn's Stylo-chrome perf bug 1428978.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

So there are two approaches:

First, we implement something new to handle invalidation of document state change.

My idea for doing so is
* add a new field to InvalidationMap which stores a list of selectors that mention pseudo-classes which depend on document state, then
* implement a new InvalidationProcessor which generates no invalidation, but just handles the invalidating action, and
* add a new method to Invalidator coded similar to invalidate everything including the root, based on the selector list from InvalidationMap

This way we should be able to handle :-moz-window-inactive, which is the main problem affecting our performance.


Second approach, we make :-moz-window-inactive an element state of the root element, so that when it changes, instead of posting a RESTYLE_SUBTREE like what we are doing currently, we simply reuse the existing state-based invalidation code to do the work.

To have it work, we would have to rewrite all selectors using :-moz-window-inactive. It is probably not a big deal inside our codebase. There are only 45 appearance of that. We can simply move :-moz-window-inactive to either the first compound selector or ancestor of the first, depending on whether the first element would match the root.

Currently, many :-moz-window-inactive are attached on the last component selector, which I suspect can affect Gecko's performance on handling them? If so, this rewrite would end up making Gecko slower before we switch to stylo-chrome, which doesn't sound very good either unless we can finish the change very fast. But I'm not sure.

Another disadvantage of doing so is that we are going to waste one element state bit, but that's probably not too bad since there are still 11 bits available...


It seems to me that, adding a general invalidation machinery for document state change may not be very useful at the moment (the only other document state selector :-moz-locale-dir() rarely changes), so it isn't worth wasting memory and time for handling any document-state-dependent pseudo-class other than :-moz-window-inactive. (Actually I'm wondering whether we can remove the document state of :-moz-locale-dir() at all.) So even if we go the first way, we would implement only for this specific pseudo-class.

Given that I incline to do the second approach, so that we don't add complexity to the invalidation code for this specific thing. We can probably have a separate bug for rewriting the existing selectors and see whether that would pose any issue to Gecko.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

So, after trying to do the second approach (making :-moz-window-inactive an element state rather than document state), it turns out not to be as easy as we previously thought.

The main issue is that there are rules in XBL stylesheet which depends on :-moz-window-inactive, but we don't iterate XBL stylists from descendants when we do invalidation, which means we would not correctly update style for elements inside XBL which depends on this state.

It isn't impossible to make it work. We can change the relevant code to have it iterate all XBL stylists in the document, but iterating all of them for each state change could have a rather larger performance impact (although we don't know how much it would be at the moment).

emilio thinks it may be better at this point to go the first way to implement invalidation for document state change, and he'd like to try hacking on it, so assigning him instead.

emilio: if you have anything higher priority to do, you can assign it back to me anytime.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #4)
> The main issue is that there are rules in XBL stylesheet which depends on
> :-moz-window-inactive, but we don't iterate XBL stylists from descendants
> when we do invalidation, which means we would not correctly update style for
> elements inside XBL which depends on this state.

What XBL sheet(s) use these rules? Maybe we can move the rule into a document sheet.

Comment 6

[Brian Grinstead [:bgrins]]

Also: are we OK with breaking / removing this feature from web content? According to: https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-window-inactive: "Prior to the addition of this pseudo-class, giving different styles to background windows was achieved by setting an attribute (active="true") on the top-level XUL chrome window. This attribute is no longer used.".

If we don't care about maintaining compat (which approach 1 here doesn't do either), then I wonder if we could go back to something like that - where an 'inactive' attribute is set on all root nodes that aren't active (or vice versa).

According to https://bugzilla.mozilla.org/show_bug.cgi?id=508482#c0, the :-moz-window-inactive selector was added because the previous approach was:

  1. It's ugly.
  2. Selectors get really long if you want to avoid descendant selectors.
  3. You can't use the activation status in scoped XBL stylesheets because
     those don't get notified about dynamic changes to the window's attributes

I think (1) and (2) aren't any different from what we'll have anyway if we end up going with an approach like Bug 1429293 where we would be using the :root selector. That leaves the XBL issue - but I'd also consider preventing XBL from using it and migrating any existing styles elsewhere. We are going to be moving XBL sheets into UA sheets anyway (which will be possible once Bug 1420229 is resolved).

Comment 7

[Markus Stange [:mstange]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Also: are we OK with breaking / removing this feature from web content?

That's fine with me. No other browser supports it; WebKit only supports it for custom scrollbar pseudo elements.

I have no opinion on the other questions. If using an attribute on the root element doesn't come with a large performance hit, then even that would be fine, I think.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> (In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #4)
> > The main issue is that there are rules in XBL stylesheet which depends on
> > :-moz-window-inactive, but we don't iterate XBL stylists from descendants
> > when we do invalidation, which means we would not correctly update style for
> > elements inside XBL which depends on this state.
> 
> What XBL sheet(s) use these rules? Maybe we can move the rule into a
> document sheet.

So... the affected selectors are platform-dependent bits in toolkit/themes, they are all in the change of bug 1429293, so we cannot move them to document sheet, but we can move them to UA sheets (maybe some to html.css, some to xul.css).

I didn't consider that as an option because I didn't know that we have had a plan to move them anyway.

But as you mentioned in comment 6, what we do in bug 1429293 is ugly, and add lots of descendant selectors. It's something inevitable for option 2. (Actually the change to make :-moz-window-inactive an element state has some dirtiness in itself, which isn't too bad, though.)

That option was raised because we thought it might be an easier route to go down than adding code to invalidator. However, after reading emilio's patch in servo/servo#19747 which implements the option 1, I feel that the code complexity is not as bad as I thought before. Actually it looks quite clean.

Given all these, we can probably try landing option 1 unless we find there is any other issue we didn't notice which can affect its feasibility.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1409672: Hook in the invalidator stuff.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

So this is a big reduction in the PGO regressions, see last stylo-chrome comparison:

   https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=46f657fa7299c1f09323bf8b9e489d2f21267a1a&newProject=try&newRevision=783915f36ea74b64daf5794b4b2d6f1f326aed96&framework=1&showOnlyConfident=1

vs.

  https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=aea8893308877a9139af703db9d5abcc822dddd5&newProject=try&newRevision=0237ac17044af8e2c8d0c3a9c243e0ac94b9b991&framework=1&showOnlyConfident=1

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, that's unfair. The first comparison was taken in Nov 2017, which is almost the first talos push before we optimize anything... The latest one should be https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b74386f34322&newProject=try&newRevision=0b8e59e7a388&framework=1

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #11)
> Hmmm, that's unfair. The first comparison was taken in Nov 2017, which is
> almost the first talos push before we optimize anything... The latest one
> should be
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=b74386f34322&newProject=try&newR
> evision=0b8e59e7a388&framework=1

Blerg, I sincerely not sure what happens to me and talos links today :(. Thanks for the catch. Anyway seems an improvement.

Also, I'll push a stylo-chrome w/ vs. without patch.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8942905 [details]
Bug 1409672: Hook in the invalidator stuff.

https://reviewboard.mozilla.org/r/213170/#review219078

r=me with the nits addressed.

::: layout/style/ServoStyleSet.cpp:253
(Diff revision 1)
> +  // User sheets.
> +  AutoTArray<RawServoStyleSetBorrowed, 20> styleSets;
> +  styleSets.AppendElement(mRawSet.get());
> +  // FIXME(emilio): When bug 1425759 is fixed we need to enumerate ShadowRoots
> +  // too.
> +  mDocument->BindingManager()->EnumerateBoundContentBindings([&](nsXBLBinding* aBinding) {

This line seems to be too long. Consider putting the lambda to the second line.

::: servo/components/style/invalidation/element/invalidator.rs:537
(Diff revision 1)
>              return self.invalidate_dom_descendants_of(node, invalidations);
>          }
>  
>          let mut any_descendant = false;
>  
> +        // NOTE(emilio): This should not not be needed for Shadow DOM for normal

Duplicate "not" here, and the last "for" is "or" I believe?

::: servo/ports/geckolib/glue.rs:4941
(Diff revision 1)
> +    states_changed: u64,
> +) {
> +    use style::invalidation::element::document_state::DocumentStateInvalidationProcessor;
> +    use style::invalidation::element::invalidator::TreeStyleInvalidator;
> +
> +    let mut borrows = SmallVec::<[_; 20]>::with_capacity((*raw_style_sets).len());

I don't really see it makes much sense to use `SmallVec` here really... You have the capacity, so there would be at most one allocation. (When we don't have the capacity, this could be beneficial because that avoids many allocations when we expand the array. But a single allocation in a not so hot function doesn't seem to be a problem.)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b54c60961def
Hook in the invalidator stuff. r=xidorn

Comment 15

[Emilio Cobos Álvarez (:emilio)]

I backed this patch out in https://hg.mozilla.org/integration/autoland/rev/845e7e62c673.

The reftest is https://treeherder.mozilla.org/logviewer.html#?job_id=156965624&repo=autoland&lineNumber=57942.

And this is annoying, the relevant selector is:

  https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/toolkit/themes/osx/global/tabbox.css#51

And that makes sense, because we're not handling :not(..) correctly from the invalidation code (we need to track the negation state and return !negated).

Will fix tomorrow.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eafefacbfaf9
Handle document state changes using the invalidation machinery. r=xidorn

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/eafefacbfaf9

Comment 18

[Emilio Cobos Álvarez (:emilio)]

The fix for this was https://github.com/servo/servo/pull/19805.