mozilla-mobile / android-components

⚠️ This project moved to a new repository. It is now developed and maintained at: https://github.com/mozilla-mobile/firefox-android
https://github.com/mozilla-mobile/firefox-android
Mozilla Public License 2.0
2.02k stars 472 forks source link

App links performs packageManager query on main thread on every page load #7416

Closed NotWoods closed 2 years ago

NotWoods commented 4 years ago

See:

Ideally this code should run on a background thread. This will be a problem for PWAs too, which need a DB lookup. An async alternative might be needed.

┆Issue is synchronized with this Jira Task

NotWoods commented 4 years ago

cc @mcomella

mcomella commented 4 years ago

We should measure the duration of this call on a low-end device to verify my belief that this takes a non-trivial amount of time (10-20ms?).

Ideally this code should run on a background thread.

To be explicit, this is so we're not blocking the main thread on an IO(-like) operation.

mcomella commented 4 years ago

I took two profiles on my Galaxy S5, one for clicking on a top site and and I don't see this showing up in it:

There are ~8ms spent in AppLinksInterceptor but it's in some JVM thing so it could also be a profiler error.

Before we dismiss this bug as WFM, we should add logcat debugging to measure the time for real though.

mcomella commented 4 years ago

Triage (by myself): I'll put it towards the bottom of our current backlog because it's page load related but since it isn't an obvious problem in profiles, it's probably fine.

cnevinc commented 4 years ago

The first query to PackageManager takes 14~20 ms on my Samsung S6. And it'll get faster over time(~6ms) It seems this issue is not severe on new devices. (it took less than 1 ms on Pixel) Every time the user starts a new page, it'll be called twice (session state changed loading->finish)

If we move this query to background thread, the caller will need to wait for it to complete asynchronously. The current callers are :

  1. Context Menu in a-c
  2. ToolbarMenu in Fenix.
  3. WebAppInterceptor in a-c
  4. AppRequestInterceptor used by Engine in Fenix

It'll be great if we have time to make changes to the caller site as well.

mcomella commented 4 years ago

@cnevinc Thanks for looking into this! A few questions/thoughts:

cnevinc commented 4 years ago

@cnevinc Thanks for looking into this! A few questions/thoughts:

  • Is the function called on packageManager always the same?

Well, most of the case is when the user clicks on a search result on a Google result page. There'll be some redirect (HTTP 302 for ad tracking) and thus pm query will happen multiple times(~8 times for Spotify). Another general case is when the user clicks on a bookmark/top-site, pm query will happen at least twice

Some other cases not related to startup-time nor page load speed like the user opens an app link or the user have 'open links in apps' enabled` may not be interesting.

  • In the short term, I'm most concerned about the calls that occur during page load or on startup. How is the result of the packageManager query used?

    • Does it seem reasonable to make those asynchronous or is there not enough code that's executed in the function to be worth it? (e.g. startAsync(); doWork(); blockForAsyncComplete() – is doWork long enough?)
    • Instead of making it async, can we prefetch and cache the result instead?

I like the idea that we prefetch the result. But it'll be hard since

  1. There might be multiple redirects while loading the URL. We need to try every one of them.
  2. There are too many possibilities for the URLs users may use. It's hard to prefetch them But for bookmarks, prefetch seems possible.
  3. It'll be hard to find a good timing to update the cache because the user can install new apps at any time.
cnevinc commented 4 years ago

Per the previous discussion, there are two possible solutions:

  1. Prefetch the supported (App) Links before we do the query. Check the list while page load/redirect and see if we really want to ask PackageManager.

I can't find a good way to get the list of supported links through PM. It seems there are some hidden API in PM. So the System setting can definitely get the list but not us.

  1. Queue the PM query request asynchronously, let the browser do the loading/redirect first. There's one situation here. If the page completes loading before PM query, the page will show the next web content and then start the app. Here comes the problem. EngineSession needs this information synchronously to notify it's observers. I'll need to dig in more into EngineSession's codebase to make sure this will work.

  2. Another possible solution: cache incoming URLs if the PM query return null. We don't need to query PM again if we see these URLs later. This is only helpful if we see those URLs often(e.g. Google query result page or tracking redirects).

mcomella commented 2 years ago

Triage: I was looking at this recently: on Moto G5, each of these requests take 8-30ms.

csadilek commented 2 years ago

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

Change performed by the Move to Bugzilla add-on.