Support "shared" histograms between processes.

components/metrics/metrics_service.cc

Index: components/metrics/metrics_service.cc
diff --git a/components/metrics/metrics_service.cc b/components/metrics/metrics_service.cc
index 50545b3f68d2758e7307dac7edd23cb7d08a35e1..a5ed883cc8b2191df13bf51515982bd9babf6921 100644
--- a/components/metrics/metrics_service.cc
+++ b/components/metrics/metrics_service.cc
@@ -133,6 +133,7 @@
 #include "base/location.h"
 #include "base/metrics/histogram_base.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram_persistence.h"
 #include "base/metrics/histogram_samples.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/metrics/statistics_recorder.h"
@@ -245,6 +246,40 @@ bool ShouldUploadLog() {
 
 }  // namespace
 
+MetricsService::PersistentHistogramIterator::PersistentHistogramIterator(
+    AllocatorSet& allocators,
+    AllocatorSet::iterator pos)
+    : allocators_(allocators),
+      allocator_iter_(pos) {
+  if (pos != allocators_.end()) {
+    (*allocator_iter_)->CreateIterator(&histogram_iter_);
+    // Have to call ++ to advance iterator to the first persistent histogram.
+    operator++();
+  }
+}
+
+MetricsService::PersistentHistogramIterator&
+MetricsService::PersistentHistogramIterator::operator++() {
+  if (allocator_iter_ != allocators_.end()) {
+    for (;;) {
+      base::HistogramBase* h = base::GetNextPersistentHistogram(
+          *allocator_iter_, &histogram_iter_);
+      if (h) {
+        current_histogram_ = new HistogramPointer(h);
+        break;
+      }
+      allocator_iter_++;
+      if (allocator_iter_ == allocators_.end()) {
+        histogram_iter_.clear();  // Clear so it matches end() iterator.
+        current_histogram_ = nullptr;
+        break;
+      }
+      (*allocator_iter_)->CreateIterator(&histogram_iter_);
+    }
+  }
+  return *this;
+}
+
 // static
 MetricsService::ShutdownCleanliness MetricsService::clean_shutdown_status_ =
     MetricsService::CLEANLY_SHUTDOWN;
@@ -541,6 +576,18 @@ void MetricsService::PushExternalLog(const std::string& log) {
   log_manager_.StoreLog(log, MetricsLog::ONGOING_LOG);
 }
 
+void MetricsService::AddPersistentMemorySegment(
+    base::PersistentMemoryAllocator* allocator) {
+  DCHECK(allocators_.find(allocator) == allocators_.end());
+  allocators_.insert(allocator);
+}
+
+void MetricsService::RemovePersistentMemorySegment(
+    base::PersistentMemoryAllocator* allocator) {
+  DCHECK(allocators_.find(allocator) != allocators_.end());
+  allocators_.erase(allocator);
+}
+
 //------------------------------------------------------------------------------
 // private methods
 //------------------------------------------------------------------------------
@@ -740,6 +787,11 @@ void MetricsService::CloseCurrentLog() {
     OpenNewLog();  // Start trivial log to hold our histograms.
   }
 
+  base::PersistentMemoryAllocator* allocator =

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

Add a comment about this block, as it's not clear what it's for.

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

Done.

+      base::GetPersistentHistogramMemoryAllocator();
+  if (allocator)
+    allocator->UpdateStaticHistograms();
+
   // Put incremental data (histogram deltas, and realtime stats deltas) at the
   // end of all log transmissions (initial log handles this separately).
   // RecordIncrementalStabilityElements only exists on the derived
@@ -1100,12 +1152,17 @@ void MetricsService::RecordCurrentEnvironment(MetricsLog* log) {
 void MetricsService::RecordCurrentHistograms() {
   DCHECK(log_manager_.current_log());
   histogram_snapshot_manager_.PrepareDeltas(
+      base::StatisticsRecorder::begin(true), base::StatisticsRecorder::end(),
+      base::Histogram::kNoFlags, base::Histogram::kUmaTargetedHistogramFlag);
+  histogram_snapshot_manager_.PrepareDeltas(

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

Can the metrics service changes be in a separate CL?

Correct me if I'm wrong, but it doesn't seem like anything is being hooked up to
metrics service here anyway - even if the trial is on, we're not going to get
AddPersistentMemorySegment() called yet until we have something coming from a
different source than the browser process. In which case, I suggest including
those changes with the first CL that starts using this - for example the CL that
adds the collection to setup.exe.

(In which case, probably all the iterator changes can also be part of that CL.)

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

It could be moved but it seems to be more "in context" here since this whole CL
is about adding persistent support.

[Alexei Svitkine (slow), 2016/01/14 21:53:41]

I disagree. Given that this CL is not adding any external persistent histograms,
I think it's cleaner to have this set of changes with the CL that does.

[bcwhite, 2016/01/15 15:24:38]

I understand the point but pushing this to the setup-metrics CL is going to
complicate the CL between it and this one (the "merge histograms" CL) since it
expands on this somewhat.  It's also going to cost me a day of time to do yet
another split and fix my CL chain in git.

[Alexei Svitkine (slow), 2016/01/15 15:43:47]

APII'm not fully sold on the iterator changes and would like to not have that
discussion block landing this CL, so would like these changes to move to a
follow-up CL - preferably the one that also adds external persistent metrics so
I can reviewer the new API in the same CL that it's used. I'm hoping we can
agree on the remaining parts on this CL and land it early next week and start
experimenting - while in parallel discussing this other part on a separate CL.

[bcwhite, 2016/01/15 19:23:41]

Done.

+      persistent_begin(), persistent_end(),
       base::Histogram::kNoFlags, base::Histogram::kUmaTargetedHistogramFlag);
 }
 
 void MetricsService::RecordCurrentStabilityHistograms() {
   DCHECK(log_manager_.current_log());
   histogram_snapshot_manager_.PrepareDeltas(
+      base::StatisticsRecorder::begin(true), base::StatisticsRecorder::end(),
       base::Histogram::kNoFlags, base::Histogram::kUmaStabilityHistogramFlag);
 }
 
@@ -1144,4 +1201,12 @@ void MetricsService::SkipAndDiscardUpload() {
   log_upload_in_progress_ = false;
 }
 
+MetricsService::PersistentHistogramIterator MetricsService::persistent_begin() {
+  return PersistentHistogramIterator(allocators_, allocators_.begin());
+}
+
+MetricsService::PersistentHistogramIterator MetricsService::persistent_end() {
+  return PersistentHistogramIterator(allocators_, allocators_.end());
+}
+
 }  // namespace metrics