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

260-524ms between HomeActivity.load and GeckoSession.load when entered hitting enter on search #21530

Closed
mcomella opened this issue Sep 28, 2021 · 11 comments
Labels
needs:triage Issue needs triage perf:P1 must have/meet (VP prod must approve miss) performance Possible performance wins pin Issues, features, improvements that are still valid

Comments

@mcomella
Copy link
Contributor

mcomella commented Sep 28, 2021

Steps to reproduce

  • From a web page, Click url bar
  • Type "goo"
  • Hit enter to go to google.com as autocomplete suggestion

Expected behavior

Gecko is notified of page load immediately in GeckoSession.load.

Actual behavior

Gecko seems to be delayed by 260+ms because HomeActivity.load is called 260ms before GeckoSession.load: https://share.firefox.dev/3ATZEBu

In another profile to apple.com, this took 524ms: https://share.firefox.dev/3ulv15d Perhaps the time above was shorter because the code is JIT'd, the XML is cached, and stuff. And another 431ms on a Moto G5+: https://share.firefox.dev/3zM4gbk Maybe it bounces between them

I'm guessing the root cause is we call HomeActivity.load but we suspend rather than handling the request synchronously so the UI inflation happens and then we call GeckoSession.load. It's also possible we need the browser fragment to call this in the current code.

Device information

  • Android device: Moto G5
  • Fenix version: Nightly 2021-09-27

┆Issue is synchronized with this Jira Task

@mcomella mcomella added the performance Possible performance wins label Sep 28, 2021
@github-actions github-actions bot added the needs:triage Issue needs triage label Sep 28, 2021
@mcomella mcomella added this to Needs triage in Performance, front-end roadmap via automation Sep 28, 2021
@mcomella mcomella changed the title 260ms between HomeActivity.load and GeckoSession.load when entered hitting enter on search 260-524ms between HomeActivity.load and GeckoSession.load when entered hitting enter on search Sep 28, 2021
@MarcLeclair MarcLeclair added the perf:P2 should have/meet (product owner must approve miss) label Sep 29, 2021
@csadilek
Copy link
Contributor

We implemented a fix for this a while back as part of #17645, but the conclusion from the performance team was that it didn't make any actual difference in page load times, see #17645 (comment), at least not for the case where a new tab was being opened (as in the profile above).

However, we did start work on optimizations in A-C now to make sure we prevent those context-switches where we can: mozilla-mobile/android-components#11066

@csadilek
Copy link
Contributor

csadilek commented Oct 6, 2021

We've landed a related optimization though for the direct load case: mozilla-mobile/android-components#11067

For the above we need feedback from the performance team as the original conclusion was that it didn't make any difference.

@MarcLeclair MarcLeclair moved this from Needs triage to Triaged (unordered) in Performance, front-end roadmap Oct 6, 2021
@MarcLeclair MarcLeclair moved this from Triaged (unordered) to Needs triage in Performance, front-end roadmap Oct 6, 2021
@MarcLeclair MarcLeclair removed the perf:P2 should have/meet (product owner must approve miss) label Oct 6, 2021
@mcomella
Copy link
Contributor Author

mcomella commented Oct 6, 2021

Triage: this is an easy fix but we don't know if it'd actually improve perf given our previous analyses. We should measure the difference before landing the change.

@mcomella
Copy link
Contributor Author

mcomella commented Oct 8, 2021

Summary of the issue

When we load a page from the search screen, there are several events that need to happen. In some cases, they are posted to the UI thread, forcing them to wait on the UI transition (which is 260-524ms, measured haphazardly) before executing. Ideally, they'd be executed synchronously before the UI transition. The events are:

  • HomeActivity.load
  • GeckoSession.load (gets posted b/c we often suspend to send the load through our architecture)
  • GV's onLoadRequest (note: this only happens in the slow case. it's posted when we redirect b/c redirects go to the Gecko thread which posts to the UI thread BZ-1734916)

Recent optimization

We've landed a related optimization though for the direct load case: mozilla-mobile/android-components#11067

afaict, this didn't improve performance in most cases due to this third event – it's a platform bug where page loads with redirects are posted to the UI thread so block on the UI transition. Since most urls from the search screen redirect to https, this affects most URLs entered on the search screen. That's https://bugzilla.mozilla.org/show_bug.cgi?id=1734916

However, once that issue is resolved, I expect to see the performance benefits.

Additional optimizations

Here's a list to keep track of the different areas where we need to align HomeActivity.load with GeckoSession.load:

  • User enters or autocompletes to a URL (e.g. apple.com)
    • Wrote test or not needed
  • User enters a search (has this regressed like ^ ?)
    • Wrote test or not needed
  • User clicks an autocomplete search result (probably the same case as above). Profile where I sleep 7s in UI to demonstrate issue: https://share.firefox.dev/3uUR1Eq
    • Wrote test or not needed
  • User needs to create a new engine session (launching a new tab. Are there other cases?) This is probably why loadUrl isn't called for ~1s after IntentReceiverActivity in WARM VIEW #17645 (comment) wasn't fixed
    • Wrote test or not needed
  • ? Custom tabs, other contexts I'm not thinking about
    • Wrote test or not needed

@MarcLeclair MarcLeclair added the perf:P2 should have/meet (product owner must approve miss) label Oct 13, 2021
@MarcLeclair MarcLeclair moved this from Needs triage to Triaged (unordered) in Performance, front-end roadmap Oct 13, 2021
@mcomella mcomella added perf:P1 must have/meet (VP prod must approve miss) and removed perf:P2 should have/meet (product owner must approve miss) labels Oct 25, 2021
@mcomella mcomella moved this from Triaged (unordered) to Needs triage in Performance, front-end roadmap Oct 25, 2021
@mcomella
Copy link
Contributor Author

The investigation that created this bug is qf:p1 so I'd like to carry it forward (I think we're dropping P* labels so moving to triage column to).

@mcomella mcomella moved this from Needs triage to Perf P1 (unordered) in Performance, front-end roadmap Nov 3, 2021
@mcomella mcomella added this to Security and Performance (top 10 issues) in Android Team Backlog Staging Board Nov 3, 2021
@mcomella
Copy link
Contributor Author

mcomella commented Nov 3, 2021

Triage: this is a very big delay to page load time so we want to address it: P1.

The remaining action items are to go through the checklist in #21530 (comment) and, for each use case, verify if the delay exists (i.e. the GeckoSession.load call is blocked on the UI transition from HomeActivity.load or equivalent) and, if it exists, fix it.

@mcomella
Copy link
Contributor Author

mcomella commented Dec 20, 2021

User enters or autocompletes to a URL (e.g. apple.com)

This use case regressed: https://share.firefox.dev/3qdBFJd These fixes are fragile so I think we have to verify we have tests for each of them.

edit: see below.

@mcomella
Copy link
Contributor Author

Nevermind. I accidentally tested the "needs new engine session case" we haven't optimized yet. Here's a profile when we don't need a new engine session that shows this did not regress: https://share.firefox.dev/3yIguCM

@mcomella
Copy link
Contributor Author

I measured an 800ms improvement on a Moto G5 when we address the GV issue so it is very much worth continuing to optimize this in fenix.

@stale
Copy link

stale bot commented Nov 16, 2022

See: #17373 This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 16, 2022
@MarcLeclair MarcLeclair added the pin Issues, features, improvements that are still valid label Nov 16, 2022
@stale stale bot removed the wontfix label Nov 16, 2022
@cpeterso
Copy link

Moved to bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1807338

Change performed by the Move to Bugzilla add-on.

Performance, front-end roadmap automation moved this from Perf P1 (unordered) to Done Dec 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs:triage Issue needs triage perf:P1 must have/meet (VP prod must approve miss) performance Possible performance wins pin Issues, features, improvements that are still valid
Projects
Android Team Backlog Staging Board
Security and Performance (top 10 issues)
Development

No branches or pull requests

4 participants