1844446 - Complex nested rules cause performance issues

Status: VERIFIED FIXED

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

Description

[Sebastian Zartner [:sebo]]

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

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

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?

Comment 2

[Emilio Cobos Álvarez (: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.

Comment 3

[Julian Descottes [:jdescottes]]

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 5

[Sebastian Zartner [:sebo]]

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

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

(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

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1844446 - [devtools] Send rules matching selectors indexes instead of the selector strings. r=#devtools-reviewers.

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.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1844446 - [devtools] Add ChromeOnly CSSStyleRule::querySelectorAll. r=#layout!,#devtools-reviewers!.

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.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1844446 - [devtools] Don't use desugared selectors for selector highlighter. r=#devtools-reviewers.

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.

Comment 11

[Phabricator Automation]

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

DAMP does report nice improvements: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=ad67089d0af7b26881509421e497700960b3e305&newProject=try&newRevision=cff78ee58fc6b71243f27ef7cb11f7dfea68e797&page=1&framework=12

Comment 14

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/71da1f064211
https://hg.mozilla.org/mozilla-central/rev/6973a3344601

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 17

[Ada C.]

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.