Add last_requested field to the favicon_bitmaps table of the favicons database.

components/history/core/browser/thumbnail_database.cc

Index: components/history/core/browser/thumbnail_database.cc
diff --git a/components/history/core/browser/thumbnail_database.cc b/components/history/core/browser/thumbnail_database.cc
index e82888492a36db72869d5b2fc5544556922ceaa8..334bab492a8633feaa900e1aee2b9744b8ffcc9e 100644
--- a/components/history/core/browser/thumbnail_database.cc
+++ b/components/history/core/browser/thumbnail_database.cc
@@ -58,14 +58,17 @@ namespace history {
 //                    the |id| field in the appropriate row in the |favicons|
 //                    table.
 //
-//  id                Unique ID.
-//  icon_id           The ID of the favicon that the bitmap is associated to.
-//  last_updated      The time at which this favicon was inserted into the
+//   id               Unique ID.
+//   icon_id          The ID of the favicon that the bitmap is associated to.
+//   last_updated     The time at which this favicon was inserted into the
 //                    table. This is used to determine if it needs to be
 //                    redownloaded from the web.
-//  image_data        PNG encoded data of the favicon.
-//  width             Pixel width of |image_data|.
-//  height            Pixel height of |image_data|.
+//   last_requested   The time at which this bitmap was last requested. This is
+//                    used to determine the priority with which the bitmap
+//                    should be retained on cleanup.
+//   image_data       PNG encoded data of the favicon.
+//   width            Pixel width of |image_data|.
+//   height           Pixel height of |image_data|.
 
 namespace {
 
@@ -77,18 +80,19 @@ namespace {
 // fatal (in fact, very old data may be expired immediately at startup
 // anyhow).
 
+// Version 8: ???????? by rogerm@chromium.org on 2015-??-??
 // Version 7: 911a634d/r209424 by qsr@chromium.org on 2013-07-01
 // Version 6: 610f923b/r152367 by pkotwicz@chromium.org on 2012-08-20
-// Version 5: e2ee8ae9/r105004 by groby@chromium.org on 2011-10-12
+// Version 5: e2ee8ae9/r105004 by groby@chromium.org on 2011-10-12 (deprecated)
 // Version 4: 5f104d76/r77288 by sky@chromium.org on 2011-03-08 (deprecated)
 // Version 3: 09911bf3/r15 by initial.commit on 2008-07-26 (deprecated)
 
 // Version number of the database.
 // NOTE(shess): When changing the version, add a new golden file for
 // the new version and a test to verify that Init() works with it.
-const int kCurrentVersionNumber = 7;
-const int kCompatibleVersionNumber = 7;
-const int kDeprecatedVersionNumber = 4;  // and earlier.
+const int kCurrentVersionNumber = 8;
+const int kCompatibleVersionNumber = 8;
+const int kDeprecatedVersionNumber = 5;  // and earlier.
 
 void FillIconMapping(const sql::Statement& statement,
                      const GURL& page_url,
@@ -310,7 +314,10 @@ bool InitTables(sql::Connection* db) {
       "last_updated INTEGER DEFAULT 0,"
       "image_data BLOB,"
       "width INTEGER DEFAULT 0,"
-      "height INTEGER DEFAULT 0"
+      "height INTEGER DEFAULT 0,"
+      // This field is at the end so that fresh tables and migrated tables have
+      // the same layout.
+      "last_requested INTEGER DEFAULT 0"
       ")";
   if (!db->Execute(kFaviconBitmapsSql))
     return false;
@@ -396,7 +403,7 @@ void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) {
   // NOTE(shess): This code is currently specific to the version
   // number.  I am working on simplifying things to loosen the
   // dependency, meanwhile contact me if you need to bump the version.
-  DCHECK_EQ(7, kCurrentVersionNumber);
+  DCHECK_EQ(8, kCurrentVersionNumber);
 
   // TODO(shess): Reset back after?
   db->reset_error_callback();
@@ -448,9 +455,8 @@ void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) {
     return;
   }
 
-  // Earlier versions have been handled or deprecated, later versions should be
-  // impossible.
-  if (version != 7) {
+  // Earlier versions have been handled or deprecated.
+  if (version < 7) {
     sql::Recovery::Unrecoverable(recovery.Pass());
     RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_WRONG_VERSION);
     return;
@@ -613,11 +619,24 @@ sql::InitStatus ThumbnailDatabase::Init(const base::FilePath& db_name) {
 }
 
 void ThumbnailDatabase::ComputeDatabaseMetrics() {
-  sql::Statement favicon_count(
-      db_.GetCachedStatement(SQL_FROM_HERE, "SELECT COUNT(*) FROM favicons"));
-  UMA_HISTOGRAM_COUNTS_10000(
-      "History.NumFaviconsInDB",
-      favicon_count.Step() ? favicon_count.ColumnInt(0) : 0);
+  // Count all icon files referenced by the DB.
+  {
+    sql::Statement favicon_count(
+        db_.GetCachedStatement(SQL_FROM_HERE, "SELECT COUNT(*) FROM favicons"));
+    UMA_HISTOGRAM_COUNTS_10000(
+        "History.NumFaviconsInDB",
+        favicon_count.Step() ? favicon_count.ColumnInt(0) : 0);
+  }
+
+  // Count all bitmap resources cached in the DB.
+  {
+    sql::Statement bitmap_count(
+        db_.GetCachedStatement(
+            SQL_FROM_HERE, "SELECT COUNT(*) FROM favicon_bitmaps"));
+    UMA_HISTOGRAM_COUNTS_10000(
+        "History.NumBitmapsInDB",
+        bitmap_count.Step() ? bitmap_count.ColumnInt(0) : 0);

[pkotwicz, 2015/03/20 19:51:55]

Can you explain why you are adding this UMA stat? My understanding is that there
is at most two favicon bitmaps per entry in the favicons database.

If you are in fact adding the stat, you should add an entry to
tools/metrics/histograms/histograms.xml

[Roger McFarlane (Chromium), 2015/03/27 14:40:19]

This change was moved to https://codereview.chromium.org/1032763002/

Let's continue the discussion there.

+  }
 }
 
 void ThumbnailDatabase::BeginTransaction() {
@@ -727,6 +746,22 @@ bool ThumbnailDatabase::GetFaviconBitmap(
   return true;
 }
 
+bool ThumbnailDatabase::GetFaviconBitmapLastRequestedTime(
+    FaviconBitmapID bitmap_id,
+    base::Time* last_requested) {
+  DCHECK(bitmap_id);
+  DCHECK(last_requested);
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "SELECT last_requested FROM favicon_bitmaps WHERE id=?"));
+  statement.BindInt64(0, bitmap_id);
+
+  if (!statement.Step())
+    return false;
+
+  *last_requested = base::Time::FromInternalValue(statement.ColumnInt64(0));
+  return true;
+}
+
 FaviconBitmapID ThumbnailDatabase::AddFaviconBitmap(
     favicon_base::FaviconID icon_id,
     const scoped_refptr<base::RefCountedMemory>& icon_data,
@@ -782,6 +817,17 @@ bool ThumbnailDatabase::SetFaviconBitmapLastUpdateTime(
   return statement.Run();
 }
 
+bool ThumbnailDatabase::SetFaviconBitmapLastRequestedTime(
+    FaviconBitmapID bitmap_id,
+    base::Time time) {
+  DCHECK(bitmap_id);
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "UPDATE favicon_bitmaps SET last_requested=? WHERE id=?"));
+  statement.BindInt64(0, time.ToInternalValue());
+  statement.BindInt64(1, bitmap_id);
+  return statement.Run();
+}
+
 bool ThumbnailDatabase::DeleteFaviconBitmap(FaviconBitmapID bitmap_id) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM favicon_bitmaps WHERE id=?"));
@@ -1086,6 +1132,8 @@ bool ThumbnailDatabase::RetainDataForPageUrls(
       "ON (old.id = mapping.old_icon_id)";
   const char kDropOldFaviconsTable[] = "DROP TABLE old_favicons";
 
+  // Note that we allow the last_requested field to be reset to the default
+  // value (0).

[pkotwicz, 2015/03/20 19:51:55]

We should copy the last_requested field over. Having special behavior is
confusing.

[Roger McFarlane (Chromium), 2015/03/27 14:40:18]

Done.

   const char kRenameFaviconBitmapsTable[] =
       "ALTER TABLE favicon_bitmaps RENAME TO old_favicon_bitmaps";
   const char kCopyFaviconBitmaps[] =
@@ -1193,8 +1241,7 @@ sql::InitStatus ThumbnailDatabase::InitImpl(const base::FilePath& db_name) {
   base::mac::SetFileBackupExclusion(db_name);
 #endif
 
-  // thumbnails table has been obsolete for a long time, remove any
-  // detrious.
+  // thumbnails table has been obsolete for a long time, remove any detritus.
   ignore_result(db_.Execute("DROP TABLE IF EXISTS thumbnails"));
 
   // At some point, operations involving temporary tables weren't done
@@ -1252,6 +1299,12 @@ sql::InitStatus ThumbnailDatabase::InitImpl(const base::FilePath& db_name) {
       return CantUpgradeToVersion(cur_version);
   }
 
+  if (cur_version == 7) {
+    ++cur_version;
+    if (!UpgradeToVersion8())
+      return CantUpgradeToVersion(cur_version);
+  }
+
   LOG_IF(WARNING, cur_version < kCurrentVersionNumber) <<
       "Thumbnail database version " << cur_version << " is too old to handle.";
 
@@ -1332,6 +1385,18 @@ bool ThumbnailDatabase::UpgradeToVersion7() {
   return true;
 }
 
+bool ThumbnailDatabase::UpgradeToVersion8() {
+  // Add the last_requested column to the favicon_bitmaps table.
+  const char kFaviconBitmapsAddLastRequestedSql[] =
+      "ALTER TABLE favicon_bitmaps ADD COLUMN last_requested INTEGER DEFAULT 0";
+  if (!db_.Execute(kFaviconBitmapsAddLastRequestedSql))
+    return false;

[pkotwicz, 2015/03/20 19:51:55]

I wonder whether adding an extra index would be useful to make the compaction
query quicker. Unfortunately, shess@ would be the person who would know whether
this is desirable.

[Roger McFarlane (Chromium), 2015/03/27 14:40:18]

Yes, that's something I wanted to discuss more as well.

I don't think an index over the last_requested column would help.

We're only compacting large icons, of which we expect to have very few compared
to the number of regular favicons. The latter gets culled on a history deletion,
modulo items referenced by bookmarks. I.e., we're deleting a limited number
large bitmaps whose IDs are in the set of large bitmaps ordered by their last
used time. AFAICT, this would still require a full scan even if we had the
index. A multi-column index over the icon type and the last_requested time would
help. As might putting the last_requested time in a separate id/value table used
only for NTP large icons.  

[Scott Hess - ex-Googler, 2015/03/27 17:59:33]

The CL doesn't contain any information about which icons are considered large,
nor which icons have their last_requested set (large only), nor who wants to
make the decision about what to trim from the cache, nor whether the decision is
"older than T" or "keep the first N newest".  As-is, it feels like the caller is
tucking the last_requested in and then pulling it back out and making the
decision itself, in which case the only index that matters is the id index.

If the last_requested were only set for large icons, then an index on
last_requested would allow writing something like:
  -- partial index.  Could also just allow NULL in last_requested and use "IS
NOT NULL".
  CREATE INDEX last_requested_index ON favicon_bitmaps(last_requested) WHERE
last_requested > 0;

  DELETE FROM favicon_bitmaps WHERE id IN (SELECT id FROM favicon_bitmaps WHERE
last_requested > 0 ORDER BY last_requested DESC LIMIT 1000000 OFFSET ?N)

The LIMIT constant is a hack to say "All the id's after N".  "Older than 30
days" would be cleaner.

I think the real question is when you're going to do this cleaning?  If you're
going to do it once per browser session, then it probably doesn't matter.  If
you're going to do it incrementally all the time, then it might matter.

+
+  meta_table_.SetVersionNumber(8);
+  meta_table_.SetCompatibleVersionNumber(std::min(8, kCompatibleVersionNumber));
+  return true;
+}
+
 bool ThumbnailDatabase::IsFaviconDBStructureIncorrect() {
   return !db_.IsSQLValid("SELECT id, url, icon_type FROM favicons");
 }