Support "shared" histograms between processes.

base/metrics/statistics_recorder.cc

Index: base/metrics/statistics_recorder.cc
diff --git a/base/metrics/statistics_recorder.cc b/base/metrics/statistics_recorder.cc
index 15e48d8ae622d3572467c93800a0b4caf6547c21..b9897027788152cd9ecda2dfed710fa9a8ba1702 100644
--- a/base/metrics/statistics_recorder.cc
+++ b/base/metrics/statistics_recorder.cc
@@ -24,6 +24,22 @@ base::LazyInstance<base::StatisticsRecorder>::Leaky g_statistics_recorder_ =
 
 namespace base {
 
+StatisticsRecorder::HistogramIterator&
+StatisticsRecorder::HistogramIterator::operator++() {
+  const HistogramMap::iterator histograms_end = histograms_->end();
+  while (iter_ != histograms_end) {
+    ++iter_;
+    if (iter_ == histograms_end)
+      break;
+    if (!include_persistent_ && (iter_->second->flags() &
+                                 HistogramBase::kIsPersistent)) {
+      continue;
+    }
+    break;
+  }
+  return *this;
+}
+
 // static
 void StatisticsRecorder::Initialize() {
   // Ensure that an instance of the StatisticsRecorder object is created.
@@ -59,7 +75,8 @@ HistogramBase* StatisticsRecorder::RegisterOrDeleteDuplicate(
       histogram_to_return = histogram;
     } else {
       const std::string& name = histogram->histogram_name();
-      uint64_t name_hash = histogram->name_hash();
+      const uint64_t name_hash = histogram->name_hash();
+      DCHECK_NE(0, name_hash);
       HistogramMap::iterator it = histograms_->find(name_hash);
       if (histograms_->end() == it) {
         (*histograms_)[name_hash] = histogram;
@@ -255,7 +272,7 @@ bool StatisticsRecorder::SetCallback(
     return false;
   callbacks_->insert(std::make_pair(name, cb));
 
-  HistogramMap::iterator it = histograms_->find(HashMetricName(name));
+  auto it = histograms_->find(HashMetricName(name));
   if (it != histograms_->end()) {
     DCHECK_EQ(name, it->second->histogram_name()) << "hash collision";
     it->second->SetFlags(HistogramBase::kCallbackExists);
@@ -275,7 +292,7 @@ void StatisticsRecorder::ClearCallback(const std::string& name) {
   callbacks_->erase(name);
 
   // We also clear the flag from the histogram (if it exists).
-  HistogramMap::iterator it = histograms_->find(HashMetricName(name));
+  auto it = histograms_->find(HashMetricName(name));
   if (it != histograms_->end()) {
     DCHECK_EQ(name, it->second->histogram_name()) << "hash collision";
     it->second->ClearFlags(HistogramBase::kCallbackExists);
@@ -296,7 +313,18 @@ StatisticsRecorder::OnSampleCallback StatisticsRecorder::FindCallback(
                                                 : OnSampleCallback();
 }
 
-// private static
+// static
+StatisticsRecorder::HistogramIterator StatisticsRecorder::begin(
+    bool include_persistent) {

[Alexei Svitkine (slow), 2016/01/14 16:43:13]

It would be better to make this an enum so that it's clear what's happening from
the callers side.

Then, you can actually have 3 values that all make logical sense:

PERSISTENT_HISTOGRAMS = 1,
NON_PERSISTENT_HISTOGRAM = 2,
ALL_HISTOGRAMS = PERSISTENT_HISTOGRAMS | NON_PERSISTENT_HISTOGRAM

[bcwhite, 2016/01/14 19:20:25]

Sure, though I don't see the use-case of that.  You either want:

1) All histograms created locally (true), or
2) All histograms created locally but not reported elsewhere (false)

I would actually prefer to remove the parameter completely and just do the
"false" case but it's more efficient to find shared histograms here than by
independently iterating over a persistent memory segment.  Once reporting
directly from persistent data is complete (a future optimization), I'll likely
go that way.

+  return HistogramIterator(histograms_->begin(), include_persistent);
+}
+
+// static
+StatisticsRecorder::HistogramIterator StatisticsRecorder::end() {
+  return HistogramIterator(histograms_->end(), true);

[Alexei Svitkine (slow), 2016/01/14 16:43:14]

Why is it correct to pass in true here, rather than relying on a param?

[bcwhite, 2016/01/14 19:20:26]

The second parameter defines what histograms are found within an allocator but
end() doesn't return a valid allocator so the value of the second parameter is
unimportant.

+}
+
+// static
 void StatisticsRecorder::GetSnapshot(const std::string& query,
                                      Histograms* snapshot) {
   if (lock_ == NULL)