1822447 - Every time you open the tab tray the thumbnails finish painting much later

Status: VERIFIED FIXED

(Firefox for Android :: Tabs, enhancement, P2)

Description

[Joe M [:jmahon]]

Every time you open the tab tray the thumbnails reload - even if nothing has changed. Sometimes Thumbnails never appear.

• JM note - just some ideas to kick-start the process of solving this: is there a way to cache the thumbnails temporarily to optimize that reload time every time the tabs tray is recreated? I have a hunch that recreating that screen always triggers an asynchronous queue of image requests to get the latest thumbnail; if that's the case, perhaps those responses would be faster if there was an in-memory cache to pull from? Or perhaps the tabs TabsAdapter or TrayPagerAdapter should be tied to the lifecycle of the window instead of being recreated every time the tray emerges? (I acknowledge that would necessitate some extra reference management precautions to prevent memory leaks, not sure if that's viable).

Originally taken from this doc.

Comment 1

[Joe M [:jmahon]]

Attachment: Android-Tabtray-Thumbnails2.mp4

Comment 2

[[:harrisono]]

Need some clarification on this ticket.

It’s mentioned that a potential solution can be implementing a memory caching system. We already cache the thumbnails into disk storage but we recently removed https://bugzilla.mozilla.org/show_bug.cgi?id=1795105 tab thumbnails from memory as it was causing issues with performance. Tab thumbnails are now loaded from disk storage.

I don’t think we can paint the thumbnails to screen any faster either.

This ticket feels like a duplicate/similar ticket to this one: https://bugzilla.mozilla.org/show_bug.cgi?id=1812912

Comment 3

[[:harrisono]]

Attachment: Screen Recording of tabs tray with placeholder thumbnail removed

Screen Recording of tabs tray with placeholder thumbnail removed

https://drive.google.com/file/d/1NNa_OROxRwMTLFZyDTkzUy0MTtYtTCJ_/view?usp=share_link

Comment 4

[Jonathan Almeida [:jonalmeida]]

Since Noah mentioned offline that these were originally requests from UX, let's redirect the NI to the original requester for clarification.

Comment 5

[[:harrisono]]

https://drive.google.com/file/d/1txwZ0uZC5SRoT68JkCkRI2L19HsGqFqG/view?usp=share_link
Link to build with removed placeholder image on Samsung A51 opening and closing the tabs tray

https://drive.google.com/file/d/1O7dRGMC5vb05Pv3uK9lhM5-bMd07JLbE/view?usp=share_link
Link to Firefox Prod on Samsung A51 opening and closing the tabs tray

Comment 6

[Verdi [:verdi] Best to slack me]

The removal of the placeholder makes things better but the goal here is for the thumbnails to not reload at all. I believe that's the only way to remove the visual flicker and it's the behavior in other browsers.

Comment 7

[Jonathan Almeida [:jonalmeida]]

Updating the title of the issue to make it a bit clearer on what the problem is that we're trying to solve: after the tabs tray is opened, the thumbnails are painted to the screen much later.

"Reload" is a bit harder to decipher the meaning and seems like the incorrect term to use here.

If the above is accurate, then as Harrison mentioned above, this is indeed a duplicate of bug 1812912, but I'll close that one as a duplicate to avoid splitting the conversation.

Comment 9

[Jonathan Almeida [:jonalmeida]]

Harrison, since we know what we're trying to solve now. It might be useful to take a profile of the tabs tray and see what is happening when we open the tabs tray. We can make a more informed decision on how to solve this bug to make the perceived perf better.

What do you think?

Happy to help with setting up the profiler if you haven't used it before.

Comment 10

[[:harrisono]]

I have been doing some further investigation Into this issue post-removing placeholder image.

1. Reducing the WEBP_QUALITY to 45 did not reduce the painting time.
2. Changing MAXIMUM_SCALE_FACTOR upwards did not reduce the painting time.
3. Reducing the maximumSize in the ThumbnailStorage.kt did not reduce the painting time.

I also checked the other browsers. It seems to me that the “Tabs Tray” for Chrome and Brave are both in a separate activity/fragment that stays in memory behind the browser activity/fragment so there is no new painting that occurs except for new tabs.
I’ve attached screen recordings of the other browsers.
I have a feeling that this might be the only way to “fix” the tab thumbnails loading is to refactor the TabsTrayFragment from a DialogFragment to a regular Fragment (or AppCompat version) that can load the thumbnails before being shown/made visible for users. I think the DialogFragment’s lifecycle is to blame for the re-painting each time the Tabs Tray is opened, even if nothing has changed. This seems like a drastic change though and I'm not sure if it's worth the change, especially if it comes at the expense of performance for low-end devices.
I'll note that the loading time for the tabs tray thumbnails in the app currently is longer on low-end devices and not as noticeable on flagships.

Chrome tabs Screen recording:
https://drive.google.com/file/d/1AZlcyr-G77tBRxOWM73xvmBnfQrUW3xw/view?usp=share_link

Brave tabs Screen recording:
https://drive.google.com/file/d/1rA0CGBbeqaHgb-4dTrClKsij8nyWoFuv/view?usp=share_link

Comment 11

[[:harrisono]]

First, I want to give a shout out to Jon on the Foundation team for introducing me to the Firefox Profiler web app that gives greater insights to the profile trace files that are captured in Android Studio than the standard Profiler tool in Android Studio. This tool is available on Firefox at profiler.firefox.com

Using the Firefox Profiler, we started by taking a look at the processes that happen when a user opens and closes the tabs tray.
In this first screenshot (InitialProfilerScreenshot.png), we see the overall trace for test user opening and closing the tab tray 3 times. We can see that there are 3 threads for “ThumbnailStorage” with some processes occurring at the beginning and end of each of the three tab openings. We will focus on the 2nd tab opening occurrence.


(Screenshot1.png) Here we see the initial call for ThumbnailLoader.loadIntoView.
(Screenshot2.png) Here we see the calls for ThumbnailLoader.loadIntoViewInternal where the thumbnail bitmap is actually set to the view.


In between these two series of events, there is a large number of Compose-related processes that are taking up the time between loadIntoView and loadIntoViewInternal on the main thread. These Compose processes are interrupting the loading of the thumbnail into the adapter items in the tabs tray, causing the delay that users experience.
We tested this hypothesis by removing the InactiveTabsAdapter from the Tabs Tray and saw a big reduction in the loading time.



https://drive.google.com/drive/folders/10SITN8vsAiBsMnwjxjODDxrZGLnAzl1W?usp=share_link
Trace files for normal Beta build and removed-inactive-tabs Beta build opening and closing the tab tray 3 times.

Path moving forward: Optimize the initialization of the TabsTray to prioritize current tabs and then inflate the inactive tabs section.

Comment 12

[[:harrisono]]

Attachment: InitialProfilerScreenshot.png

Comment 13

[[:harrisono]]

Attachment: screenshot1.png

Comment 14

[[:harrisono]]

Attachment: screenshot2.png

Comment 15

[[:harrisono]]

Attachment: Stack Trace with removed Inactive Tabs

Comment 16

[[:harrisono]]

Attachment: tabs tray opening recording after updating Recyclerview to 1.3.0

Updated androidx.recyclerview:recyclerview: to 1.3.0 which fixed some issues with Compose viewholders

Comment 17

[[:harrisono]]

There is a new version of androidx.recyclerview:recyclerview: 1.3.0 that have performance improvements for Compose in recyclerviews.

https://developer.android.com/jetpack/androidx/releases/recyclerview#recyclerview-1.3.0
I updated to recyclerview:1.3.0 and removed the DisposeOnViewTreeLifecycleDestroyed ViewCompositionStrategy used in ComposeViewHolder.


This resulted in a small increase in performance (.26 seconds between call of loadIntoView and loadIntoViewInternal) vs the older version of recyclerview (.31 seconds). This is a ~17% improvement.

NOTE: profiler does not accurately record exact times when running as the profiler takes up process time from the app. Time measurements are useful for comparing and getting relative performance.


I think the performance enhancements are more noticeable when running without the profiler.


There are other areas in the app that use the DisposeOnViewTreeLifecycleDestroyed ViewCompositionStrategy as used in ComposeViewHolder that would also need to be changed and would also gain similar performance enhancements.

I've attached a screen recording of this new build above.

Comment 18

[[:harrisono]]

Attachment: stack trace with updated recyclerview library

Comment 19

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Harrison/fix/bug 1822447 tab thumbnails reload (#1691)

Comment 20

[[:harrisono]]

Attachment: Tabs Tray opening with Updated Recyclerview and placeholder image

Screen recording showing Tabs Tray opening with Updated Recyclerview and placeholder image as default image for the imageView that the tab thumbnails are loaded into

Comment 21

[BMO Github Automation]

Authored by https://github.com/HarrisonOg
https://github.com/mozilla-mobile/firefox-android/commit/07b6ea8cd024c3f1e96dcc146f2697970dd67bec
[main] Bug 1822447 - Upgrade RecyclerView Library and remove tab tray thumbnail placeholder (#1691)

Comment 22

[Andrei Mocirean]

Verified as fixed on the latest Nightly (114.0a1) from 7th of May 2023.
Tested on: Google Pixel 7 Pro (Android 13), Lenovo Tab P11 Plus (Android 12).
Thanks!

Comment 23

[Chris Peterson [:cpeterson]]

Setting status-firefox113=wontfix because this change looks too big to risk uplift to a Fenix 113 dot release.

Comment 24

[Verdi [:verdi] Best to slack me]

(In reply to [:harrisono] from comment #10)

I also checked the other browsers. It seems to me that the “Tabs Tray” for Chrome and Brave are both in a separate activity/fragment that stays in memory behind the browser activity/fragment so there is no new painting that occurs except for new tabs.
I’ve attached screen recordings of the other browsers.
I have a feeling that this might be the only way to “fix” the tab thumbnails loading is to refactor the TabsTrayFragment from a DialogFragment to a regular Fragment (or AppCompat version) that can load the thumbnails before being shown/made visible for users. I think the DialogFragment’s lifecycle is to blame for the re-painting each time the Tabs Tray is opened, even if nothing has changed. This seems like a drastic change though and I'm not sure if it's worth the change, especially if it comes at the expense of performance for low-end devices.
I'll note that the loading time for the tabs tray thumbnails in the app currently is longer on low-end devices and not as noticeable on flagships.

I just want to note that while what we have now is certainly better than when we started, it's not actually fixed (I didn't know the bug was marked fixed until I read it just now because I can still see this behavior in release). But given Harrison's comment, I guess we'll have to reevaluate if it's worth pursuing.