Closed Bug 1824897 Opened 2 years ago Closed 2 years ago

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)

defect

Tracking

()

RESOLVED FIXED
114 Branch
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.

Flags: needinfo?(jteow)

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

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.

Flags: needinfo?(jteow)

Please set a severity on this when you get a chance.

Assignee: nobody → jteow
Flags: needinfo?(jteow)

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.
Severity: -- → S3
Flags: needinfo?(jteow)
Priority: -- → P2

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.

See Also: → 1827167

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

See Also: → 1827536

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.

Blocks: 1827624
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 114 Branch

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 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(jteow)

The performance regression is fairly minimal, so I am going to opt to set Fx 113 to wontfix.

Flags: needinfo?(jteow)

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

(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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: