Disk cache: Remove stats_histogram.

net/disk_cache/blockfile/stats.cc

 // Copyright (c) 2011 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 "net/disk_cache/blockfile/stats.h"
 
 #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"
 
 namespace {
 
 const int32 kDiskSignature = 0xF01427E0;
 
 struct OnDiskStats {
   int32 signature;
   int size;
@@ skipping 61 matching lines @@
     stats->signature = kDiskSignature;
   } else if (static_cast<unsigned int>(stats->size) != sizeof(*stats)) {
     size_t delta = sizeof(*stats) - static_cast<unsigned int>(stats->size);
     memset(reinterpret_cast<char*>(stats) + stats->size, 0, delta);
     stats->size = sizeof(*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) {
   OnDiskStats local_stats;
   OnDiskStats* stats = &local_stats;
   if (!num_bytes) {
     memset(stats, 0, sizeof(local_stats));
     local_stats.signature = kDiskSignature;
     local_stats.size = sizeof(local_stats);
   } else if (num_bytes >= static_cast<int>(sizeof(*stats))) {
     stats = reinterpret_cast<OnDiskStats*>(data);
     if (!VerifyStats(stats))
       return false;
   } else {
     return false;
   }
 
   storage_addr_ = address;
 
   memcpy(data_sizes_, stats->data_sizes, sizeof(data_sizes_));
   memcpy(counters_, stats->counters, sizeof(counters_));
 
   // Clean up old value.
   SetCounter(UNUSED, 0);
   return true;
 }
 
 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() {
   // If we have more than 512 bytes of counters, change kDiskSignature so we
   // don't overwrite something else (LoadStats must fail).
   COMPILE_ASSERT(sizeof(OnDiskStats) <= 256 * 2, use_more_blocks);
   return 256 * 2;
 }
 
 void Stats::ModifyStorageStats(int32 old_size, int32 new_size) {
@@ skipping 94 matching lines @@
   if (i > static_cast<size_t>(kDataSizesLength)) {
     NOTREACHED();
     i = kDataSizesLength;
   }
 
   i -= 17;
   n <<= i;
   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)
 //    1    [1024, 2048)
 //    2    [2048, 4096)
 //    3      [4K, 6K)
 //      ...
 //   10     [18K, 20K)
 //   11     [20K, 24K)
 //   12     [24k, 28K)
@@ skipping 33 matching lines @@
 int Stats::GetRatio(Counters hit, Counters miss) const {
   int64 ratio = GetCounter(hit) * 100;
   if (!ratio)
     return 0;
 
   ratio /= (GetCounter(hit) + GetCounter(miss));
   return static_cast<int>(ratio);
 }
 
 }  // namespace disk_cache