Disk cache: Remove stats_histogram.

net/disk_cache/blockfile/stats.cc

Index: net/disk_cache/blockfile/stats.cc
diff --git a/net/disk_cache/blockfile/stats.cc b/net/disk_cache/blockfile/stats.cc
index eaaf75aa70d3c57c2165ddbcc9bfdc4a7f5d3b3c..0e0649baa494263710a00deafbc75f7c1c62a216 100644
--- a/net/disk_cache/blockfile/stats.cc
+++ b/net/disk_cache/blockfile/stats.cc
@@ -6,7 +6,11 @@
 
 #include "base/format_macros.h"
 #include "base/logging.h"
+#include "base/metrics/bucket_ranges.h"
+#include "base/metrics/histogram.h"
 #include "base/metrics/histogram_samples.h"
+#include "base/metrics/sample_vector.h"
+#include "base/metrics/statistics_recorder.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 
@@ -88,13 +92,10 @@ bool VerifyStats(OnDiskStats* stats) {
   return true;
 }
 
-Stats::Stats() : size_histogram_(NULL) {
+Stats::Stats() {
 }
 
 Stats::~Stats() {
-  if (size_histogram_) {
-    size_histogram_->Disable();
-  }
 }
 
 bool Stats::Init(void* data, int num_bytes, Addr address) {
@@ -123,17 +124,30 @@ bool Stats::Init(void* data, int num_bytes, Addr address) {
 }
 
 void Stats::InitSizeHistogram() {
-  // It seems impossible to support this histogram for more than one
-  // simultaneous objects with the current infrastructure.
+  // Only generate this histogram for the main cache.
   static bool first_time = true;
-  if (first_time) {
-    first_time = false;
-    if (!size_histogram_) {
-      // Stats may be reused when the cache is re-created, but we want only one
-      // histogram at any given time.
-      size_histogram_ = StatsHistogram::FactoryGet("DiskCache.SizeStats", this);
-    }
+  if (!first_time)

[Alexei Svitkine (slow), 2014/05/27 18:59:47]

It seems that in the new version of the code, you're only recording the stats
once on start up (when this function is called), whereas the old code I believe
may be doing whenever SnapshotSamples() is called. (I'm not super sure what that
actually means - perhaps it ends up being once every UMA upload?).

If the old code did indeed do it once every UMA upload, then this can be
supported too in the new code by hooking in to
ChromeMetricsServiceClient::CollectFinalMetrics() - and we do this for other
things like memory stats, etc.

[rvargas (doing something else), 2014/05/27 19:36:38]

The logic is pretty much the same: The histogram is generated only once and it
is not updated after that. The old code was changing the uma flag from 1 to 0
after the first snapshot so it was only uploaded once (for some reason I don't
fully remember ATM), but the result on the server should be the same as the next
snapshot will see a delta of 0 and do nothing. The only difference would be if
there is memory corruption and the data stored by base::Histogram changes 'cause
of that. The old code seems immune to that.

+    return;
+
+  first_time = false;
+  int min = 1;
+  int max = 64 * 1024;
+  int num_buckets = 75;
+  base::BucketRanges ranges(num_buckets + 1);
+  base::Histogram::InitializeBucketRanges(min, max, &ranges);
+
+  base::HistogramBase* stats_histogram = base::Histogram::FactoryGet(
+      "DiskCache.SizeStats2", min, max, num_buckets,

[Alexei Svitkine (slow), 2014/05/27 18:59:47]

Please add a histograms.xml entry for this histogram with yourself as owner in
the same CL.

[rvargas (doing something else), 2014/05/27 19:36:38]

Done.

+      base::HistogramBase::kUmaTargetedHistogramFlag);
+
+  base::SampleVector samples(&ranges);
+  for (int i = 0; i < kDataSizesLength; i++) {
+    if (data_sizes_[i] < 0)

[Alexei Svitkine (slow), 2014/05/27 18:59:47]

This could use a comment.

[rvargas (doing something else), 2014/05/27 19:36:38]

Done.

+      data_sizes_[i] = 0;

[Alexei Svitkine (slow), 2014/05/27 18:59:47]

Does it make sense for this to actually change data_sizes_? Or would it be
better to simply skip the Accumulate() call in this case? (In the old code, you
don't actually change data_sizes).

[rvargas (doing something else), 2014/05/27 19:36:38]

That's something that I saw when testing with my development profile... because
it crashes a lot, there was a bunch of negative numbers in there so it makes
sense to fix them at some point, and this seems like the perfect time to do it
(this is the only place where we actually go through the stored data).

+
+    samples.Accumulate(GetBucketRange(i) / 1024, data_sizes_[i]);
   }
+  stats_histogram->AddSamples(samples);
 }
 
 int Stats::StorageSize() {
@@ -248,15 +262,6 @@ int Stats::GetBucketRange(size_t i) const {
   return n;
 }
 
-void Stats::Snapshot(base::HistogramSamples* samples) const {
-  for (int i = 0; i < kDataSizesLength; i++) {
-    int count = data_sizes_[i];
-    if (count < 0)
-      count = 0;
-    samples->Accumulate(GetBucketRange(i), count);
-  }
-}
-
 // The array will be filled this way:
 //  index      size
 //    0       [0, 1024)