Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

~100ms regression in cold main time to first frame (1/28; Nimbus FML?) #23492

Closed
mcomella opened this issue Jan 31, 2022 · 5 comments · Fixed by #23556
Closed

~100ms regression in cold main time to first frame (1/28; Nimbus FML?) #23492

mcomella opened this issue Jan 31, 2022 · 5 comments · Fixed by #23556
Assignees
Labels
needs:triage Issue needs triage performance Possible performance wins

Comments

@mcomella
Copy link
Contributor

mcomella commented Jan 31, 2022

Caught in our nightly backfill:

image

We go from 1.468s to 1.570s (102ms).

┆Issue is synchronized with this Jira Task

@mcomella mcomella added the performance Possible performance wins label Jan 31, 2022
@mcomella mcomella added this to Needs triage in Performance, front-end roadmap via automation Jan 31, 2022
@github-actions github-actions bot added the needs:triage Issue needs triage label Jan 31, 2022
@mcomella
Copy link
Contributor Author

Bisection:

Possible regressing commits:

$ git log bfa8d85bd..82a6f8cae --pretty=oneline 
82a6f8cae411603ba5abced3b13fc43ca40f335e First use of Nimbus FML plugin (#23400)
e92fe26df74f22c530186d52cf66439b94ce99c3 Import l10n.
15d8ca86d94c7736ace16226f01148afcce3e5df For #23448 - Update the address bar color to use @color/fx_mobile_layer_color_3
6596f4c28b619a42b27e50443636d02ea5e7c96e For #23432 - Use the imageUrl as the favicon for a provided top site

@mcomella
Copy link
Contributor Author

Continued bisection: the regression appears to be from 82a6f8c #23400.

  • e92fe26 (i.e. commit before below) 1373ms local build
  • 82a6f8c 1501ms local build

@mcomella mcomella changed the title ~100ms regression in cold main time to first frame (1/28) ~100ms regression in cold main time to first frame (1/28; Nimbus FML?) Jan 31, 2022
@mcomella
Copy link
Contributor Author

@jhugman Do you know what may have caused this start up regression in #23400?

@jhugman
Copy link
Contributor

jhugman commented Feb 3, 2022

Eyeballing the code, this looks like a re-organization of the code to look up the default browser.

BrowserCache.all(context).isFirefoxDefaultBrowser

causes a lookup of the packageManager, which we've seen causing perf problems before.

        val resolveInfo = packageManager.resolveActivity(intent, PackageManager.MATCH_DEFAULT_ONLY)
            ?: return null

The short term fix is to re-order the code to what it was before. The long term fix is to ensure that isFirefoxDefaultBrowser isn't called from the UI thread.

I'll put up a PR shortly.

@mcomella
Copy link
Contributor Author

mcomella commented Feb 3, 2022

I'm sorry – I should have provided profiles. Here is with the Firefox Profiler (starts around when Gecko starts in start up, low overhead):

Here is with the Debug API (starts earlier during start up but has non-trivial overhead):

I don't see anything obvious.

jhugman added a commit that referenced this issue Feb 7, 2022
jhugman added a commit that referenced this issue Feb 8, 2022
jhugman added a commit that referenced this issue Feb 8, 2022
@mergify mergify bot closed this as completed in #23556 Feb 8, 2022
Performance, front-end roadmap automation moved this from Needs triage to Done Feb 8, 2022
mergify bot pushed a commit that referenced this issue Feb 8, 2022
…hread (#23556)

* Fixes #23492 — Fixup perf regression of calling isFirefoxDefault from the main thread

* Tightening of near defunct default browser message

* Fixup early crash in debug build

* ktlint
pkirakosyan pushed a commit to gexsi/user-agent-android that referenced this issue Mar 10, 2022
…lt from main thread (mozilla-mobile#23556)

* Fixes mozilla-mobile#23492 — Fixup perf regression of calling isFirefoxDefault from the main thread

* Tightening of near defunct default browser message

* Fixup early crash in debug build

* ktlint
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs:triage Issue needs triage performance Possible performance wins
Development

Successfully merging a pull request may close this issue.

2 participants