1896837 - 12.04 - 8.7% about_preferences_basic / about_preferences_basic (OSX) regression on Thu April 25 2024

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Acasandrei Beatrice (needinfo me)]

Perfherder has detected a talos performance regression from push 46e84060342cffc0c5867c198c9275511075fb65. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
12%	about_preferences_basic	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	108.69 -> 121.78
9%	about_preferences_basic	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	111.39 -> 121.08

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 193

For more information on performance sheriffing please see our FAQ.

Comment 1

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

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

Comment 2

[Hanna Jones [:hjones]]

The team is working on getting some before/after profiles locally and trying to hunt down possible causes for the regression. We've got a hunch that it could be related to custom element creation callback functions getting invoked more than once, but we're looking to get some data to back that up.

tl;dr -investigation is in progress, we'll hopefully have more for this next week (factoring in Canadian holidays).

Comment 3

[Tim Giles [:tgiles]]

Still investigating this, but I would hold off on backing this out if all possible. Due to the new mechanism for loading custom elements in the regressing patch, we could have visual regressions that go undetected since before we were calling ensureCustomElements() and the new mechanism allows developers to import the moz- custom elements without needing to write this ensureCustomElements() call in their part of the code. I'm running experiments 1 2 to see if we can prevent this regression. Hope to have better news soon.

Comment 4

[Tim Giles [:tgiles]]

Acasandrei, I'm having trouble understanding how 1803678 is causing a regression here. For example, here's a comparison of the alert commit on autoland versus a Try run reverting the regressing patch in question. If 1803678 is causing this performance regression, shouldn't we see an improvement in the about_preferences_basic test run on Try with the regressing patch backed out?

I'm not sure I'm reading this other comparison of the base alert versus reverting the patch correctly, but it seems like reverting the patch creates a low confidence improvement.

Is there anything else we can try to verify this regressor?

Comment 5

[Acasandrei Beatrice (needinfo me)]

Hi @tgiles, the graph clearly shows that your patch caused the regression.

Comparing autoland data to try data is not recommended since the builds are slightly different.
Your try run comparison is showing an improvement. Please add 15-20 more retriggers to get a higher confidence on the improvement.
You could also generate a profile of the task to investigate it using the Generate performance profile button from Treeherder's performance tab.

Comment 6

[Tim Giles [:tgiles]]

Thank you for the additional information, especially about not comparing autoland to try, didn't know about that! We've been generating performance profiles for the various experiment we've been running, but the profiles for both base and new end up being very close to one another (and not showing any immediate gains that can be made). For example. this comparison shows that we have a 15% increase in performance, yet the test is 15% slower when using the profiler. While it's understandable that there's some overhead when using the profiler, it seems odd that the profiler results are way slower than the base. Maybe it's something to do with the particular patch, but we've been trying profiles. Have you encountered something like this before?

Comment 7

[Tim Giles [:tgiles]]

Alright, here's an update on the progress so far. We've discovered a fix that gives us a 15% increase in performance for this about_preferences_basic test, seen in this perfherder comparison. Unfortunately, we can't use this fix because it causes consistent crashes.

An alternative is to remove the script tags for the moz- elements in preferences.xhtml. This gives us a ~5.4% increase in performance, but the confidence is only medium (even with 30+ runs for base and new). This doesn't place us back at pre-regression levels, but gets us closer to that point. Additionally, this doesn't cause any crashes unlike the 15% performance gain fix.

Another alternative is to only do synchronous imports after DOMContentLoaded is fired. Still waiting for the talos test to run for that particular build, but this will be the comparison link when it's done running those tests. If this fix also generates some performance benefit, we can test removing the script tags and doing synchronous imports after the DOMContentLoaded event. Hopefully this will get us closer to the 12% that we lost in the first place.

Comment 8

[Acasandrei Beatrice (needinfo me)]

Yes, we have seen that and it's the main reason why we don't gather profiles during the tests that gather the metrics (we gather the profile in a separate test run that doesn't gather metrics).

Comment 9

[Mark Striemer [:mstriemer]]

Attachment: Bug 1896837 - Only sync import widget modules after DOMContentLoaded r?#recomp-reviewers

Depends on D212190

Comment 10

[:Gijs (he/him)]

(In reply to Tim Giles [:tgiles] from comment #7)

Alright, here's an update on the progress so far. We've discovered a fix that gives us a 15% increase in performance for this about_preferences_basic test, seen in this perfherder comparison. Unfortunately, we can't use this fix because it causes consistent crashes.

Can we make sure to file these crashes as a separate bug? Frontend custom element changes shouldn't be able to cause crashes in the custom element DOM implementation. I bet the DOM folks would at least have their curiosity piqued if we told them this.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d43359dfa1d
Only sync import widget modules after DOMContentLoaded r=reusable-components-reviewers,settings-reviewers,mconley,hjones

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/1d43359dfa1d

Comment 13

[Tim Giles [:tgiles]]

(In reply to :Gijs (he/him) from comment #10)

Can we make sure to file these crashes as a separate bug? Frontend custom element changes shouldn't be able to cause crashes in the custom element DOM implementation. I bet the DOM folks would at least have their curiosity piqued if we told them this.

Just filed Bug 1900888 for this. Feel free to correct any mistakes I made in filing the bug