Firefox spends 70x more time than Chrome styling Backbone-Complex-DOM subtest
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
People
(Reporter: denispal, Unassigned, NeedInfo)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
|
8.28 KB,
patch
|
Details | Diff | Splinter Review | |
|
4.72 MB,
text/plain
|
Details |
Test can be found here: https://browserbench.org/Speedometer3.1/?suite=TodoMVC-Backbone-Complex-DOM&iterationCount=50
This test is not officially part of SP3 so it's not super important. But Firefox spends 6x more time than Chrome on this test. Specifically, we spend around 70x more time in styling than Chrome does.
Firefox:
- Score: 12.52
Chrome:
- Score: 66.72
Firefox Top Styling Functions:
1. style::rule_tree::compute_rule_node - 2237 samples
2. style::stylist::Stylist::push_applicable_declarations - 1568 samples
3. Combined styling: ~3800+ samples
Chrome Top Styling Functions:
1. blink::ElementRuleCollector::CollectMatchingRules - 25 samples
2. blink::StyleResolver::ResolveStyle - 15 samples
3. blink::SelectorChecker::MatchSelector - 15 samples
4. Combined styling: ~55 samples
Firefox: https://share.firefox.dev/4sEuYz5
Chrome: https://share.firefox.dev/45clIZa
|
Reporter | |
Updated•4 months ago
|
|
||
Comment 1•4 months ago
|
||
I was curious and I took a quick look, we're hitting I think a rather pathological case. There's this selector in speedometer which I believe is wrong:
.filter-group > div > div > :first-child label {
padding-top: 1px;
padding-bottom: 1px;
}
If we try to match it with a random label, and that has no :first-child ancestor, that ends up propagating all the way to the root looking for a :first-child. Then jQuery does something like appending to the <body> to figure out the default display of an element, and that just causes us to restyle the whole <body>...
Can you confirm that removing that selector (or the one above) helps by any chance?
|
|
Reporter | |
Comment 2•4 months ago
|
||
I measured without the selector but it unfortunately seems to be the same performance as before.
|
|
||
Updated•4 months ago
|
|
|
||
Comment 3•4 months ago
|
||
Ok, so that selector was part of the problem, but there's another one that's problematic. Here's a diagnostic patch that prints out when we set the slow selector flags on a node, the selector we were matching, and which element is affected by it.
|
|
||
Comment 4•4 months ago
|
||
Here's the log that that produces on a run. There are two instances of us just setting the flag all the way up to the root / body elements:
...
apply_selector_flags(<body> (0x7f2df1a11c10), ElementSelectorFlags(HAS_SLOW_SELECTOR_LATER_SIBLINGS)
apply_selector_flags(<html lang="en" data-framework="backbone" data-version="1.4.1" data-features="" class="spectrum spectrum--medium spectrum--l..."> (0x7f2df1a118b0), ElementSelectorFlags(HAS_SLOW_SELECTOR_LATER_SIBLINGS)
matches_selector(<button class="spectrum-ActionButton spectrum-Action..."> (0x7f2df1a084e0), Selector(.reminder-group .is-invalid ~ * button, specificity = 0x801, flags = 0x0))
and:
...
apply_selector_flags(<div class="ribbon"> (0x7f2df1a1aa60), ElementSelectorFlags(HAS_SLOW_SELECTOR_NTH | HAS_EDGE_CHILD_SELECTOR)
apply_selector_flags(<div class="main-ui" dir="ltr"> (0x7f2df1a11ca0), ElementSelectorFlags(HAS_SLOW_SELECTOR_NTH | HAS_EDGE_CHILD_SELECTOR)
apply_selector_flags(<body> (0x7f2df1a11c10), ElementSelectorFlags(HAS_SLOW_SELECTOR_NTH | HAS_EDGE_CHILD_SELECTOR)
apply_selector_flags(<html lang="en" data-framework="backbone" data-version="1.4.1" data-features="" class="spectrum spectrum--medium spectrum--l..."> (0x7f2df1a118b0), ElementSelectorFlags(HAS_SLOW_SELECTOR_NTH | HAS_EDGE_CHILD_SELECTOR)
matches_selector(<label for="stepper-m" class="spectrum-FieldLabel spectrum-FieldLab..."> (0x7f2df1a2d820), Selector(.filter-group > div > div > :first-child label, specificity = 0x803, flags = 0x0))
Both of those selectors are kinda silly... The common thing pattern here, tho, is that in this particular case we could avoid restyling, because by the time we're looking at :first-child, we haven't tried to match .filter-group > div > div / .reminder-group...
For the .filter-group > div > div > :first-child label selector, we could prove that <body> / <html> will never be affected by this selector. For .reminder-group .is-invalid ~ * button tho, that's a bit harder, because you could imagine something like document.documentElement.classList.add("reminder-group") and document.head.classList.add("is-invalid"), and that would affect every button on the page...
We might need to try to implement more fine-grained invalidation for these DOM mutations. I guess we do some of that for :nth- of, so maybe it's not that terrible to extend it... Maybe just doing a relatively simple match from the relevant combinator offset would do?
David, any thoughts, since you have also worked with this code?
|
|
||
Comment 5•4 months ago
|
||
Dennis can you try removing both the selectors mentioned above? The one I identified in comment 1, and this one?
.reminder-group .is-invalid ~ * button {
visibility: hidden;
}
I'd expect that to make a massive difference here.
|
|
||
Comment 6•4 months ago
|
||
This seems relatively rare, so triaging as S3 for now.
It's also only an issue because jQuery does something like this on show() to compute the default display of the element, which is rather weird?
Looking around this code seems to have some history, but its behavior is super weird...
Michal, the behavior of getDefaultDisplay is rather weird in the sense that it just gets the display value of an element with the same tagname, but in a totally different position in the DOM (and without any of the class-based styling). That means that something like:
<!doctype html>
<script src="https://code.jquery.com/jquery-3.7.1.js"></script>
<style>
.main { display: flex }
.hidden { display: none }
</style>
<random-element class="main hidden"></random-element>
<script>
$(".main").show();
</script>
Will end up with display: inline, which is unlikely to be what you want?
If I were to implement it, I'd probably just do something like defaulting to block except for standard tags (so <tr> will return table-row, <a> and <span> will return inline, etc, everything else would be block)...
How likely is that to be changeable? I suspect not very, but that code basically trashes the layout of the whole page at the very least (and styling if you have silly selectors like the ones described above), and it seems most of the regressions fixed by that commit were about not having a default display at all...
|
|
Reporter | |
Comment 7•4 months ago
|
||
I disabled those 3 and a couple more in resources/todomvc/big-dom-generator/src/app.css but I still see similar performance with them all commented out:
.filter-group > div > div > :first-child > * {
max-width: 150px;
}
.filter-group > div > div > :first-child label {
padding-top: 1px;
padding-bottom: 1px;
}
.filter-group > div > div > :last-child {
display: flex;
direction: column;
justify-content: flex-end;
}
.notifications-group .notifications-list > li div :first-child {
grid-column-start: 1;
grid-column-end: 2;
}
.reminder-group .is-invalid ~ * button {
visibility: hidden;
}
.backlog-group li :last-child button {
background-color: white;
border-color: black;
border-width: 1px;
}
|
|
||
Comment 8•4 months ago
|
||
Yeah, sorry, I think that is the source, but not what gets used. I think the one that's used is resources/todomvc/architecture-examples/backbone-complex/dist/big-dom-with-stacking-context-scrollable.css, which has those selectors minified. I was hoping to avoid having to learn how that's generated? :)
|
|
Reporter | |
Comment 9•4 months ago
|
||
(In reply to Denis Palmeiro [:denispal] from comment #2)
I measured without the selector but it unfortunately seems to be the same performance as before.
I was commenting the selectors out in the wrong file. I disabled the selector from comment 1 in the generated file instead and the score improved dramatically from 12 -> 60. Turning off the other selectors didn't seem to help any further.
Here is an updated profile with the selector disabled: https://share.firefox.dev/49MCJuy. All of the styling time has basically vanished.
|
|
Reporter | |
Updated•4 months ago
|
|
||
Comment 10•4 months ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
This seems relatively rare, so triaging as S3 for now.
It's also only an issue because jQuery does something like this on show() to compute the default display of the element, which is rather weird?
jQuery's .show() does have a history, but conceptually, its is to guess what display the element would have if it didn't have its display set to none. If its set like that via inline styles, we already just set display to an empty string, but there's no way to reliably handle the case when it's done in a stylesheet.
There's no way for us to differentiate between one class setting a display to flex and another one setting it to none. In that case, we at least want to know the default display for the tag name of the element we're trying to show in a fresh document without user styles. Ideally, we'd have a Web API that provides it but there's nothing like that AFAIK.
We used to do even more crazy stuff, like temporarily attach an iframe to which we'd append an element of a certain tag name to measure its default display, but we simplified it in jQuery 3.0.0.
If I were to implement it, I'd probably just do something like defaulting to
blockexcept for standard tags (so<tr>will returntable-row,<a>and<span>will return inline, etc, everything else would be block)...
That would require jQuery to maintain a large-ish mapping from all known non-inline tag names and keep it up to date. We've been bitten a few times by us manually maintaining such lists (e.g. in https://api.jquery.com/jQuery.cssNumber/), so we prefer to run some kind of detection rather than maintain hard-coded expanding lists; especially that we're wary of the library size and we literally count bytes in minified gzipped builds for our changes to make sure we're not adding too much.
When working on jQuery 3.x, we did experiment with just handling the inline style case:
https://github.com/jquery/jquery/commit/86419b10bfa5e3b71a7d416288ab806d47a31d1f
but it broke too much. I think there was an idea of just setting display to block in such cases, but in the end we had to backtrack.
In any case, even if we did manage to simplify the logic and avoid running this test internally, it's not like the whole web would immediately update to that version; there's a long tail of old jQuery versions people just don't update.
I'll CC the other jQuery team members who were more active in these "default display" changes in case I missed any important context.
How likely is that to be changeable? I suspect not very, but that code basically trashes the layout of the whole page at the very least (and styling if you have silly selectors like the ones described above), and it seems most of the regressions fixed by that commit were about not having a default display at all...
We do know .show() has a potential for performance issues due to this and we warn about that in our docs: https://api.jquery.com/show/
|
|
||
Comment 11•4 months ago
|
||
BTW, there was time when jQuery was using getDefaultComputedStyle when available, but it was causing a number of issues like:
and we removed it. Besides, this API is Firefox-only and we now avoid using such non-standard APIs.
|
||
Comment 12•4 months ago
|
||
Redirect needinfos that are pending on inactive users to the triage owner.
:emilio, since the bug has recent activity, could you have a look please?
For more information, please visit BugBot documentation.
|
|
||
Updated•4 months ago
|
Description
•