1660047 - DAMP Perf regression in custom.inspector.manyrules.selectnode (40%)

Status: NEW

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

Description

[Julian Descottes [:jdescottes]]

As well as in custom.inspector.manycssvariables.selectnode (10%)

The documentation about devtools performance tests canbe found at https://docs.firefox-dev.tools/tests/performance-tests.html

The regression is either coming from https://bugzilla.mozilla.org/show_bug.cgi?id=1649018 or from https://bugzilla.mozilla.org/show_bug.cgi?id=1649021

I am marking Bug 1649021 as the regressing bug for now because it seems more likely.

Comment 1

[Julian Descottes [:jdescottes]]

Any idea what could make selecting a node slower with the patches from the bugs mentioned above? The regression seems much more visible on nodes which have many css rules, so we should look at something that slows down the rendering of a single rule.

Comment 2

[Julian Descottes [:jdescottes]]

Note that you can run the test and generate a profile with:

./mach talos-test --activeTests damp --subtests custom.inspector --cycles 1 --tppagecycles 1 --gecko-profile --gecko-profile-entries 100000000

Comment 3

[Julian Descottes [:jdescottes]]

This could also be from the additional computation needed to create the form of the StyleRuleActor:
https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/devtools/server/actors/styles.js#1680-1682

I will do an additional try push to check if this is from Bug 1649018 or Bug 1649021

Comment 4

[Kriyszig]

Thank you for finding this @jdescottes].

I haven't run the profile yet but afaict finding declaration issues is computation intensive process because of the cross checking against the CSS dataset and then the browser dataset to find the compatibility issues so https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/devtools/server/actors/styles.js#1680-1682 might cause slowdown.

Most of tooltip operations are async but there is a O(n) check that happens against the compatibility issues to display the tooltip icon. This check can be found here
https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/models/text-property.js#319-321
For m rules (most being compatible), the ruleview will take O(mn) time to compute the visibility of tooltip icon. (This check will not happen if the pref is disabled however but the compatibility issues will still be calculated in the StyleRuleActor. If this is happening despite the pref staying disabled, then the form creation might be the cause of the perf regression)

If it is related to the two bugs mentioned then it would be one of these that is causing the regression.

If it is the former where the form is taking a long time to be created, Patch D87613 (https://phabricator.services.mozilla.com/D87613) will indirectly address this as we look to remove the association of compatibility issues with the style rule actor and instead connect to the compatibility actor to get the data. If this is the cause, as an immediate solution, we can only perform the compatibility issue computation if the pref is enabled.

Please let us know if you find the root cause for this performance regression. I'll also look into this in the meantime.
Thank you.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Kriyszig from comment #4)

...
If it is the former where the form is taking a long time to be created, Patch D87613 (https://phabricator.services.mozilla.com/D87613) will indirectly address this as we look to remove the association of compatibility issues with the style rule actor and instead connect to the compatibility actor to get the data. If this is the cause, as an immediate solution, we can only perform the compatibility issue computation if the pref is enabled.

Thanks for all the info! After an additional push, the regression indeed comes from Bug 1649018. Great to see that this should be improved with Bug 1653520. No need to rush an intermediary fix for now. Let's block this bug on Bug 1653520 and we will check if we still have a perf issue after that.

Comment 6

[Julian Descottes [:jdescottes]]

Clearing the needinfos, I think Kriyszig's answer covers the main questions.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1649018

Comment 8

[Jens Stutte [:jstutte]]

(In reply to Julian Descottes [:jdescottes] from comment #6)

Clearing the needinfos, I think Kriyszig's answer covers the main questions.

Hi Julian, can you please set the severity according to this answer ? Thank you!

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Not sure if this is still relevant? There's no mention of the pref, so I'm not sure if the regression is still there?

Note that I worked on a patch to prevent a too big regression on the DAMP tests mentioned in Comment 0 here (Bug 1833617)