Performance issue with multiple :has rules
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
Tracking | Status | |
---|---|---|
thunderbird_esr115 | --- | unaffected |
firefox-esr115 | --- | unaffected |
firefox121 | --- | wontfix |
firefox122 | + | fixed |
firefox123 | + | fixed |
People
(Reporter: liamre, Assigned: dshin)
References
(Blocks 2 open bugs, Regression, )
Details
(Keywords: regression)
Attachments
(4 files)
512 bytes,
text/html
|
Details | |
48 bytes,
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-release+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-release+
|
Details | Review |
841.12 KB,
video/mp4
|
Details |
Steps to reproduce:
- Visit
https://jsfiddle.net/e70xjovw/8/
- Type something in the search box labeled "Enter product"
Actual results:
The browser hung for 30+ seconds, and I was prompted to close the tab because it was slowing down Firefox.
Expected results:
The browser should not have hung.
Reporter | ||
Comment 1•7 months ago
|
||
Running mozregression with layout.css.has-selector.enabled
set to true yielded the following:
2024-01-10T20:29:40.226000: INFO : Narrowed integration regression window from [faf426a5, 97c3c6c6] (3 builds) to [faf426a5, d02d47db] (2 builds) (~1 steps left)
2024-01-10T20:29:41.284000: DEBUG : Found commit message:
Bug 1860136: Fix up check for subject/non-subject match in relative selector invalidation. r=emilio
Differential Revision: https://phabricator.services.mozilla.com/D191486
Performance profile: https://share.firefox.dev/47xbH7c
Reporter | ||
Updated•7 months ago
|
Comment 2•7 months ago
|
||
:dshin, since you are the author of the regressor, bug 1860136, could you take a look? Also, could you set the severity field?
For more information, please visit BugBot documentation.
Comment 3•7 months ago
•
|
||
I can reproduce.
Here's the reporter's perf profile as a clickable link:
https://share.firefox.dev/47xbH7c
Here's a performance profile that I recorded (where I just typed "abc" into the box) -- I recorded with "sequential styling" enabled since sometimes that makes stylo profiles easier to reason about (though that might not matter in this case):
https://share.firefox.dev/3U1jEhX
Updated•7 months ago
|
Comment 4•7 months ago
|
||
--> S2 given the size of the perf gap here, and that this affects real sites in-the-wild e.g. https://github.com/fomantic/Fomantic-UI/issues/2971
Updated•7 months ago
|
Comment 5•7 months ago
|
||
Here is also discussion about this bug:
https://discourse.mozilla.org/t/has-anyone-else-noticed-discourse-slowdowns-following-120-121-upgrades/126062/6
Affects also our live site.
Updated•7 months ago
|
Comment 6•7 months ago
|
||
For reference, comment 5 looks similar but a bit different (the :has()
is in an ancestor selector).
Would you be able to provide a test-case for that Jyri?
Comment 7•7 months ago
|
||
Sorry can't help, because we are using Fomantic-ui, so our issue was the same as the original one here. Try to contact Sam directly:
https://discourse.mozilla.org/t/has-anyone-else-noticed-discourse-slowdowns-following-120-121-upgrades/126062/4
Comment 8•7 months ago
|
||
Yeah so at least in this case, Servo_StyleSet_MaybeInvalidateRelativeSelectorNthDependencyFromSibling
is quadratic when removing nodes front to back, which is not great... But there's also the question of why are we ending up there. There are only edge child selectors inside the :has(). Probably something else in the CSS is triggering that.
But also, for this case, where the :has()
is in the subject position, we don't need to invalidate at all, we've already thrown away the towel here and restyled the whole subtree.
Assignee | ||
Comment 9•7 months ago
|
||
... But there's also the question of why are we ending up there. There are only edge child selectors inside the :has(). Probably something else in the CSS is triggering that.
All nth-like selectors go into the same invalidation bucket, which is triggering this behaviour
Assignee | ||
Comment 10•7 months ago
|
||
STR: Click "Go"
Expected: No jank
Actual: Noticeable jank, indicated by button being stuck on clicked state
- First selector marks #container with
HAS_SLOW_SELECTOR_LATER_SIBLINGS
, so any time a child is removed, we run invalidation on siblings after the removed element. - :{first|last}-child selectors go into Nth invalidation bucket, so we try to invalidate the second set of selectors on all of them
:has
invalidation looks for what can anchor the:has()
selectors - in this case, it's#container
.- Before bug 1860136, we'd incorrectly consider these
:has()
selectors to be non-subject, and#container
is a subject anchor, so we'd just not run the invalidation - We go on to invalidate the pseudo-element, which we do by invalidating the
#container
and its descendants.
Assignee | ||
Comment 11•7 months ago
|
||
Assignee | ||
Comment 12•7 months ago
|
||
Depends on D198464
Comment 13•7 months ago
|
||
Pushed by dshin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f30b3089d593 Distinguish edge selectors vs. other nth selectors. r=firefox-style-system-reviewers,emilio https://hg.mozilla.org/integration/autoland/rev/dc36bc5a45ac Add regression perf test. r=firefox-style-system-reviewers,perftest-reviewers,emilio,sparky
Comment 14•7 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f30b3089d593
https://hg.mozilla.org/mozilla-central/rev/dc36bc5a45ac
Comment 15•7 months ago
|
||
The patch landed in nightly and beta is affected.
:dshin, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- If no, please set
status-firefox122
towontfix
.
For more information, please visit BugBot documentation.
Assignee | ||
Comment 16•7 months ago
|
||
Comment on attachment 9372640 [details]
Bug 1874066: Distinguish edge selectors vs. other nth selectors. r=#style
Beta/Release Uplift Approval Request
- User impact if declined: Perf issue as described on comment 0, with in-the-field reports as in comment 4.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): * A relative small change for big perf difference
- Confirmed no jank in example from comment 0 with
mozregression --launch 2024-01-16
- String changes made/needed:
- Is Android affected?: Yes
Assignee | ||
Updated•7 months ago
|
Comment 17•7 months ago
|
||
Comment on attachment 9372640 [details]
Bug 1874066: Distinguish edge selectors vs. other nth selectors. r=#style
Fx122 is now in release, switching to a release uplift request for possible inclusion in an RC respin or dot release.
Updated•7 months ago
|
Comment 18•7 months ago
|
||
Based on the report received via webcompat the issue is still present on Firefox Nightly (123): https://github.com/webcompat/web-bugs/issues/132131#issuecomment-1894242344
There's a delay of 2-3 seconds when clicking on "Select Animal" drop down on: https://fomantic-ui.com/modules/dropdown.html#sanitized-version
Note: On Chrome works just fine.
Updated•7 months ago
|
Updated•7 months ago
|
Assignee | ||
Comment 19•7 months ago
|
||
(In reply to Calin Tanase from comment #18)
Tracking with bug 1875081
Comment 20•7 months ago
|
||
Comment on attachment 9372640 [details]
Bug 1874066: Distinguish edge selectors vs. other nth selectors. r=#style
Approved for 122.0 RC2
Comment 21•7 months ago
|
||
Comment on attachment 9372641 [details]
Bug 1874066: Add regression perf test. r=#style,#perftest-reviewers
Approved for 122.0 RC2
Comment 22•7 months ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/339d777b398d https://hg.mozilla.org/releases/mozilla-release/rev/a61e9d5fdc61
Updated•7 months ago
|
Comment 23•7 months ago
•
|
||
I've checked the issue on Windows 10.
Reproducible: Firefox Release 121.0.1
Not reproducible: Firefox Beta 122, Nightly 123.0a1 (2024-01-18)
Edit: Tested on https://archive.mozilla.org/pub/firefox/candidates/122.0-candidates/build2/ and works as expected
Description
•