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

components/history/core/browser/thumbnail_database.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/history/core/browser/thumbnail_database.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 40 matching lines @@
 //                    the link tag. The FAVICON type is used for the default
 //                    favicon.ico favicon.
 //
 // favicon_bitmaps    This table contains the PNG encoded bitmap data of the
 //                    favicons. There is a separate row for every size in a
 //                    multi resolution bitmap. The bitmap data is associated
 //                    to the favicon via the |icon_id| field which matches
 //                    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 {
 
 // For this database, schema migrations are deprecated after two
 // years.  This means that the oldest non-deprecated version should be
 // two years old or greater (thus the migrations to get there are
 // older).  Databases containing deprecated versions will be cleared
 // at startup.  Since this database is a cache, losing old data is not
 // 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,
                      IconMapping* icon_mapping) {
   icon_mapping->mapping_id = statement.ColumnInt64(0);
   icon_mapping->icon_id = statement.ColumnInt64(1);
   icon_mapping->icon_type =
       static_cast<favicon_base::IconType>(statement.ColumnInt(2));
   icon_mapping->icon_url = GURL(statement.ColumnString(3));
   icon_mapping->page_url = page_url;
@@ skipping 201 matching lines @@
     return false;
 
   const char kFaviconBitmapsSql[] =
       "CREATE TABLE IF NOT EXISTS favicon_bitmaps"
       "("
       "id INTEGER PRIMARY KEY,"
       "icon_id INTEGER NOT NULL,"
       "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;
 
   return true;
 }
 
 // NOTE(shess): Schema modifications must consider initial creation in
 // |InitImpl()|, recovery in |RecoverDatabaseOrRaze()|, and history pruning in
 // |RetainDataForPageUrls()|.
@@ skipping 65 matching lines @@
 // Recover the database to the extent possible, razing it if recovery
 // is not possible.
 // TODO(shess): This is mostly just a safe proof of concept.  In the
 // real world, this database is probably not worthwhile recovering, as
 // opposed to just razing it and starting over whenever corruption is
 // detected.  So this database is a good test subject.
 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();
 
   // For histogram purposes.
   size_t favicons_rows_recovered = 0;
   size_t favicon_bitmaps_rows_recovered = 0;
   size_t icon_mapping_rows_recovered = 0;
   int64 original_size = 0;
   base::GetFileSize(db_path, &original_size);
@@ skipping 31 matching lines @@
   // NOTE(shess): v5 and v6 are currently not deprecated in the normal Init()
   // path, but are deprecated in the recovery path in the interest of keeping
   // the code simple.  http://crbug.com/327485 for numbers.
   DCHECK_LE(kDeprecatedVersionNumber, 6);
   if (version <= 6) {
     sql::Recovery::Unrecoverable(recovery.Pass());
     RecordRecoveryEvent(RECOVERY_EVENT_DEPRECATED);
     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;
   }
 
   // Recover to current schema version.
   sql::MetaTable recover_meta_table;
   if (!recover_meta_table.Init(recovery->db(), kCurrentVersionNumber,
                                kCompatibleVersionNumber)) {
     sql::Recovery::Rollback(recovery.Pass());
@@ skipping 142 matching lines @@
     if (status == sql::INIT_OK)
       return status;
 
     meta_table_.Reset();
     db_.Close();
   }
   return status;
 }
 
 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() {
   db_.BeginTransaction();
 }
 
 void ThumbnailDatabase::CommitTransaction() {
   db_.CommitTransaction();
 }
 
@@ skipping 89 matching lines @@
     *png_icon_data = data;
   }
 
   if (pixel_size) {
     *pixel_size = gfx::Size(statement.ColumnInt(2),
                             statement.ColumnInt(3));
   }
   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,
     base::Time time,
     const gfx::Size& pixel_size) {
   DCHECK(icon_id);
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, width, "
       "height) VALUES (?, ?, ?, ?, ?)"));
   statement.BindInt64(0, icon_id);
@@ skipping 35 matching lines @@
     FaviconBitmapID bitmap_id,
     base::Time time) {
   DCHECK(bitmap_id);
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "UPDATE favicon_bitmaps SET last_updated=? WHERE id=?"));
   statement.BindInt64(0, time.ToInternalValue());
   statement.BindInt64(1, bitmap_id);
   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=?"));
   statement.BindInt64(0, bitmap_id);
   return statement.Run();
 }
 
 bool ThumbnailDatabase::SetFaviconOutOfDate(favicon_base::FaviconID icon_id) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "UPDATE favicon_bitmaps SET last_updated=? WHERE icon_id=?"));
@@ skipping 284 matching lines @@
   const char kRenameFaviconsTable[] =
       "ALTER TABLE favicons RENAME TO old_favicons";
   const char kCopyFavicons[] =
       "INSERT INTO favicons (id, url, icon_type) "
       "SELECT mapping.new_icon_id, old.url, old.icon_type "
       "FROM old_favicons AS old "
       "JOIN temp.icon_id_mapping AS mapping "
       "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[] =
       "INSERT INTO favicon_bitmaps "
       "  (icon_id, last_updated, image_data, width, height) "
       "SELECT mapping.new_icon_id, old.last_updated, "
       "    old.image_data, old.width, old.height "
       "FROM old_favicon_bitmaps AS old "
       "JOIN temp.icon_id_mapping AS mapping "
       "ON (old.icon_id = mapping.old_icon_id)";
@@ skipping 87 matching lines @@
     return sql::INIT_FAILURE;
 
   // TODO(shess): Failing Begin() implies that something serious is
   // wrong with the database.  Raze() may be in order.
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
   // Exclude the thumbnails file from backups.
   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
   // atomically and users have been stranded.  Drop those tables and
   // move on.
   // TODO(shess): Prove it?  Audit all cases and see if it's possible
   // that this implies non-atomic update, and should thus be handled
   // via the corruption handler.
   ignore_result(db_.Execute("DROP TABLE IF EXISTS temp_favicons"));
   ignore_result(db_.Execute("DROP TABLE IF EXISTS temp_favicon_bitmaps"));
@@ skipping 25 matching lines @@
   }
 
   if (cur_version < 7 && !db_.DoesColumnExist("favicons", "sizes")) {
     LOG(ERROR) << "Raze because of missing favicon.sizes";
     RecordInvalidStructure(STRUCTURE_EVENT_VERSION5);
 
     db_.RazeAndClose();
     return sql::INIT_FAILURE;
   }
 
   if (cur_version == 5) {

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

Given that version 5 is now deprecated shouldn't this be removed?

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

Done.

See: https://codereview.chromium.org/1010973011

     ++cur_version;
     if (!UpgradeToVersion6())
       return CantUpgradeToVersion(cur_version);
   }
 
   if (cur_version == 6) {
     ++cur_version;
     if (!UpgradeToVersion7())
       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.";
 
   // Initialization is complete.
   if (!transaction.Commit())
     return sql::INIT_FAILURE;
 
   // Raze the database if the structure of the favicons database is not what
   // it should be. This error cannot be detected via the SQL error code because
   // the error code for running SQL statements against a database with missing
@@ skipping 60 matching lines @@
       db_.Execute("CREATE INDEX IF NOT EXISTS favicons_url ON favicons(url)");
 
   if (!success)
     return false;
 
   meta_table_.SetVersionNumber(7);
   meta_table_.SetCompatibleVersionNumber(std::min(7, kCompatibleVersionNumber));
   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");
 }
 
 }  // namespace history