cache ComputedStyles for anonymous content we know is always styled the same
Categories
(Core :: Layout, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
References
Details
(Keywords: perf)
Attachments
(4 files)
On pages with many scrollable elements, we can end up spending a lot of time styling the anonymous scrollbar content on the main thread. For example, when loading the single page HTML spec, we spend ~840 ms out of the total ~8,950 ms load time (~9%) under nsCSSFrameConstructor::GetAnonymousContent
's call to StyleNewSubtree
.
This is what the distribution of frame types for GetAnonymousContent
calls looks like in that document:
1 GetAnonymousContent Canvas
739 GetAnonymousContent GfxButtonControl
912 GetAnonymousContent Details
1229 GetAnonymousContent Scroll
2234 GetAnonymousContent Scrollbar
So 66% of the anonymous content generating frames are scrollbar-related. Scrollframes generate four elements (two <scrollbar>
s, a <scrollcorner>
, and a <resizer>
). Scrollbars themselves generate six elements (four <scrollbarbutton>
s, some of which are display:none depending on the platform, the <slider>
, and the <thumb>
). So by number of elements these consist of the vast majority of anonymous content styling work.
If we ignore scrollbar-color, then the number of unique ComputedStyle
objects we need for all the scrollbar parts is manageable, and mostly depends on a few specific attributes set on the elements. I think the only other inherited property values that matter for scrollbar parts are scrollbar-width and visibility. Apart from that, we should be able to cache the ComputedStyle
for a given combination of these attributes and reuse them on subsequent scrollable elements.
The downside of this is increased coupling between the UA sheets that style the scrollbar parts and the code that checks the attributes that we'd key the cache off. But the UA sheet rules for scrollbar parts don't change very often, so maybe a comment reminding people to check whether the caching code needs updating is enough.
Comment 1•5 years ago
|
||
If we ignore scrollbar-color, then the number of unique ComputedStyle objects we need for all the scrollbar parts is manageable, and mostly depends on a few specific attributes set on the elements. I think the only other inherited property values that matter for scrollbar parts are scrollbar-width and visibility.
And color
(to resolve scrollbar-color), right?
The downside of this is increased coupling between the UA sheets that style the scrollbar parts and the code that checks the attributes that we'd key the cache off. But the UA sheet rules for scrollbar parts don't change very often, so maybe a comment reminding people to check whether the caching code needs updating is enough.
Is the plan to cache styles in the whole subtree? Also, is this something that could be enforced by asserts?
Assignee | ||
Comment 2•5 years ago
|
||
(In reply to Emilio Cobos Γlvarez (:emilio) from comment #1)
And
color
(to resolve scrollbar-color), right?
Yes. I was thinking just to check if scrollbar-color is non-auto, and skip caching in that case, just to avoid issues like someone animating scrollbar-color and blowing out the cache.
The downside of this is increased coupling between the UA sheets that style the scrollbar parts and the code that checks the attributes that we'd key the cache off. But the UA sheet rules for scrollbar parts don't change very often, so maybe a comment reminding people to check whether the caching code needs updating is enough.
Is the plan to cache styles in the whole subtree?
Yeah, although the only NAC subtree that actually has a child of a NAC root is <slider>
, which has the <thumb>
as its child.
Also, is this something that could be enforced by asserts?
I think so, yes. We could resolve a new ComputedStyle
, and assert that all reset properties, scrollbar-width, and visibility match.
I can't think of a good way to assert that we don't accidentally rely on some other inherited property.
Comment 3•5 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #2)
I can't think of a good way to assert that we don't accidentally rely on some other inherited property.
We could maybe add:
all: initial;
At the first definition of the anon box, then in debug builds we can assert that the style is exactly the same as the cached one. Assuming it makes the style not too slow to compute, which I hope it won't...
Otherwise just hoping that we don't add code that depends on inherited properties seems very handwavey in my opinion... We probably also depend on a bunch of other properties accidentally (and non-accidentally) like visibility
for sure.
Assignee | ||
Comment 4•5 years ago
|
||
Great idea, I'll try adding a scrollcorner, resizer, scrollbar, scrollbarbutton, slider, thumb { all: initial; }
rule to the top of minimal-xul.css and see what the effect is.
I misremembered about scrollbar-width too; that's a reset property, and none of the UA rules set it explicitly on the scrollbar part elements, so we don't need to worry about it.
Assignee | ||
Comment 5•5 years ago
|
||
Not only that, but scrollbar-color, while inherited, is only ever looked at on the originating element of the NAC (see the various nsLayoutUtils::StyleForScrollbar
calls). So we should be able to cache scrollbar styles with non-default colors after all.
Assignee | ||
Comment 6•5 years ago
|
||
Some added trickiness is that the initial values of font related properties and of direction depend on the language in the document, and so might change. Since the scrollbar parts don't rely on font or direction values coming from the scrollable elements, we can reset them to some known values rather than use initial
.
Assignee | ||
Comment 7•5 years ago
|
||
I have a patch now that reduces time spent under nsCSSFrameConstructor::GetAnonymousContent
from ~820 ms down to ~25 ms, for the HTML spec.
Assignee | ||
Comment 8•5 years ago
|
||
Assignee | ||
Comment 9•5 years ago
|
||
Assignee | ||
Comment 10•5 years ago
|
||
Assignee | ||
Comment 11•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
For nsGfxScrollFrame
, the number of NAC elements we produce varies (depending on which scrollbars we have), but for nsScrollbarFrame
, we always produce the same six, so if we wanted to we could change the setup so that we could use a single AnonymousContentKey
to represent all of a frame's anonymous content, rather than separate values for each element.
Assignee | ||
Comment 13•5 years ago
|
||
On my machine this test takes ~5200 ms with the anonymous content style
caching pref disabled, and ~1000 ms with it enabled.
Comment 14•5 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #13)
Created attachment 9074395 [details]
Bug 1554571 - Part 4: Add perf-reftest singleton for anonymous content style caching.On my machine this test takes ~5200 ms with the anonymous content style
caching pref disabled, and ~1000 ms with it enabled.
Super cool!
Assignee | ||
Comment 15•5 years ago
|
||
Comment 16•5 years ago
|
||
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a2143551d7bc Part 1: Remove unused argument from Servo_ResolveStyle. r=emilio https://hg.mozilla.org/integration/autoland/rev/c14e9c381345 Part 2: Remove unused sbattr="scrollbar-thumb" from XUL thumb elements. r=emilio https://hg.mozilla.org/integration/autoland/rev/30728685499e Part 3: Cache computed styles of scrollbar part anonymous content. r=emilio https://hg.mozilla.org/integration/autoland/rev/1cc4e6374b8a Part 4: Add perf-reftest singleton for anonymous content style caching. r=emilio
Comment 17•5 years ago
|
||
Backed out 4 changesets for causing failures in minimal-xul.css
Backout link: https://hg.mozilla.org/integration/autoland/rev/0a8bb0b56daadd482efd43d00fea439eb0a39553
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253641360&repo=autoland&lineNumber=2286
2:36:27 INFO - Buffered messages finished
02:36:27 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Got error message for chrome://global/content/minimal-xul.css: Unknown property β-moz-list-reversedβ. Declaration dropped. -
02:36:27 INFO - Stack trace:
02:36:27 INFO - chrome://mochikit/content/browser-test.js:test_ok:1313
02:36:27 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:messageIsCSSError:256
02:36:27 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:455
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 18•5 years ago
|
||
Comment 19•5 years ago
|
||
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8c16d6d715bf Part 1: Remove unused argument from Servo_ResolveStyle. r=emilio https://hg.mozilla.org/integration/autoland/rev/0e6bda67a31b Part 2: Remove unused sbattr="scrollbar-thumb" from XUL thumb elements. r=emilio https://hg.mozilla.org/integration/autoland/rev/427a480c163a Part 3: Cache computed styles of scrollbar part anonymous content. r=emilio https://hg.mozilla.org/integration/autoland/rev/94b01ff1b161 Part 4: Add perf-reftest singleton for anonymous content style caching. r=emilio
Comment 20•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8c16d6d715bf
https://hg.mozilla.org/mozilla-central/rev/0e6bda67a31b
https://hg.mozilla.org/mozilla-central/rev/427a480c163a
https://hg.mozilla.org/mozilla-central/rev/94b01ff1b161
Updated•2 years ago
|
Description
•