Reorganize histograms for persistence.

base/metrics/histogram.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.
 
 // Histogram is an object that aggregates statistics, and can summarize them in
 // various forms, including ASCII graphical, HTML, and numerically (as a
 // vector of numbers corresponding to each of the aggregating buckets).
 // See header file for details and examples.
 
 #include "base/metrics/histogram.h"
 
 #include <math.h>
 
 #include <algorithm>
 #include <string>
 
 #include "base/compiler_specific.h"
 #include "base/debug/alias.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/metrics/metrics_hashes.h"
 #include "base/metrics/sample_vector.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/pickle.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/lock.h"
 #include "base/values.h"
 
 namespace base {
 
@@ skipping 221 matching lines @@
 
   if (*minimum >= *maximum)
     return false;
   if (*bucket_count < 3)
     return false;
   if (*bucket_count > static_cast<size_t>(*maximum - *minimum + 2))
     return false;
   return true;
 }
 
+uint64_t Histogram::name_hash() const {
+  return samples_->id();
+}
+
 HistogramType Histogram::GetHistogramType() const {
   return HISTOGRAM;
 }
 
 bool Histogram::HasConstructionArguments(Sample expected_minimum,
                                          Sample expected_maximum,
                                          size_t expected_bucket_count) const {
   return ((expected_minimum == declared_min_) &&
           (expected_maximum == declared_max_) &&
           (expected_bucket_count == bucket_count()));
@@ skipping 18 matching lines @@
   samples_->Accumulate(value, count);
 
   FindAndRunCallback(value);
 }
 
 scoped_ptr<HistogramSamples> Histogram::SnapshotSamples() const {
   return SnapshotSampleVector();
 }
 
 void Histogram::AddSamples(const HistogramSamples& samples) {
+  DCHECK(samples.id() == 0 || samples.id() == samples_->id());

[Alexei Svitkine (slow), 2015/12/01 16:32:14]

This seems to be adding an implicit requirement to the API, which is leaking
knowledge of how this class is used to its implementation. Can this check be
moved to the caller?

[bcwhite, 2015/12/01 18:12:15]

I always imagined that "id" (if present, that is non-zero) would be unique or at
least associated.  The code should never try to merge samples from different
samples.

I could move it but I think it should be part of this supporting class.

[Alexei Svitkine (slow), 2015/12/01 18:27:14]

Well, my mental model is this method is just "add some values, where values are
coming from a container |samples|". So, it doesn't pre-suppose that this is used
for merging, for example. In that mental model, it seems the check doesn't make
sense.

If we want to make the method more opinionated, then it probably needs a name to
match and documentation to this factor. But I have a slight preference for
keeping this agnostic of the use case.

[bcwhite, 2015/12/01 19:33:44]

Okay, I can move the DCHECK no problem.

   samples_->Add(samples);
 }
 
 bool Histogram::AddSamplesFromPickle(PickleIterator* iter) {
   return samples_->AddFromPickle(iter);
 }
 
 // The following methods provide a graphical histogram display.
 void Histogram::WriteHTMLGraph(std::string* output) const {
   // TBD(jar) Write a nice HTML bar chart, with divs an mouse-overs etc.
@@ skipping 18 matching lines @@
 
 Histogram::Histogram(const std::string& name,
                      Sample minimum,
                      Sample maximum,
                      const BucketRanges* ranges)
   : HistogramBase(name),
     bucket_ranges_(ranges),
     declared_min_(minimum),
     declared_max_(maximum) {
   if (ranges)
-    samples_.reset(new SampleVector(ranges));
+    samples_.reset(new SampleVector(HashMetricName(name), ranges));
 }
 
 Histogram::~Histogram() {
 }
 
 bool Histogram::PrintEmptyBucket(size_t index) const {
   return true;
 }
 
 // Use the actual bucket widths (like a linear histogram) until the widths get
@@ skipping 36 matching lines @@
       histogram_name, declared_min, declared_max, bucket_count, flags);
 
   if (!ValidateRangeChecksum(*histogram, range_checksum)) {
     // The serialized histogram might be corrupted.
     return NULL;
   }
   return histogram;
 }
 
 scoped_ptr<SampleVector> Histogram::SnapshotSampleVector() const {
-  scoped_ptr<SampleVector> samples(new SampleVector(bucket_ranges()));
+  scoped_ptr<SampleVector> samples(
+      new SampleVector(HashMetricName(histogram_name()), bucket_ranges()));

[Alexei Svitkine (slow), 2015/12/01 16:32:14]

Can this use the id from samples_ instead of re-computing it?

[bcwhite, 2015/12/01 18:12:15]

Done.

[Alexei Svitkine (slow), 2015/12/01 18:27:14]

Doesn't seem like you uploaded a new patch set?

[bcwhite, 2015/12/01 19:33:44]

Not yet.  So far all the changes have been cosmetic.

   samples->Add(*samples_);
   return samples;
 }
 
 void Histogram::WriteAsciiImpl(bool graph_it,
                                const std::string& newline,
                                std::string* output) const {
   // Get local (stack) copies of all effectively volatile class data so that we
   // are consistent across our output activities.
   scoped_ptr<SampleVector> snapshot = SnapshotSampleVector();
@@ skipping 486 matching lines @@
 
   BucketRanges* bucket_ranges = new BucketRanges(ranges.size());
   for (size_t i = 0; i < ranges.size(); i++) {
     bucket_ranges->set_range(i, ranges[i]);
   }
   bucket_ranges->ResetChecksum();
   return bucket_ranges;
 }
 
 }  // namespace base