1647267 - 21.07 - 29.38% startup_about_home_paint / startup_about_home_paint_realworld_webextensions (linux64-shippable, windows10-64-shippable|-qr, windows7-32-shippable) regression on push 9e61cb1a6d0f31634a9abf0e5507dd804f4efe11 (Thu June 18 2020)

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Perfherder has detected a talos performance regression from push 9e61cb1a6d0f31634a9abf0e5507dd804f4efe11. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

29% startup_about_home_paint_realworld_webextensions windows10-64-shippable-qr opt e10s stylo 632.33 -> 818.08
29% startup_about_home_paint windows7-32-shippable opt e10s stylo 606.75 -> 782.17
29% startup_about_home_paint windows10-64-shippable-qr opt e10s stylo 609.08 -> 784.33
27% startup_about_home_paint_realworld_webextensions windows7-32-shippable opt e10s stylo 637.17 -> 811.25
27% startup_about_home_paint windows10-64-shippable opt e10s stylo 631.77 -> 800.50
25% startup_about_home_paint_realworld_webextensions windows7-32-shippable opt e10s stylo 642.42 -> 803.08
24% startup_about_home_paint linux64-shippable opt e10s stylo 720.42 -> 894.25
24% startup_about_home_paint_realworld_webextensions linux64-shippable opt e10s stylo 751.00 -> 927.67
21% startup_about_home_paint_realworld_webextensions windows10-64-shippable opt e10s stylo 662.58 -> 802.17

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) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[Dale Harvey (:daleharvey)]

Suspect this is due to https://bugzilla.mozilla.org/show_bug.cgi?id=1646650 and is fixed, will do test runs to verify

Comment 2

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

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

Comment 3

[Mike Conley (:mconley) (:⚙️)]

It looks like the regression still persists even with bug 1646650 closed.

Hey Dale, I notice that a few functions in your patch now await for some things... I wonder if the rendering of about:home is being stalled because we're now waiting for the Search Service to boot more, whereas before we'd continue on rather than awaiting?

Comment 4

[Dale Harvey (:daleharvey)]

We make a few extra calls into the addonManager after this, we arent awaiting on anything extra that we werent before, but we are doing more things.

Shane, we have added 2 calls to AddonManager.getAddonByID, both are used within the startup path, in particularly when the AddonManager itself calls addEnginesFromExtension during the addon startup we have to call back to find out whether it is "AppProvided" (we want to know if it was either shipped in omni.ja of via normandy)

https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm#2343
https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm#2585

Is this potentially a slow thing to be calling and is there any faster way to do it? (would rather best testing those properties, but in a pinch endsWith("@search.mozilla.com") may work)

Comment 5

[Shane Caraveo (:mixedpuppy)]

You could avoid that call/wait by using what you should already have.

let {addonData} = extension;
let appProvided = addonData.builtIn || addonData.signedState === AddonManager.SIGNEDSTATE_SYSTEM;

It is a little different...addon.isSystem tells you the addon is installed into a system location, whereas the above tells you it is a signed system addon. For all practical purposes, I don't think it matters. Worst case scenario is a user installs one of our system-signed addons, that's a pretty big edge case.

see https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/toolkit/components/extensions/Extension.jsm#2018-2019

Comment 6

[Dale Harvey (:daleharvey)]

Attachment: Bug 1647267 - Avoid calling getAddonByID during startup

Comment 7

[Dale Harvey (:daleharvey)]

Thanks Shane

Pushed to try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c5fed6fe5ba92a87eaf5aae4b3de82830f55862 and while I havent spent a long time looking at talos graphs, it looks like that has fixed the issue.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d491fbcccc72
Avoid calling getAddonByID during startup r=mixedpuppy

Comment 9

[Noemi Erli[:noemi_erli]]

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