Closed Bug 1844446 Opened 1 year ago Closed 2 months ago

Complex nested rules cause performance issues

Categories

(DevTools :: Inspector: Rules, defect, P2)

defect

Tracking

(firefox128 verified)

VERIFIED FIXED
128 Branch
Tracking Status
firefox128 --- verified

People

(Reporter: sebo, Assigned: nchevobbe)

References

(Blocks 2 open bugs, )

Details

Attachments

(3 files, 1 obsolete file)

Attached file test_case.html

In https://github.com/w3c/csswg-drafts/issues/2881#issuecomment-1642450622 there is a test case for a very deeply nested rules structure.

Inspecting the deepest nested element cause the DevTools UI to freeze for a long time until the rule is shown.
This is surely an edge case, though still one that should be handled gracefully.

STR:

  1. Open the attached test case
  2. Inspect the element with "text" as content and lime background

=> The Rules view takes a long time to update. (On my machine ~30 seconds.)

Of course, the best would be to improve the performance to update the Rules view faster.
Though if that's not possible, it might be worth to add a throbber to indicate to the user that the DevTools are not broken and that the pane is going to be updated eventually.

(The element highlighter is affected by that as well. Let me know if that's worth filing a separate bug.)

Sebastian

Thanks Sebastian

First, here's a profile loading the tab, without DevTools : https://share.firefox.dev/46SXX7v
There's already a 3.5s layout task

Here's one with the inspector opened, selecting the green text: https://share.firefox.dev/3K5d6J3

It looks like we're spending a very long time building the selectors

The issue you're seeing with the node picker seems to originate from the same issue.

Emilio, any idea how this could be made faster?

Flags: needinfo?(emilio)

Well, DevTools is asking for a desugared selector which is massive, so I'm not sure how to optimize it short of:

  • Not doing it
  • Putting an artificial limit on it.

Presumably we want to do the later, but the question would be what would the right limit look like.

Flags: needinfo?(emilio)

Nicolas will check if we are calling this API too many times.

Severity: -- → S3
Flags: needinfo?(nchevobbe)
Priority: -- → P2
Depends on: 1845730
Depends on: 1845731

So we were indeed calling the method too many times.
Since we were returning all the rules , we were calling StyleRule#form (and so the method to get the desugared selectors) for each of them.
Also, we weren't caching the desugared selectors, so it would be very painful to do anything in the rules view.

With patches for those 2 items, we get way better result: https://share.firefox.dev/3OxK87x (~7s instead of ~30s)
7s on my quite powerful machine is still a lot, and so we should try to find alternatives to not block the rules view that much.
(Note that computing the desugared selector starts to take more than 1s when the nesting depth is > 20 , on my machine, and then it looks exponential)

This does also brings some slowness to the parent process: https://share.firefox.dev/3Ki7fjX

  • Since we're putting the desugared selector in a data attribute (rule-editor.js), and given it's pretty long, it takes a while to set it (250ms on my machine)
  • the join that we do on the desugared selectors array (rule-editor.js) takes almost 100ms
  • the Array#includes that we do (rule-editor.js) is taking more than 250ms

Overall that's 650ms to create the markup for a single rule, which is definitely too slow. Maybe we can avoid handling/comparing the desugared selectors on the client, and only pass indexes/references, I'll keep digging.

Flags: needinfo?(nchevobbe)

For what it's worth, the Chrome DevTools go another route and display a single desugared selector using :is() pseudo-classes when hovering the & instead of building a complete markup for all selectors.
This makes the selector harder to grasp, especially with deep nesting. Though it might be a solution in case other performance optimizations don't bring the desired results.
Also, we're talking about an extreme edge case here. Presumably the average nesting isn't deeper than five levels.

If desugaring the selector takes that much time, another approach could also be to hide deeper levels behind a twisty.

Though of course the best way from a UX perspective is to keep the nested selector structure and optimize the display speed.

Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #5)

For what it's worth, the Chrome DevTools go another route and display a single desugared selector using :is() pseudo-classes when hovering the & instead of building a complete markup for all selectors.
This makes the selector harder to grasp, especially with deep nesting. Though it might be a solution in case other performance optimizations don't bring the desired results.

Yes, I saw that, and they get it wrong in some edge cases (like attribute selectors containing &)
Also, we have different needs, the desugared selectors is used for 2 different things:

  • the selector highlighter
  • whether a given selector matches the current selected element

even if deep nesting might be uncommon, we should still aim at something fast, so we should look into ways of doing things a bit differently than we do now

Depends on: 1846947
Blocks: 1875992

Here's an updated profile: https://share.firefox.dev/3IWpJpa , comment 4 is still relevant

This is done to avoid a performance issue with deeply nested rules,
where computing the desugared selector can be super expensive.
This should help overall since we're doing a bit less work, as well
as returning smaller data.

Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED

For the selector highlighter, we were retrieving the desugared selector of each
displayed rule, and using the selector text in querySelectorAll to retrieve the
elements matching the rule.
This can be very expensive, especially for deeply nested rule, for a feature that
might not even be used.
This patch is adding an InspectorUtils method which takes a root node and a Rule,
and will return the elements inside the root node that match the rule selectors.
We're only exposing the method that existed in glue.rs to get the SelectorList
of a given Rule, and call Servo_SelectorList_QueryAll with it to get our NodeList.

A test file is added to ensure this works as expected.

Instead, we use the new InspectorUtils.getElementsMatchingRuleSelectors method
to retrieve the elements to highlight.
We still compute a "selector", which is basically a concatenation of the ancestor
rules, so we can still mark the rule as highlighted when we display a different
StyleRule front for a rule that was previously highlighted.

Attachment #9398159 - Attachment description: Bug 1844446 - [devtools] Add InspectorUtils.getElementsMatchingRuleSelectors. r=#layout!,#devtools-reviewers!. → Bug 1844446 - [devtools] Add ChromeOnly CSSStyleRule::querySelectorAll. r=#layout!,#devtools-reviewers!.
Depends on: 1893923

Comment on attachment 9398159 [details]
Bug 1844446 - [devtools] Add ChromeOnly CSSStyleRule::querySelectorAll. r=#layout!,#devtools-reviewers!.

Revision D208363 was moved to bug 1893923. Setting attachment 9398159 [details] to obsolete.

Attachment #9398159 - Attachment is obsolete: true
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71da1f064211
[devtools] Send rules matching selectors indexes instead of the selector strings. r=devtools-reviewers,bomsy.
https://hg.mozilla.org/integration/autoland/rev/6973a3344601
[devtools] Don't use desugared selectors for selector highlighter. r=devtools-reviewers,bomsy.
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 128 Branch

Perfherder has detected a devtools performance change from push f0390136ebbeef553f7b365c19300ce28d4179c4.

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
91% damp custom.inspector.deeplynestedrule.refresh macosx1015-64-shippable-qr e10s fission stylo webrender 75.31 -> 6.59
91% damp custom.inspector.deeplynestedrule.refresh macosx1015-64-shippable-qr e10s fission stylo webrender-sw 78.90 -> 7.42
80% damp custom.inspector.deeplynestedrule.refresh linux1804-64-shippable-qr e10s fission stylo webrender 163.21 -> 32.11
79% damp custom.inspector.deeplynestedrule.refresh linux1804-64-shippable-qr e10s fission stylo webrender-sw 160.99 -> 34.12
76% damp custom.inspector.deeplynestedrule.refresh windows10-64-shippable-qr e10s fission stylo webrender 74.54 -> 18.05
... ... ... ... ...
2% damp custom.styleeditor.open.DAMP windows10-64-shippable-qr e10s fission stylo webrender 523.34 -> 511.01

Regressions:

Ratio Test Platform Options Absolute values (old vs new)
23% damp simple.styleeditor.open.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 135.73 -> 166.75
6% damp simple.styleeditor.close.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 3.91 -> 4.12
5% damp simple.inspector.close.DAMP windows10-64-shippable-qr e10s fission stylo webrender 6.11 -> 6.44
5% damp simple.styleeditor.close.DAMP windows10-64-shippable-qr e10s fission stylo webrender 3.83 -> 4.02
3% damp simple.inspector.reload.DAMP windows10-64-shippable-qr e10s fission stylo webrender 135.06 -> 139.70
2% damp simple.inspector.reload.DAMP windows10-64-shippable-qr e10s fission stylo webrender-sw 134.99 -> 137.89

Bug 1897717 was filed to investigate those numbers

Flags: qe-verify+

Reproduced the issue on Firefox 127.
Verified as fixed using the latest Nightly 129.0a1 and Firefox 128.0b9 on macOS 14 and Windows 11 x64.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.