1874050 - Too much time spent in CustomPropertiesBuilder::cascade on TodoMVC-Lit-Complex-DOM (copying custom properties from ancestors)

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

https://speedometer-preview.netlify.app/?suites=TodoMVC-Lit-Complex-DOM

We are 2x slower than Safari at "Update style" during the measured time of this benchmark.
Firefox: https://share.firefox.dev/3NZXB7o
Safari: https://share.firefox.dev/41RBlCa

Almost half the time is spent in CustomPropertiesBuilder::cascade.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Curious is this a recent regression? That code has seen a lot of change recently. It seems it's mostly copying custom properties map.

cc'ing some people that have worked on relevant code recently.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Taking a quick look at https://speedometer-preview.netlify.app/interactiverunner?suites=TodoMVC-Lit-Complex-DOM (clicking "step"), it seems the root element has approx 1600 custom properties. So every time we deep-clone the map, well, that's a lot of work.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Given ^ I don't think that'd be a regression.

Ok, it seems WebKit implements some optimization for this, where they keep the parent value references instead of copying them all here.

That seems worth doing, and we could implement such an optimization in ComputedCustomProperties or CustomPropertiesMap I believe. One gotcha is that we wouldn't be able to just remove custom properties from the map and would need some invalid value marker of sorts (or just making an Option the hashmap value).

Seems WebKit uses an "invalid" value:

https://searchfox.org/wubkat/rev/889804f28206ffa2dd7930e2e52df50dc4260049/Source/WebCore/style/StyleBuilderCustom.h#1991-1992

I'll put this on my queue to look at when time allows, but David maybe you're interested and have the cycles?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Probably a bit higher priority than our average S3, since having tons of custom props on the root is actually not that uncommon. Firefox has about 300.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Taking for now, will try to get it done this week.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1874050 - Share more memory in custom property storage. r=dshin,#style,#layout

This implements a similar optimization as WebKit's, see the bug comment.
This should be specially useful for sites with lots of custom
properties.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/jobs?repo=try&revision=ce940fd109e00a49470e857e3262dfe5f60be5cb looks pretty good, and Markus confirmed this helps a lot with that speedometer test.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Do we expect this to help on all of the ComplexDOM tests or just TodoMVC-Lit?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

At least a couple of the complex DOM ones I checked (didn't check them all) also suffered from this. They all seem to use this "spectrum" library thingie that uses thousands of custom props.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

So presumably all are affected.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a81660943a3
Share more memory in custom property storage. r=dshin

Comment 12

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/7a81660943a3

Comment 13

[Andrej (:aglavic)]

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=41034(In reply to Pulsebot from comment #11)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a81660943a3
Share more memory in custom property storage. r=dshin

We're seeing speedometer3 improvement in CI!

== Change summary for alert #41034 (as of Tue, 16 Jan 2024 20:47:35 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
2%	speedometer3	windows10-64-shippable-qr	fission webrender	11.62 -> 11.90	Before/After