Closed Bug 1874066 Opened 6 months ago Closed 6 months ago

Performance issue with multiple :has rules

Categories

(Core :: CSS Parsing and Computation, defect)

Firefox 123
defect

Tracking

()

RESOLVED FIXED
123 Branch
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)

Steps to reproduce:

  1. Visit https://jsfiddle.net/e70xjovw/8/
  2. 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.

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

: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.

Flags: needinfo?(dshin)

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

--> 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

Severity: -- → S2
Status: UNCONFIRMED → NEW
Ever confirmed: true

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?

Flags: needinfo?(jkytomak)

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

Flags: needinfo?(jkytomak)

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.

... 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: nobody → dshin
Flags: needinfo?(dshin)

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.
Blocks: 1874485
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
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 123 Branch

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 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(dshin)

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
Flags: needinfo?(dshin)
Attachment #9372640 - Flags: approval-mozilla-beta?
Attachment #9372641 - Flags: approval-mozilla-beta?

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.

Attachment #9372640 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Attachment #9372641 - Flags: approval-mozilla-beta? → approval-mozilla-release?

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.

Flags: needinfo?(smolnar)
Blocks: 1875081
Flags: needinfo?(smolnar)

(In reply to Calin Tanase from comment #18)

Tracking with bug 1875081

Comment on attachment 9372640 [details]
Bug 1874066: Distinguish edge selectors vs. other nth selectors. r=#style

Approved for 122.0 RC2

Attachment #9372640 - Flags: approval-mozilla-release? → approval-mozilla-release+

Comment on attachment 9372641 [details]
Bug 1874066: Add regression perf test. r=#style,#perftest-reviewers

Approved for 122.0 RC2

Attachment #9372641 - Flags: approval-mozilla-release? → approval-mozilla-release+

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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: