808212 - Expire thumbnails along with history

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We should expire thumbnails that we don't need. We probably need at most 10-15.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

This kills off thumbnails whenever we expire history. We could separate this out to a separate data/function, but I don't get the feeling its worth it. Includes some tests.

I also talked to mfinkle about refusing to even store a thumbnail if that site isn't currently in TopSites. We decided that's likely to have lots of edge cases and not to worry about that here.

I also added a call to deleteUnusedImages that's slightly unrelated, but should have been in to begin with. I know bnicholson is rewriting some db stuff. From what I heard, he's leaving in the thumbnail table and we'll still need some form of this.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 677911 [details] [diff] [review]
Patch

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

Let's go 'round again so we can make sure those tests are right, and double-check the reorg.

Good stuff! Should help with DB shrinkage.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +88,5 @@
>      static final int AGGRESSIVE_EXPIRY_RETAIN_COUNT = 500;
>  
>      // Minimum duration to keep when expiring.
>      static final long DEFAULT_EXPIRY_PRESERVE_WINDOW = 1000L * 60L * 60L * 24L * 28L;     // Four weeks.
> +    // Minimum number of thumbnails to keep around

Tiny nit: closing period for consistency, linebreak before comment.

@@ +1348,5 @@
>       */
>      public void expireHistory(final SQLiteDatabase db, final int retain, final long keepAfter) {
>          final long rows = DatabaseUtils.queryNumEntries(db, TABLE_HISTORY);
> +
> +        if (retain < rows) {

Reverse this conditional: early exit is preferable.

"But there are two parts to this method!"

Yup. Split them into two.

  protected void expireHistory(…) {
    …
    if (retain >= rows) {
      debug("Not expiring…");
      return;
    }
    …
  }

  protected void eliminateRedundantThumbnails() {
    …
  }

That way you can reuse the thumbnail remover, test it more easily, and everything will be easier to read.

If you always want to prune thumbnails when you expire history (I'm not sure you do…), and regardless of whether you remove any history rows, then split this into expireHistoryRows, and have an expireHistory method that calls both.

And see below…

@@ +1352,5 @@
> +        if (retain < rows) {
> +            final String sortOrder = BrowserContract.getFrecencySortOrder(false, true);
> +            final long toRemove = rows - retain;
> +            debug("Expiring at most " + toRemove + " rows earlier than " + keepAfter + ".");
> +    

Trailing whitespace.

@@ +1355,5 @@
> +            debug("Expiring at most " + toRemove + " rows earlier than " + keepAfter + ".");
> +    
> +            final String sql;
> +            if (keepAfter > 0) {
> +                sql = "DELETE FROM " + TABLE_HISTORY + " " + 

Trailing whitespace. (You didn't add it, but let's kill it as we're breaking blame!)

@@ +1366,5 @@
> +                sql = "DELETE FROM " + TABLE_HISTORY + " WHERE " + History._ID + " " +
> +                      "IN ( SELECT " + History._ID + " FROM " + TABLE_HISTORY + " " +
> +                      "ORDER BY " + sortOrder + " LIMIT " + toRemove + ")";
> +            }
> +    

Trailing whitespace.

@@ +1375,3 @@
>          }
>  
> +        // remove any unnecessary thumbnails

Caps and punctuation. This can go in a Javadoc comment when this is split out.

@@ +1378,5 @@
> +        final String sortOrder = BrowserContract.getFrecencySortOrder(true, false);
> +        final String sql = "UPDATE " + TABLE_IMAGES + " SET " + Images.THUMBNAIL + " = NULL " +
> +              " WHERE " + Images.URL + " NOT IN ( SELECT " +
> +                Combined.URL + " FROM " + VIEW_COMBINED_WITH_IMAGES +
> +              " ORDER BY " + sortOrder + " LIMIT " + DEFAULT_EXPIRY_THUMBNAIL_COUNT + ")";

Can we do this?

final String sql = "UPDATE " + TABLE_IMAGES +
                   "SET " + Images.THUMBNAIL + " = NULL " +
                   "WHERE " + Images.URL + " NOT IN (" +
                     " SELECT " + Combined.URL +
                     " FROM " + VIEW_COMBINED_WITH_IMAGES +
                     " ORDER BY " + sortOrder +
                     " LIMIT " + DEFAULT_EXPIRY_THUMBNAIL_COUNT +
                   ")";

Much easier for suckers like me to read :D

@@ +1533,5 @@
>                      keepAfter = 0;
>                      retainCount = AGGRESSIVE_EXPIRY_RETAIN_COUNT;
>                  }
>                  expireHistory(db, retainCount, keepAfter);
> +                deleteUnusedImages(uri);

So now we have a nice symmetry:

  expireHistory(…);
  eliminateRedundantThumbnails();
  deleteUnusedImages(…);

Or you can call eliminateRedundantThumbnails from within expireHistory.

::: mobile/android/base/tests/testBrowserProvider.java.in
@@ +1628,5 @@
> +                allVals[i].put(mImagesFaviconCol, i);
> +            }
> +
> +            inserts = mProvider.bulkInsert(mImagesUri, allVals);
> +            mAsserter.is(inserts, count, "Excepted number of inserts matches");

Accepted?

@@ +1630,5 @@
> +
> +            inserts = mProvider.bulkInsert(mImagesUri, allVals);
> +            mAsserter.is(inserts, count, "Excepted number of inserts matches");
> +
> +            c = mProvider.query(mImagesUri, null, "THUMBNAIL IS NOT NULL", null, null);

Hardcoding column name.

@@ +1658,5 @@
>              c = mProvider.query(mHistoryUri, null, "", null, null);
>              mAsserter.is(c.getCount(), 500, "500 history entries found");
>  
> +            // expiring with a aggressive priority should delete all but 10 thumbnails
> +            c = mProvider.query(mImagesUri, null, "THUMBNAIL IS NOT NULL", null, null);

You haven't restored any thumbnails after your last expiry, so you'll *never* have more than 10 at this point.

You need to re-add thumbnails between tests.

@@ +1685,5 @@
>              mProvider.delete(url, null, null);
>              c = mProvider.query(mHistoryUri, null, "", null, null);
>              mAsserter.is(c.getCount(), 500, "500 history entries found");
> +
> +            // expiring with a aggressive priority should delete all but 10 thumbnails

Same again.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

I just used expireThumbnails instead of eliminateRedundantThumbnails. We're not removing redundant things. Just information that we're unlikely to ever need.

I'm a bit worried about adding all these entries during the test. We're using a transaction, but it still takes a long long LONG time. I'll file another bug to look into it.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 678393 [details] [diff] [review]
Patch

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

Looks good to me.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1456,5 @@
>          db.execSQL(sql);
>      }
>  
> +    /**
> +     * Remove any thumbnails that for sites that aren't likely to be ever shown.

Nit: "ever be shown".

::: mobile/android/base/tests/testBrowserProvider.java.in
@@ +1653,5 @@
> +            mAsserter.is(c.getCount(), thumbCount, thumbCount + " thumbnails found");
> +
> +            ensureEmptyDatabase();
> +            // insert a bunch of new entries
> +            createFakeHistory(0, count);

If you're concerned with perf, you could just restore thumbnails by doing a bulk UPDATE on the existing rows, no? Normal priority comes before aggressive, so there will still be rows to update. Would save you a wipe and recreate.

Comment 5

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/810f9f2dd49d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/810f9f2dd49d