Closed Bug 1130694 Opened 5 years ago Closed 5 years ago

Opening the TabsTray causes all tab thumbnails to refresh and save to the DB

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: mfinkle, Assigned: mfinkle)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files)

I started looking into why my Galaxy Nexus was always getting onTrimMemory notifications at level 15 while running Fennec. output in logcat. Level 15 is critical levels, background process are getting killed.

I noticed that a lot of Bitmap, ByteArrays and BitmapCompress were happening via Monitor's allocation tracking.

It turns out that every time we open the TabsTray, we refresh all the thumbnails via a Gecko event and then save the resulting Bitmaps into the DB. This was added in bug 711543, which I r+, but I did have memory concerns back then!

On a whim, I commented out Tabs.refreshThumbnails [1] and I saw zero onTrimMemory notifications.

When restoring tabs after a crash or just cause you have the preference set, I noticed that the "zombie" (STATE_DELAYED) tabs had no thumbnails until I loaded them. There is code in ThumbnailHelper.getAndProcessThumbnailFor(...) that looks for zombie tabs and tries to load a saved thumbnail from the DB. So I uncommented the code and just put a check to only refresh Tabs with a null thumbnail. This made the zombie tabs show a thumbnail, since it was highly likely their thumbnail was in the DB.

The downside of not refresh all the time is stale thumbnails for dynamic tabs, tabs where DOM stuff is changing and updating the page. Given the memory issues we have and the larger thumbnail sizes (high DPI), I am leaning (again) to not unconditionally update the tab thumbnails when opening the TabsTray.
To add insult to injury, the current code will load zombie tab thumbnails from the DB, call ThumbnailHelper.setTabThumbnail(...) which calls Tab.updateThumbnail(...) and then saves the bitmap back out to the DB.
First patch just removes the code that loads a thumbnail from the DB in ThumbnailHelper.getAndProcessThumbnailFor(...). It's the only code in the class that uses the DB and it forces a few other callers to pass in a DB, even if they don't want the data from the DB.

Removing this code means the other callers no longer need to pass the DB.
Assignee: nobody → mark.finkle
Attachment #8560832 - Flags: review?(rnewman)
Comment on attachment 8560832 [details] [diff] [review]
remove-db-stuff v0.1

I forgot to mention that I move the code to load a thumbnail from the DB into a helper method in the Tab class.
This patch tweaks the Tabs.refreshThumbnails(..) method to only attempt to load thumbnails from the DB into Tabs that currently have no thumbnail. Note that the helper method does not attempt to save the thumbnail back out to the DB, but it does notify the Tab Listeners of the THUMBNAIL change.
Attachment #8560833 - Flags: review?(rnewman)
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → All
Comment on attachment 8560832 [details] [diff] [review]
remove-db-stuff v0.1

Review of attachment 8560832 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/Tab.java
@@ +672,5 @@
> +    public void loadThumbnailFromDB(final BrowserDB db) {
> +        try {
> +            String url = getURL();
> +            if (url == null)
> +                return;

{}, make url final.
Attachment #8560832 - Flags: review?(rnewman) → review+
Comment on attachment 8560833 [details] [diff] [review]
refresh-empty-tabs v0.1

Review of attachment 8560833 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/Tabs.java
@@ +547,5 @@
>              @Override
>              public void run() {
>                  for (final Tab tab : mOrder) {
> +                    if (tab.getThumbnail() == null) {
> +                        tab.loadThumbnailFromDB(db);

Please file a follow-up: now that we explicitly retrieve from the DB for a given tab, we can turn this inside out and retrieve all of these thumbnails at once, rather than making a separate query for each.
Attachment #8560833 - Flags: review?(rnewman) → review+
Keywords: perf
https://hg.mozilla.org/mozilla-central/rev/8839dac4cffc
https://hg.mozilla.org/mozilla-central/rev/fef1003fe34b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.