1417970 - Make recascading when only custom properties change cheaper

Status: NEW

Description

[Randell Jesup [:jesup]]

Created attachment 8929041 [details]
jprof profile

innovati reports serious perf issues with dynamic CSS pages in 57.

On Mac, for this URL below, I see 100% + 35% (135%) for firefox 57; in Chrome I see 26 + 22 + 13 = 51%, and the Firefox framerate is much lower (content seems to have maxed out the core)

Attached is a jprof profile from m-c as of a week-ish ago.

Comment 1

[Randell Jesup [:jesup]]

Created attachment 8929043 [details]
jprof separated by thread

Each thread broken out

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Regarding styling performance, a CSS variable is changed in the root, so we need to recascade the whole thing...

If that's slow we may want to look into speeding up the whole thing, or into optimizing the special-case of switching variables on the root... But the second one doesn't sound like a trivial problem to me.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Per the conversation on #developers, this doesn't seem to be a regression from 56.

Comment 4

[Chris Peterson [:cpeterson]]

Emilio says this is not a regression or caused by Stylo because Gecko's old style system in 56 is also slow. 75%+ of the time in his profile was painting, not styling.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> Regarding styling performance, a CSS variable is changed in the root, so we
> need to recascade the whole thing...
> 
> If that's slow we may want to look into speeding up the whole thing, or into
> optimizing the special-case of switching variables on the root... But the
> second one doesn't sound like a trivial problem to me.

I'm wondering whether we can have a fastpath to skip elements which are not affected by variable changes, and only recascade subtrees which do reference variables, or preferably, only those reference changed variables.

Comment 6

[Tobi Reif]

I hope that these serious perf issues can be fixed.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Created attachment 8942822 [details]
testcase with single page HTML spec

To be honest, I don't really see any "serious" performance issue here... It seems to behave pretty well. But yeah, I can see that we basically need to restyle the whole page over and over again, which can become a problem when the page grows larger.

There is a "better" testcase for this issue, which is using the flashlight overlay with the single page HTML spec.

As far as I can see, Firefox has a higher frame rate than Chrome here, probably thanks to parallelism, although the frame rate is low enough that it is basically unusable in either browser.

This may also indicate that Chrome doesn't really have any special optimization for this case, and thus they basically need to restyle the whole page over and over again just like us.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

So my suggestion to this would be... don't do this... If you want this effect, just create a new element and append it at the end of the document and change the style of that. It would be much more performant.

From the browser side, I guess we have something doable, though. We can probably skip cascading reset properties if
* the rule node doesn't change
* the reset properties are cacheable
and skip cascading inherited properties if
* the rule node doesn't change
* no inherited property in parent is changed (we can special-case variables somehow)

I can see that setting variables on the root element maybe something authors would like to do, so maybe it's worth some further optimization at some point... but I have no idea whether it's something we should prioritize.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(Removing the stylo: prefix since this isn't a stylo regression)

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Just for a record, another idea occurs to me is that, we can probably create an invalidation map of variable to selectors for elements which can be affected by the given variable. When a variable is changed somewhere, we then use that map to invalidate elements. That way we may be able to avoid majority of restyle.

The difficulty here would be that we may need a special cascading procedure to maximize the usefulness of this.

Another related thing about this: based on the feature request from Apple, as well as the widely-used variables-on-root pattern, CSSWG has resolved to add a concept of environment variables, which are variables defined globally rather than in element rules, and referenced via env() CSS function. That would be a much better fit for this kind of use cases, and the aforementioned optimization can be implemented in a cleaner way with this concept.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Having it block bug 1420423 since we can see the same pattern in the UI (see bug 1435122).

If we want to do the optimization mentioned in comment 10, we may want to have a custom-property-only cascading mode, and then an invalidation (or can be have them together?)

It is not completely clear to me how that would look like at the moment.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Ok, so I thought about this a bit with Xidorn on IRC. Two ideas:

First of all, worth a shot, try to use the style sharing cache for CascadeOnly restyles too. This is low-effort, and may improve performance basically for free, so it's worth trying, specially on our browser chrome.

Second, which is the hard fix, would be something like:

 * Adding a RestyleHint::RECASCADE_CUSTOM_PROPERTIES hint, or something like that.
 * Hook it up with the CalcStyleDifference / ChildCascadeRequirement / traversal machinery.
 * Record during the cascade whether the style contains any variable reference. We can optimize further tracking all the actual variables referenced from the style, but that would bloat memory substantially, so probably the first is more than enough.
 * Make that hint look into that flag and basically act as `CascadeOnly` when variables are referenced, and `CascadeCustomProperties` (with a custom path that just does that and copies all the style structs) if not, which is the actual optimization.

I can look into that tomorrow (3AM here :P)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

I have an (unmeasured, for now) patch in https://github.com/servo/servo/pull/20151.