Open Bug 1660047 Opened 7 months ago Updated 6 months ago

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

Categories

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

defect

Tracking

(firefox-esr68 unaffected, firefox-esr78 unaffected, firefox79 unaffected, firefox80 unaffected, firefox81 wontfix, firefox82 fix-optional)

Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox79 --- unaffected
firefox80 --- unaffected
firefox81 --- wontfix
firefox82 --- fix-optional

People

(Reporter: jdescottes, Unassigned)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

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.

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.

Flags: needinfo?(tigleym)
Flags: needinfo?(lelouch.cpp)
Flags: needinfo?(daisuke)

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

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

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.

Flags: needinfo?(lelouch.cpp)

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

Depends on: 1653520
Regressed by: 1649018
No longer regressed by: 1649021
Blocks: 1636336
Flags: needinfo?(tigleym) → needinfo?(mtigley)

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

Flags: needinfo?(mtigley)
Flags: needinfo?(daisuke)

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

Flags: needinfo?(jdescottes)
Severity: -- → S3
Flags: needinfo?(jdescottes)
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.