1334732 - stylo: Handle all the ways that style data can be invalidated

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Just discussed this with dbaron a little bit, and here's how I think this stuff should work.

There are basically 4 types of invalidation to handle:
(1) The set of available rules changed, either by explicitly adding/removing/reordering stylesheets, or by virtue of matching different media queries.
(2) Selectors need to be rematched, either because the document changed, or because of (1).
(3) Computed values need to be regenerated, either because of a changed rule node from (2), or because the default computed values changed, or because something affecting value computation (like default system color) changed.
(4) Layout changes need to be made, either due to running CalcStyleDifference after (3), or other things that caused Gecko to pass an explicit change hint.

We currently track (2) and (4) via the nsRestyleHint and nsChangeHint stored on the Element (via the lazily-allocated RestyleData), which is quite elegant. We sorta track (3) in the same way, except it's a per-element boolean, which is insufficient for expressing "the entire tree needs to be recascaded" (due to mutated initial values of stuff like default system color). So I think we should replace that boolean with a CascadeHint, which has Cascade_Self and Cascade_Descendants, and propagate it down the tree like we do for restyle hints.

Our handling of (1) is currently incorrect in various ways, and needs to be fixed. I think the best approach here is to move the tracking of "the style rules have changed" into gecko, and have the servo-side machinery be a dumb mirror. When the Gecko machinery concludes that the set of rules have changed, it does an FFI call to flush the stylist, and calls Servo_NoteExplicitHints to pass the appropriate RestyleHint and CascadeHint to all the style roots.

One interesting effect here is that, as long as we're not caching structs on the rule tree, there's no reason to drop the rule tree in any of these four scenarios. Once (2) computes new rule nodes, all the old ones will just become unused, and will be destroyed next time we GC.

Comment 1

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #0)
> We currently track (2) and (4) via the nsRestyleHint and nsChangeHint stored
> on the Element (via the lazily-allocated RestyleData), which is quite
> elegant. We sorta track (3) in the same way, except it's a per-element
> boolean, which is insufficient for expressing "the entire tree needs to be
> recascaded" (due to mutated initial values of stuff like default system
> color). So I think we should replace that boolean with a CascadeHint, which
> has Cascade_Self and Cascade_Descendants, and propagate it down the tree
> like we do for restyle hints.

It sounds like Cascade_Descendants is equivalent to eRestyle_ForceDescendants, which is used to override the logic that decides if restyling can stop at a certain node, and causes the traversal to progress all the way down instead?

Comment 2

[Boris Zbarsky [:bzbarsky]]

For (1), what's our plan for putting in place optimizations like the ones described in bug 1213122 comment 13 or bug 1213122 comment 34?  It's not something Gecko does at the moment, so not supporting it initially is not necessarily a blocker, but we should have a plan, because I _do_ think we'll want this in the near future.  It keeps popping up.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #1)
> It sounds like Cascade_Descendants is equivalent to
> eRestyle_ForceDescendants, which is used to override the logic that decides
> if restyling can stop at a certain node, and causes the traversal to
> progress all the way down instead?

Ah, indeed! I think I'd still like to keep them separate in Servo, since I think it makes the code more understandable. But we can just map from those Gecko hints to the right Servo stuff.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> For (1), what's our plan for putting in place optimizations like the ones
> described in bug 1213122 comment 13

I think this would work fine in the setup described in comment 0, since we're explicitly not blowing away the old rule tree. We'd just put Restyle_SomeDescendants on the root instead of Restyle_Subtree.

> or bug 1213122 comment 34?

Is this the same thing as the one above? It seems like it to me, but there may be a distinction I'm missing.

Comment 4

[Boris Zbarsky [:bzbarsky]]

They're somewhat similar, and the goals are pretty similar, but the details are a bit different.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> For (1), what's our plan for putting in place optimizations like the ones
> described in bug 1213122 comment 13 or bug 1213122 comment 34?  It's not
> something Gecko does at the moment, so not supporting it initially is not
> necessarily a blocker, but we should have a plan, because I _do_ think we'll
> want this in the near future.  It keeps popping up.

Just dumping my thoughts on this at the moment:

I really like the Blink approach to this because it allows you to run whatever complex logic you want when style has been invalidated without bloating the common restyling path.

That being said, Blink has a much more complex system that the one we've designed for stylo so far to optimize the stuff that eRestyle_SomeDescendants does (they basically do some selector analysis and fill some selector invaliadtion sets[1], then check those sets when an element class name/tag name/attribute/etc. changes, and find the selectors that may affect, then run the invalidation pass in the same way that when they add a rule with a given selector).

Note that we already have something that roughly resembles this, which is the dependency tracking we do to calculate restyle hints.

Not sure which of the approaches (either eRestyle_SomeDescendants or this separated invalidation phase) do we want. One thing to note is that eRestyle_SomeDescendants requires to keep the selector list too. The way we store restyle hints on the elements right now means that it would be at least a pointer more in the element data that I'm not sure we can afford.

[1]: https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/css/RuleFeature.cpp#441

Comment 6

[Bobby Holley (:bholley)]

Cameron is taking this as part of incremental restyle.

Comment 7

[Bobby Holley (:bholley)]

Cameron, what's the status on this? I think we may need a way to trigger a rescascade in bug 1290276.

Comment 8

[Cameron McCormack (:heycam)]

I'll work on this next.  Filed bug 1367647 for it.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Is there anything actionable in this bug anymore?

Should this be closed?

Comment 10

[Bobby Holley (:bholley)]

I think it's all taken care of, will let heycam confirm.

Comment 11

[Bobby Holley (:bholley)]

Resolving WFM, heycam can reopen if he wants.