1.43 - 1.36% Base Content JS / Base Content JS + 2 more (Linux, OSX, Windows) regression on Fri March 24 2023
Categories
(Firefox :: Search, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr102 | --- | unaffected |
firefox111 | --- | unaffected |
firefox112 | --- | unaffected |
firefox113 | --- | wontfix |
firefox114 | --- | fixed |
People
(Reporter: bacasandrei, Assigned: jteow)
References
(Regression)
Details
(Keywords: perf, perf-alert, regression)
Attachments
(1 file)
Perfherder has detected a awsy performance regression from push 61b5bdae971a0fad048aa9039567c3a8f87c2743. 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) |
---|---|---|---|---|
1% | Base Content JS | windows11-64-2009-shippable-qr | fission | 1,638,357.33 -> 1,661,832.00 |
1% | Base Content JS | macosx1015-64-shippable-qr | fission | 1,636,629.33 -> 1,659,822.00 |
1% | Base Content JS | macosx1015-64-shippable-qr | fission | 1,636,745.33 -> 1,659,705.33 |
1% | Base Content JS | linux1804-64-shippable-qr | fission | 1,636,904.67 -> 1,659,192.00 |
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 offending 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.
For more information on performance sheriffing please see our FAQ.
Comment 1•2 years ago
|
||
Set release status flags based on info from the regressing bug 1816728
Assignee | ||
Comment 2•2 years ago
•
|
||
It's probably because I forgot to gate the new class behind a pref. I'll issue a fix.
Eventually however it will have to incur some memory cost.
Edit: Actually, it'll be the same regardless of the pref because of the existence of the class definition.
Comment 3•2 years ago
|
||
Please set a severity on this when you get a chance.
Assignee | ||
Comment 4•2 years ago
|
||
It's below the 10% threshold that would warrant a backout (S3), but we'd like to get more information on this now and look into possible optimizations (P2).
Next steps:
- Investigate on a deeper level what these tests are measuring exactly and how the new code impacts it.
- Investigate if there's anything we can do in the child that could help long term (i.e. lazy loading) or is that just kicking the can down the road (aka, we're passing the Perf tests but when the pref is enabled the user will end up incurring the cost in memory anyway)?
- Investigate ways in which we can optimize how and where SearchSERPTelemetryChild is loaded in content.
Assignee | ||
Comment 5•2 years ago
|
||
In this first perf comparison, I compare a recent patch on Central vs. a patch where I gate the instance of the new class behind a pref. There's little difference in the Base Content JS results.
In the second perf comparison, I compare that same recent patch on Central vs. a patch where I moved the new class into a separate module and lazily load it. The difference in Base Content JS results has a delta of roughly -1.21%.
The problem with the latter approach is if the feature is enabled for all users at some point we're going to have to load the class in memory so passing the test in this way might just be kicking the can down the line.
Comment 6•2 years ago
|
||
Set release status flags based on info from the regressing bug 1816728
Assignee | ||
Comment 7•2 years ago
|
||
Since telemetry tests load http pages, we need to include both http and https.
I avoided using a wildcard for the scheme since that includes WebSocket URLs.
Comment 9•2 years ago
|
||
bugherder |
Comment 10•2 years ago
|
||
The patch landed in nightly and beta is affected.
:jteow, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- If no, please set
status-firefox113
towontfix
.
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 11•2 years ago
|
||
The performance regression is fairly minimal, so I am going to opt to set Fx 113 to wontfix
.
Comment 12•2 years ago
|
||
This patch also doesn't actually fix the regression for users, only the performance test regression, so there's no need to uplift it. (It avoids the test regression because the AWSY test loads about:blank instead of a real page.)
Comment 13•2 years ago
|
||
(In reply to Pulsebot from comment #8)
Pushed by jteow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/47696227e38a
Restrict SearchSERPTelemetry Actor to load only on http and https -
r=Standard8
== Change summary for alert #38094 (as of Fri, 14 Apr 2023 17:28:18 GMT) ==
Improvements:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
5% | Base Content JS | linux1804-64-shippable-qr | fission | 1,675,810.67 -> 1,590,458.00 |
5% | Base Content JS | windows11-64-2009-shippable-qr | fission | 1,677,942.67 -> 1,592,458.67 |
5% | Base Content JS | linux1804-64-shippable-qr | fission | 1,675,750.00 -> 1,591,659.33 |
2% | Base Content Explicit | windows11-64-2009-shippable-qr | fission | 10,721,109.33 -> 10,500,437.33 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38094
Description
•