Reorganize histograms for persistence.

base/metrics/histogram.cc

Index: base/metrics/histogram.cc
diff --git a/base/metrics/histogram.cc b/base/metrics/histogram.cc
index 1bbcab7023a8be9e45e3ca72fb631abee31ef4a6..5e24e005fabb23560870e6c8914859d263553ef4 100644
--- a/base/metrics/histogram.cc
+++ b/base/metrics/histogram.cc
@@ -18,6 +18,7 @@
 #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"
@@ -259,6 +260,10 @@ bool Histogram::InspectConstructionArguments(const std::string& name,
   return true;
 }
 
+uint64_t Histogram::name_hash() const {
+  return samples_->id();
+}
+
 HistogramType Histogram::GetHistogramType() const {
   return HISTOGRAM;
 }
@@ -297,6 +302,7 @@ scoped_ptr<HistogramSamples> Histogram::SnapshotSamples() const {
 }
 
 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);
 }
 
@@ -335,7 +341,7 @@ Histogram::Histogram(const std::string& name,
     declared_min_(minimum),
     declared_max_(maximum) {
   if (ranges)
-    samples_.reset(new SampleVector(ranges));
+    samples_.reset(new SampleVector(HashMetricName(name), ranges));
 }
 
 Histogram::~Histogram() {
@@ -392,7 +398,8 @@ HistogramBase* Histogram::DeserializeInfoImpl(PickleIterator* iter) {
 }
 
 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;
 }