789923 - Page thumbnails take up a lot of space

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

See https://bugzilla.mozilla.org/show_bug.cgi?id=760653#c5

Even on latest nightly, most of my browser.db file is taken up by thumbnails. We should only store thumbnails for the top few visited sites (those on about:home, plus maybe a couple more). If a site is not on about:home, then most likely it will need to be visited before it will show up on about:home, so we can grab the thumbnail at that time.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Remove thumbnails for non-top-sites after leaving page

Any thoughts on whether or not we should do this? Right now the 86 thumbnails in my browser.db file are taking over a megabyte.

Instructions to see how much space thumbnails take up if you want to try it yourself:

PROFILE=$(adb shell run-as org.mozilla.fennec_$USER ls files/mozilla/ | grep default | tr -d '\r\n')
adb shell run-as org.mozilla.fennec_$USER cp files/mozilla/$PROFILE/browser.db /sdcard/
adb pull /sdcard/browser.db
sqlite3 browser.db "SELECT count(*), sum(length(thumbnail)) FROM images"

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 659833 [details] [diff] [review]
Remove thumbnails for non-top-sites after leaving page

I am not against clearing the thumbnails. I can't tell right now if 16 (enough for about:home) is OK or if we should keep more. This is a good start.

Tab.handleLocationChange might be on the Gecko thread. If so, we should probably explicitly put the DB calls on the background thread using GeckoAppShell.getHandler, right?

Also, this won't clear out the existing large backlog of thumbnails.

We also have a method called cleanupSomeDeletedRecords which tries to remove some stale records from the DB. I'm not sure how well this works at this point.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in?force=1#1185

f+ from me for now. I'd like to see if we should use a general purge instead of limited to the URL we are navigating away from.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 659833 [details] [diff] [review]
Remove thumbnails for non-top-sites after leaving page

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

I'd probably add a DB migration that removes all or most of the existing non-top site thumbnails somehow.

I'm not totally against this approach but I feel we should avoid running isTopSite() on every page visit. Giving r- to maybe get some exploration on different options.

::: mobile/android/base/Tab.java
@@ +554,5 @@
> +        // When we're leaving a page, drop the thumbnail for it if the site isn't
> +        // one of the top 16 sites (this should cover anything appearing on about:home with
> +        // a little fuzz to account for sites moving on and off the toplist). This
> +        // should prevent the browser.db file from blowing up with useless image data.
> +        if (oldUrl != null && !BrowserDB.isTopSite(mContentResolver, NUM_TOPSITES_TO_KEEP_THUMBNAILS, oldUrl)) {

This query is relatively expensive to be run on every page visit. Maybe a better option would be something like a non-greedy cleanup operation that runs once (or more) per day and removes some or all thumbnails for non-top sites (in a single transaction).

In any case, I agree with mfinkle that this should probably be run in the background thread. Also, I wonder if this cleanup call should actually reside in LocalBrowserDB.updateVisitedHistory() instead of here.

Comment 4

[Richard Newman [:rnewman]]

See Bug 784040, perhaps?

IMO Fennec should listen for the system low memory intent -- ACTION_DEVICE_STORAGE_LOW -- and have a pile of remedial methods called from there: two of those can be to prune history and thumbnails. By all means prune some thumbnails regularly, but do it really aggressively on low memory (I'm sure your average user would prefer blank about:home images to a non-functional phone!).

Re Comment 3: perhaps a good time to prune thumbnails is after displaying about:home? At that point we know which are the top sites…

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Drop thumbnails when device storage is running low

This one only gets run when we get the storage-is-low notification and just drops all of the thumbnails. We might want to drop favicons too but I don't know how that will affect sync behaviour.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 671493 [details] [diff] [review]
Drop thumbnails when device storage is running low

Looks good to me. I think a small test would be useful too:
* Load some thumbnails
* Count thumbmails
* Call removeThumbnails
* Count should be zero

Comment 7

[Richard Newman [:rnewman]]

Have you verified that sqlite actually reclaims the space when you do this? And if it does, that it doesn't do it through naive copying, which will be bad under memory pressure?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Richard Newman [:rnewman] from comment #7)
> Have you verified that sqlite actually reclaims the space when you do this?
> And if it does, that it doesn't do it through naive copying, which will be
> bad under memory pressure?

I've verified that the browser.db file drops in size when this code is run, and the following statement:

select sum(length(thumbnail)) from images;

goes from being non-zero to zero. However I don't know if it does it via naive copying or not. I feel like if it is doing copying in this case, then it will be doing copying any time we do a data update, which means there's no reasonable way to reduce our disk storage use while under pressure anyway.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Looks good to me. I think a small test would be useful too:

Sounds good, I'll add a test.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Test BrowserDB.removeThumbnails

I tacked it on to the existing testThumbnails test since it was already doing the stuff I would need to set up the thumbnails.

Try build passed: https://tbpl.mozilla.org/?tree=Try&rev=cb14c575a927

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 671493 [details] [diff] [review]
Drop thumbnails when device storage is running low

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

So, this will drop all thumbnails without any special handling for top sites. How often is storage pressure likely to happen? Patch looks good. Just trying to understand the impact on UX (i.e. we don't want users to see a lot of empty thumbnails on about:home very often).

::: mobile/android/base/MemoryMonitor.java
@@ +107,5 @@
>      public void onReceive(Context context, Intent intent) {
>          if (Intent.ACTION_DEVICE_STORAGE_LOW.equals(intent.getAction())) {
>              Log.d(LOGTAG, "Device storage is low");
>              mStoragePressure = true;
> +            GeckoAppShell.getHandler().post(new StorageReducer());

Not a problem in your patch but it would be nice if getHandler() was renamed to something like getBackgroundHandler() for clarity...

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 671698 [details] [diff] [review]
Test BrowserDB.removeThumbnails

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

I'd personally prefer to have this test in testBrowserProvider just to keep all BrowserProvider tests in one place. I don't feel strongly about it though.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Lucas Rocha (:lucasr) from comment #11)
> So, this will drop all thumbnails without any special handling for top
> sites. How often is storage pressure likely to happen? Patch looks good.
> Just trying to understand the impact on UX (i.e. we don't want users to see
> a lot of empty thumbnails on about:home very often).

The storage pressure intent is triggered when the free space on disk drops below a certain threshold. [1] On my GN, for example, which has ~13 GB, the storage pressure triggers at around 300 MB free. It's lower on other device which have less storage. The intent is "sticky" so if Fennec starts up after the intent is dispatched we will still receive a copy (i.e. the storage pressure on/off intents basically act as flags for whether or not the storage pressure is currently on or off). I've noticed that while storage pressure is "on" various other actions are also impossible - for example trying to reinstall fennec doesn't work (it says not enough space is available) even though there are 200+ megs free.

> Not a problem in your patch but it would be nice if getHandler() was renamed
> to something like getBackgroundHandler() for clarity...

Filed bug 802130.

(In reply to Lucas Rocha (:lucasr) from comment #12)
> I'd personally prefer to have this test in testBrowserProvider just to keep
> all BrowserProvider tests in one place. I don't feel strongly about it
> though.

I did consider that initially but decided against because it seemed to be testing at the wrong level of abstraction (i.e. I would have to add/remove things directly in the BrowserContentProvider whereas what I really want to test is the BrowserDB API, regardless of how it's implemented).

[1] https://android.googlesource.com/platform/frameworks/base/+/826343138dfd8666d2263dd82bfdbf657fc1881e/services/java/com/android/server/DeviceStorageMonitorService.java - the sendNotification call in checkMemory sends the intent

Comment 14

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/386b12107730
https://hg.mozilla.org/integration/mozilla-inbound/rev/47ee1a38fe31

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/386b12107730
https://hg.mozilla.org/mozilla-central/rev/47ee1a38fe31