1831833 - 14.15 - 6.99% bing fcp / bing ContentfulSpeedIndex + 3 more (Android) regression on Tue May 2 2023

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, defect, P1)

Description

[Alex Finder]

Perfherder has detected a browsertime performance regression from push ca8b14caf6218ea583414dc768e186af73591886. 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)
14%	bing fcp	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	376.90 -> 430.21
13%	bing loadtime	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	404.71 -> 457.38
11%	bing FirstVisualChange	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	487.12 -> 541.08
10%	bing ContentfulSpeedIndex	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	760.08 -> 837.92
7%	bing ContentfulSpeedIndex	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	780.38 -> 834.92

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

[Issam Mani [:issammani]]

This performance regression is most likely caused by using static imports directly instead of lazy loading modules. I will open a patch for this. Please note that this is a patch that is required for a feature on iOS so backing this out might break things.

Comment 2

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

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

Comment 3

[Issam Mani [:issammani]]

Attachment: Bug 1831833 - Import modules lazily where possible. r=#credential-management-reviewers!

Comment 4

[Issam Mani [:issammani]]

:afinder just ran a performance test for bing on the new patch and compared results with an old patch. Performance seems to be up. Can you double check just to make sure ?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0494eb72005b
Import modules lazily where possible. r=credential-management-reviewers,dimi

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0494eb72005b

Comment 7

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

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

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

For more information, please visit BugBot documentation.

Comment 8

[Issam Mani [:issammani]]

Attachment: Bug 1831833 - Import modules lazily where possible.

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

Comment 9

[Issam Mani [:issammani]]

I think we can uplift this to beta since it fixes a performance regression. I submitted an uplift request to beta.

Comment 10

[Issam Mani [:issammani]]

Attachment: Bug 1831833 - Import modules lazily where possible.

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

Comment 11

[Pascal Chevrel:pascalc]

Issam, when requestion an uplift to beta, there is a form to fill in with specific information, could you re-request and fill it in? Thanks

Comment 12

[Phabricator Automation]

Uplift Approval Request

• Needs manual QE test: no
• String changes made/needed: No
• Risk associated with taking this patch: None
• Steps to reproduce for manual QE testing: Compare perf test using perherder. https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=455b0cef82bb0c8744c7eafa04f0f9b9d20a7a08&newProject=try&newRevision=e3fac8742e9657e6e18135a7ab30713c7984f9d1&framework=13&page=1&showOnlyComparable=1&showOnlyImportant=1
• User impact if declined: Potentionally longer load times on android.
• Is Android affected?: yes
• Explanation of risk level: This fixes a performance regression on android.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes

Comment 13

[Issam Mani [:issammani]]

Hey Pascal, that's weird I remember filling it out before. In any case, I filled it out again. Let me know if any other steps are required from my end.

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9334139 [details]
Bug 1831833 - Import modules lazily where possible.

https://hg.mozilla.org/releases/mozilla-beta/rev/59efee44047ab7ec777916f9f2aa533eb33a25fe

Comment 15

[Andrej (:aglavic)]

(In reply to Issam Mani [:issammani] from comment #9)

I think we can uplift this to beta since it fixes a performance regression. I submitted an uplift request to beta.

Just writing to confirm that it did indeed resolve a performance regression :)
== Change summary for alert #38359 (as of Tue, 16 May 2023 22:05:26 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
12%	bing FirstVisualChange	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	540.29 -> 474.75
11%	bing fcp	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	434.52 -> 385.00
11%	bing loadtime	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	464.04 -> 415.25
8%	bing ContentfulSpeedIndex	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	835.42 -> 770.58
7%	bing ContentfulSpeedIndex (geomean)	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	850.05 -> 787.78

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38359