1367029 - Telemetry takes 93ms in isDefaultBrowser on startup on reference hardware

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Georg Fritzsche [:gfritzsche]]

Per Florian on IRC:
> here is a cold startup profile on the quantum reference hardware:
> https://perf-html.io/public/37eba6ac7b53f5d9ba25ba2a7e5755c72c2ea089/calltree/?implementation=js&range=0.0000_14.9170~0.0000_6.8354&search=Telemetry&thread=0 
> Telemetry takes ~180ms before first paint
> with most of the time spent in isDefaultBrowser and getGfxField

Comment 1

[Georg Fritzsche [:gfritzsche]]

We already have bug 1358927 on the slow gfx init, so lets make this one about spending ~90ms in isDefaultBrowser.

We have different code paths for this depending on the platform:
Mac: https://dxr.mozilla.org/mozilla-central/rev/34ac1a5d6576d6775491c8a882710a1520551da6/browser/components/shell/nsMacShellService.cpp#38
Windows: https://dxr.mozilla.org/mozilla-central/rev/34ac1a5d6576d6775491c8a882710a1520551da6/browser/components/shell/nsWindowsShellService.cpp#290

In the raw profile data i see this is on Windows:
> "platform":"Windows","oscpu":"Windows NT 10.0; WOW64"

The Windows implementation doesn't do much, presumably this is slow from hitting:
> ... CoCreateInstance(CLSID_ApplicationAssociationRegistration,
> ...

We should probably just delay this 1) until after first paint or 2) until the delayed telemetry init 60sec after startup.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Apparently we already hit isDefaultBrowser from "sessionstore-windows-restored" elsewhere:
https://dxr.mozilla.org/mozilla-central/rev/34ac1a5d6576d6775491c8a882710a1520551da6/browser/components/nsBrowserGlue.js#1132

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

Gathering this data point takes up to 100ms on reference hardware and might impact
startup performance.

Review commit: https://reviewboard.mozilla.org/r/148976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148976/

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

https://reviewboard.mozilla.org/r/148976/#review153844

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:785
(Diff revision 1)
> +  // To guard against slowing down startup, defer gatherings heavy environment
> +  // entries after the session is restored.

Nit: I think that should be: "... defer gathering ... until the session is restored."

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1255
(Diff revision 1)
>        return null;
>      }
>    },
>  
> +  _updateDefaultBrowser() {
> +    if (AppConstants.platform === "android" || !this._sessionWasRestored) {

Do we need a special flag to track this?
I don't that scales well.

Can we just:
- not call this in the initial `EnvironmentCache` init
- update it once from the session restored topic

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:1572
(Diff revision 1)
> +  // Please note that this test must live after the default search engine one,
> +  // otherwise the other test will fail.

Or we make it possible to do proper test resets for the environment data.
Then we can presumably avoid these test depedencies.

Lets file a follow-up on this?

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148976/diff/1-2/

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

https://reviewboard.mozilla.org/r/148976/#review153844

> Do we need a special flag to track this?
> I don't that scales well.
> 
> Can we just:
> - not call this in the initial `EnvironmentCache` init
> - update it once from the session restored topic

Good point. I was afraid pref flips & other stuff could happen right after cache init but before the session-restore, but that seems unlikely and didn't really happen in a few manual QA runs.

> Or we make it possible to do proper test resets for the environment data.
> Then we can presumably avoid these test depedencies.
> 
> Lets file a follow-up on this?

Hah! You know, while I was filing this bug I found that we already had that :-D I've changed the test accordingly.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

https://reviewboard.mozilla.org/r/148976/#review153874

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1264
(Diff revision 2)
> +  },
> +
>    /**
>     * Update the cached settings data.
>     */
> -  _updateSettings() {
> +  _updateSettings(initialCall=false) {

This can also be called from `_onPrefChanged()`, which could happen earlier.
I'm not sure about scenarios where addons or the distribution code might flip prefs here.

Could we just simplify this to call `_updateDefaultBrowser()` once per session?
It doesn't look like we systematically update this currently whenever it changes anyway.

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

https://reviewboard.mozilla.org/r/148976/#review153874

> This can also be called from `_onPrefChanged()`, which could happen earlier.
> I'm not sure about scenarios where addons or the distribution code might flip prefs here.
> 
> Could we just simplify this to call `_updateDefaultBrowser()` once per session?
> It doesn't look like we systematically update this currently whenever it changes anyway.

Good point, I've changed this to only be called once after the session is restored.

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148976/diff/2-3/

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

https://reviewboard.mozilla.org/r/148976/#review153892

::: commit-message-a8f8e:1
(Diff revision 3)
> +Bug 1367029 - Defer gathering isDefaultBrowser after session restore completes. r?gfritzsche

Nit: "until session restore completes"

::: toolkit/components/telemetry/docs/data/environment.rst:39
(Diff revision 3)
>          hotfixVersion: <string>, // e.g. "20141211.01"
>        },
>        settings: {
>          addonCompatibilityCheckEnabled: <bool>, // Whether application compatibility is respected for add-ons
>          blocklistEnabled: <bool>, // true on failure
> -        isDefaultBrowser: <bool>, // null on failure, not available on Android
> +        isDefaultBrowser: <bool>, // null on failure, not available on Android and before session restore completes

Lets make this `null` instead of not available to avoid downstream changes.

Can we call the possible data change (only updated once per session from Firefox 56 on) out below in the detailed docs?

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148976/diff/3-4/

Comment 12

[Alessio Placitelli [:Dexter]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

https://reviewboard.mozilla.org/r/148976/#review153892

> Lets make this `null` instead of not available to avoid downstream changes.
> 
> Can we call the possible data change (only updated once per session from Firefox 56 on) out below in the detailed docs?

I've reverted back to the previous message using the boolen flag to guard the updates: unfortunately, since updateSettings is always overwriting that section of the environment, getting around that is tricky and produces some test fallout. Let's defer that to the upcoming refactoring in bug 1372845.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

https://reviewboard.mozilla.org/r/148976/#review153988

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:1584
(Diff revision 4)
> +  Assert.ok("isDefaultBrowser" in environmentData.settings,
> +            "isDefaultBrowser must be available after the session is restored.");

We should check that this has a reasonable value (non-null, right type).

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148976/diff/4-5/

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8877523 [details]
Bug 1367029 - Defer gathering isDefaultBrowser until session restore completes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148976/diff/5-6/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adb51b322ca8
Defer gathering isDefaultBrowser until session restore completes. r=gfritzsche

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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