Open Bug 1417970 Opened 3 years ago Updated 2 months ago

Make recascading when only custom properties change cheaper

Categories

(Core :: CSS Parsing and Computation, defect, P3)

57 Branch
defect

Tracking

()

Tracking Status
firefox57 --- wontfix
firefox58 --- ?
firefox59 --- ?

People

(Reporter: jesup, Assigned: emilio)

References

(Blocks 3 open bugs, )

Details

(Whiteboard: [layout:p3][qf:p3:67])

Attachments

(4 files)

Attached file 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.
Each thread broken out
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.
Summary: Perf issues with dynamic CSS(?) in 57/Stylo → stylo: Perf issues with dynamic CSS recascade in 57/Stylo
Version: 55 Branch → 57 Branch
Priority: -- → P1
Per the conversation on #developers, this doesn't seem to be a regression from 56.
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.
Priority: P1 → P3
Summary: stylo: Perf issues with dynamic CSS recascade in 57/Stylo → stylo: Perf issues with dynamic CSS recascade
(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.
I hope that these serious perf issues can be fixed.
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.
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.
(Removing the stylo: prefix since this isn't a stylo regression)
Summary: stylo: Perf issues with dynamic CSS recascade → Perf issues with dynamic CSS recascade
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.
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.
Blocks: 1420423
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)
Summary: Perf issues with dynamic CSS recascade → Make recascading when only custom properties change cheaper
Flags: needinfo?(emilio)
Depends on: 1435247
I have an (unmeasured, for now) patch in https://github.com/servo/servo/pull/20151.
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Blocks: 1452622
Whiteboard: [layout:p3][qf]
Whiteboard: [layout:p3][qf] → [layout:p3][qf:p3:67]

This didn't seem to improve times enough for it to be worth landing IMO.

Blocks: 1656327
Duplicate of this bug: 1656327
You need to log in before you can comment on or make changes to this bug.