Precache tracking database

components/precache/core/precache_database.cc

+// Copyright 2013 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/precache/core/precache_database.h"
+
+#include <map>
+
+#include "base/metrics/histogram.h"
+#include "base/time/time.h"
+#include "components/precache/core/precache_statistics_table.h"
+#include "components/precache/core/precache_url_table.h"
+#include "sql/connection.h"
+#include "url/gurl.h"
+
+namespace {
+
+// The number of days old that an entry in the precache URL table can be before
+// it is considered "old" and is removed from the table.
+const int64 kPrecacheHistoryExpiryPeriodDays = 60;
+
+// The default max range of UMA_HISTOGRAM_COUNTS is 1 million, which is too
+// small for tracking precache byte counts, since counts can easily be greater
+// than 1 MB.
+#define UMA_HISTOGRAM_BYTES(name, sample) \
+  UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 1000000000, 50);

[mmenke, 2013/10/29 18:23:34]

Samples are integers, aren't they?  If that's the case, and we actually can hit
1 GB, it's not outside the realm of possibility we can hit 2 GB...At which point
we overflow.  If it's outside the realm of possibility, we should be using a
smaller number.

My recommended solution:  Record all values in kilobytes.

[mmenke, 2013/10/29 18:23:34]

I think we really should change the name.  One could well imaging a
UMA_HISTOGRAM_BYTES being added as a standard code, which could well cause
confusion.  Maybe just PRECACHE_UMA_HISTOGRAM_BYTES or PRECACHE_UMA_BYTES.

[sclittle, 2013/10/29 23:40:45]

Done.

[sclittle, 2013/10/29 23:40:45]

Changed to report values in KB using UMA_HISTOGRAM_COUNTS.

+
+void RecordSingleDayPrecacheUMA(
+    const precache::PrecacheStatisticsTable::PrecacheStatistics& stats) {
+  UMA_HISTOGRAM_BYTES("Precache.DailyPrecachedBytes", stats.precached_bytes);
+  UMA_HISTOGRAM_BYTES("Precache.DailyDownloadedBytes", stats.downloaded_bytes);
+  UMA_HISTOGRAM_BYTES("Precache.DailyDownloadedBytes.Cellular",
+                      stats.downloaded_bytes_cellular);
+  UMA_HISTOGRAM_BYTES("Precache.DailySavedBytes", stats.saved_bytes);
+  UMA_HISTOGRAM_BYTES("Precache.DailySavedBytes.Cellular",
+                      stats.saved_bytes_cellular);
+
+  if (stats.saved_bytes + stats.downloaded_bytes > 0) {
+    UMA_HISTOGRAM_PERCENTAGE("Precache.DailySavingsPercentage",
+                             stats.saved_bytes * 100.0 /
+                                 (stats.saved_bytes + stats.downloaded_bytes));
+  }
+
+  if (stats.saved_bytes_cellular + stats.downloaded_bytes_cellular > 0) {
+    UMA_HISTOGRAM_PERCENTAGE(
+        "Precache.DailySavingsPercentage.Cellular",
+        stats.saved_bytes_cellular * 100.0 /
+            (stats.saved_bytes_cellular + stats.downloaded_bytes_cellular));
+  }
+}
+
+#undef UMA_HISTOGRAM_BYTES

[mmenke, 2013/10/29 18:23:34]

Don't think this is needed, and #undef seems pretty uncommon in Chrome.

[sclittle, 2013/10/29 23:40:45]

Removed.

+
+}  // namespace
+
+namespace precache {
+
+PrecacheDatabase::PrecacheDatabase()
+    : precache_url_table_(new PrecacheURLTable()),
+      precache_statistics_table_(new PrecacheStatisticsTable()) {
+  // A PrecacheDatabase can be constructed on any thread.
+  DetachFromThread();

[mmenke, 2013/10/29 18:23:34]

Does this work?  Looks like the parent's destructor will call
CalledOnValidThread(), which will fail when the thread ID is not set, which is
what DetachFromThread() does.

[sclittle, 2013/10/29 23:40:45]

If the thread ID is not set, CalledOnValidThread() will set the thread ID to
that of the current thread.

https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

ThreadCheckerImpl::EnsureThreadIdAssigned() sets the thread ID to the current
thread if it is unset.

+}
+
+PrecacheDatabase::~PrecacheDatabase() {
+  // Since the PrecacheDatabase is refcounted, it will only be deleted if there
+  // are no references remaining to it, meaning that it is not in use. Thus, it
+  // is safe to delete it, regardless of what thread we are on.
+  DetachFromThread();
+}
+
+void PrecacheDatabase::Init(sql::Connection* db) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!db_);  // Init must only be called once.
+  DCHECK(db);    // |db| must not be NULL.
+
+  db_.reset(db);
+  if (CantAccessDatabase()) {
+    // Don't initialize the URL table or statistics table if unable to access
+    // the database.
+    return;
+  }
+
+  precache_url_table_->Init(db_.get());
+  precache_statistics_table_->Init(db_.get());
+}
+
+void PrecacheDatabase::ReportAndDeleteOldStats(base::Time end_date) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(db_);
+
+  if (CantAccessDatabase()) {
+    // Do nothing if unable to access the database.
+    return;
+  }
+
+  // Delete old precache history that has expired.
+  precache_url_table_->DeleteAllPrecachedBetween(
+      base::Time::FromInternalValue(0),

[mmenke, 2013/10/29 18:23:34]

Can't this just be base::Time()?

[sclittle, 2013/10/29 23:40:45]

Done.

+      end_date - base::TimeDelta::FromDays(kPrecacheHistoryExpiryPeriodDays));
+
+  std::map<base::Time, PrecacheStatisticsTable::PrecacheStatistics> stats_map;

[mmenke, 2013/10/29 18:23:34]

This is really crying out for a public typedef in PrecacheStatisticsTable.

[sclittle, 2013/10/29 23:40:45]

Done.

+  precache_statistics_table_->GetAllStatsBetween(
+      base::Time::FromInternalValue(0), end_date, &stats_map);

[mmenke, 2013/10/29 18:23:34]

base::Time(0)?

[sclittle, 2013/10/29 23:40:45]

Changed to base::Time().

+
+  // Report UMA for every row of old statistics in the statistics table. There
+  // won't be any rows in the statistics table for days when nothing was fetched
+  // or precached.
+  for (std::map<base::Time,
+                PrecacheStatisticsTable::PrecacheStatistics>::const_iterator
+           it = stats_map.begin();
+       it != stats_map.end(); ++it) {
+    RecordSingleDayPrecacheUMA(it->second);
+  }
+
+  precache_statistics_table_->DeleteAllStatsBetween(
+      base::Time::FromInternalValue(0), end_date);
+}
+
+void PrecacheDatabase::RecordURLFetched(GURL url, base::Time fetch_time,
+                                        int64 size, bool was_cached,
+                                        bool is_precaching, bool is_cellular) {

[mmenke, 2013/10/29 18:23:34]

Should we separate out those resources we had to revalidate from those we
didn't?  Successful precaching is great in either case, but makes a bigger
difference in the no-revalidation case.

[sclittle, 2013/10/29 23:40:45]

That's true, but for individual fetches we're just tracking the effects on
bandwidth usage, and the main difference between a fresh cache hit and a 304 is
the latency. Latency metrics are outside the scope of this CL.

+  DCHECK(CalledOnValidThread());
+  DCHECK(db_);
+
+  if (CantAccessDatabase()) {

[mmenke, 2013/10/29 18:23:34]

Should we just DCHECK on this instead, or is it an expected state in some cases?

[sclittle, 2013/10/29 23:40:45]

If the user ran out of disk space or something and we can't access the database,
we don't want to crash Chrome.

+    // Don't track anything if unable to access the database.
+    return;
+  }
+
+  if (is_precaching) {
+    if (was_cached && !precache_url_table_->HasURL(url)) {
+      // If a precache fetch came from the cache, and the user browsed the
+      // resource recently, then the precache fetch did nothing, so ignore it.

[mmenke, 2013/10/29 18:23:34]

I'm not following...The table should have the URL if the page was fetched in the
last 60 days, which isn't exactly recent, and doesn't mean we didn't update it
in the cache...does it?

[sclittle, 2013/10/29 23:40:45]

The URL table only keeps track of URLs that are in the cache because of
precaching, and would not be in the cache if not for precaching.

I've updated the comment to be clearer, and I've also updated the class comment
for PrecacheURLTable to be clearer about this.

[mmenke, 2013/10/30 18:29:08]

You seem to add everything we download to the table, as long as precaching is
enabled via a command line flag, no?  I see nothing to make is_pracaching true
only in the particular case that a request was initiated by the precache code.

[sclittle, 2013/10/30 20:29:48]

The purpose of the URL table is to allow us to identify when a fetch comes from
the cache thanks to precaching. Hopefully renaming the parameter from
"is_precaching" to "is_precache" can clear that up.

I've also added a comment to the precache_url_table_ member to hopefully help
clarify this.

+      return;
+    }
+
+    if (!was_cached) {
+      PrecacheStatisticsTable::PrecacheStatistics stats;
+      stats.precached_bytes = size;
+      precache_statistics_table_->IncreaseStatsForFetch(fetch_time, stats);

[mmenke, 2013/10/29 18:23:34]

Why are we only setting one of the fields?  I'm confused.  Also, why only when
it wasn't cached?

[sclittle, 2013/10/29 23:40:45]

All fields in the PrecacheStatistics default to 0.

Added comment.

+    }
+
+    precache_url_table_->AddURL(url, fetch_time);
+    return;
+  }
+
+  if (!was_cached) {
+    PrecacheStatisticsTable::PrecacheStatistics stats;
+    stats.downloaded_bytes = size;
+    if (is_cellular) {
+      stats.downloaded_bytes_cellular = size;
+    }
+    precache_statistics_table_->IncreaseStatsForFetch(fetch_time, stats);
+
+    // Since the fetch was over the network, delete any record of it having been
+    // prefetched from the URL table. If it had been precached, the resource
+    // must have expired.
+    precache_url_table_->DeleteURL(url);
+    return;
+  }
+
+  if (was_cached && precache_url_table_->HasURL(url)) {
+    PrecacheStatisticsTable::PrecacheStatistics stats;
+    stats.saved_bytes = size;
+    if (is_cellular) {
+      stats.saved_bytes_cellular = size;
+    }
+    precache_statistics_table_->IncreaseStatsForFetch(fetch_time, stats);

[mmenke, 2013/10/29 18:23:34]

When not precaching, we record stats if it wasn't cached, or it was cached and
it's in the precache_url_table_...Why the difference from the precaching case?

[sclittle, 2013/10/29 23:40:45]

Added comment.

+
+    precache_url_table_->DeleteURL(url);
+  }
+}
+
+bool PrecacheDatabase::CantAccessDatabase() {

[mmenke, 2013/10/29 18:23:34]

nit:  I think it's easy to miss the "T" when reading the name.  I suggest either
"IsDatabaseInaccessible()" or "IsDatabaseAccessible()" and negating the result
(I suggest "is" over "can" to make it even clearer it returns a book, but I'm
fine with "CanAccessDatabase" as well).  Open to other ideas.

[mmenke, 2013/10/29 18:23:34]

Can this be const?

[sclittle, 2013/10/29 23:40:45]

Done.

[sclittle, 2013/10/29 23:40:45]

Done.

+  DCHECK(CalledOnValidThread());
+  DCHECK(db_);
+
+  return !db_->is_open();
+}
+
+}  // namespace precache