1824897 - 1.43 - 1.36% Base Content JS / Base Content JS + 2 more (Linux, OSX, Windows) regression on Fri March 24 2023

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Acasandrei Beatrice (needinfo me)]

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

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

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

Comment 2

[James Teow [:jteow]]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 4

[James Teow [: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.

Comment 5

[James Teow [:jteow]]

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

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

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

Comment 7

[James Teow [:jteow]]

Attachment: Bug 1824897 - Restrict SearchSERPTelemetry Actor to load only on http and https - r?standard8

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 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47696227e38a
Restrict SearchSERPTelemetry Actor to load only on http and https - r=Standard8

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/47696227e38a

Comment 10

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

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.

Comment 11

[James Teow [:jteow]]

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

Comment 12

[Andrew McCreight [:mccr8]]

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

[Andrej Glavic (:aglavic)]

(In reply to Pulsebot from comment #8)

Pushed by [e-mail]:
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