1916240 - 23.85 - 2.47% bing-search fcp / bing-search SpeedIndex + 16 more (Linux, OSX, Windows) regression on Fri August 23 2024

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Treeherder Bug Filer]

Perfherder has detected a browsertime performance regression from push f52d393424ddb96ea5423dee5830ad944288a1d6. 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)	Performance Profiles
24%	bing-search fcp	linux1804-64-shippable-qr	fission warm webrender	79.44 -> 98.39	Before/After
21%	bing-search FirstVisualChange	linux1804-64-shippable-qr	fission warm webrender	100.03 -> 120.67	Before/After
19%	bing-search FirstVisualChange	linux1804-64-shippable-qr	fission warm webrender	103.41 -> 122.96	Before/After
11%	google-search largestContentfulPaint	linux1804-64-shippable-qr	fission warm webrender	61.75 -> 68.81
7%	bing-search fcp	windows11-64-shippable-qr	fission warm webrender	47.15 -> 50.48
7%	bing-search fcp	windows11-64-shippable-qr	bytecode-cached fission warm webrender	49.13 -> 52.57	Before/After
7%	google-search largestContentfulPaint	windows11-64-shippable-qr	fission warm webrender	49.88 -> 53.20	Before/After
6%	bing-search largestContentfulPaint	linux1804-64-shippable-qr	fission warm webrender	103.47 -> 109.51	Before/After
6%	bing-search largestContentfulPaint	windows11-64-shippable-qr	fission warm webrender	59.01 -> 62.37	Before/After
5%	bing-search loadtime	windows11-64-shippable-qr	fission warm webrender	61.98 -> 65.35	Before/After
...	...	...	...	...	...
5%	bing-search loadtime	windows11-64-shippable-qr	bytecode-cached fission warm webrender	65.70 -> 68.67	Before/After
4%	bing-search loadtime	macosx1015-64-shippable-qr	fission warm webrender	75.15 -> 78.51
4%	bing-search loadtime	macosx1015-64-shippable-qr	bytecode-cached fission warm webrender	78.34 -> 81.82
4%	bing-search PerceptualSpeedIndex	linux1804-64-shippable-qr	fission warm webrender	354.63 -> 368.29	Before/After
2%	bing-search SpeedIndex	linux1804-64-shippable-qr	fission warm webrender	364.33 -> 373.34	Before/After

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 1818

For more information on performance sheriffing please see our FAQ.

Comment 1

[Rob Wu [:robwu]]

I think that the detected change is the opposite of the "improvement" from https://bugzilla.mozilla.org/show_bug.cgi?id=1885002#c5

The reduced responsiveness of the search pages is not surprising, considering that the built-in extension blocks network requests until the extension has responded: https://searchfox.org/mozilla-central/rev/55f2ada1564baaeebd69d277b38737961a3ca5f3/browser/extensions/search-detection/extension/background.js#71

The extension is observational only. It should not be necessary to block the request. The justification for that listener is at https://github.com/mozilla-extensions/addons-search-detection/pull/1#issuecomment-875908533 :

onBeforeRedirect cannot be blocking so I guess that's why it'd work when a block onBeforeRequest listener is added? It's just a somewhat educated guess, I don't really know what's going on in WebRequest.jsm.

Registering a no-op listener for onBeforeRequest seems to work as intended... Not sure if that's what we want, though.

Given this reasoning, I believe that we can drop "blocking". The reason that the listener is needed at all is at bug 1799118: ChannelWrapper cannot trace redirects until a webRequest listener is present. The concrete part responsible for activating the redirect tracking is the registerTraceableChannel call at https://searchfox.org/mozilla-central/rev/55f2ada1564baaeebd69d277b38737961a3ca5f3/toolkit/components/extensions/parent/ext-webRequest.js#33. I'll put up a patch for review.

Comment 2

[Mark Banner (:standard8)]

(In reply to Rob Wu [:robwu] from comment #1)

I think that the detected change is the opposite of the "improvement" from https://bugzilla.mozilla.org/show_bug.cgi?id=1885002#c5

I was thinking the same.

Given this reasoning, I believe that we can drop "blocking". The reason that the listener is needed at all is at bug 1799118: ChannelWrapper cannot trace redirects until a webRequest listener is present. The concrete part responsible for activating the redirect tracking is the registerTraceableChannel call at https://searchfox.org/mozilla-central/rev/55f2ada1564baaeebd69d277b38737961a3ca5f3/toolkit/components/extensions/parent/ext-webRequest.js#33. I'll put up a patch for review.

Thank you for the analysis. Good to know we should be able to keep the improvement.

Comment 3

[Rob Wu [:robwu]]

(removing mention of bug 1799118 because it is not relevant: the WebRequest.onRedirected listener is enough for redirect tracking purposes.)

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1916240 - Drop no-op webRequest listener

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df22aff84e9a
Drop no-op webRequest listener r=willdurand

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/df22aff84e9a

Comment 8

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

The patch landed in nightly and beta is affected.
:robwu, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox131 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[Rob Wu [:robwu]]

The patch is low risk and a performance improvement is always nice. I'll request an uplift.

Comment 10

[Rob Wu [:robwu]]

Attachment: Bug 1916240 - Drop no-op webRequest listener

Original Revision: https://phabricator.services.mozilla.com/D220835

Comment 11

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Mild performance regression in load times when the user browses websites that are registered as default/built-in search engines
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: Issue well understood based on profiler analysis. Replaces slow code with equivalent code in another place; relevant unit tests still pass.
• String changes made/needed: None
• Is Android affected?: no

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/b2f8e51c45aa