Improved iterator for persistent memory allocator.

base/metrics/persistent_histogram_allocator.cc

Index: base/metrics/persistent_histogram_allocator.cc
diff --git a/base/metrics/persistent_histogram_allocator.cc b/base/metrics/persistent_histogram_allocator.cc
index 6006d31fbee70031acb901fb6a0f04a944f5ff04..ab957a69ec5e85c176749f1399f70e7e0e54f0fc 100644
--- a/base/metrics/persistent_histogram_allocator.cc
+++ b/base/metrics/persistent_histogram_allocator.cc
@@ -109,16 +109,26 @@ struct PersistentHistogramAllocator::PersistentHistogramData {
   char name[1];
 };
 
+PersistentHistogramAllocator::Iterator::Iterator(
+    PersistentHistogramAllocator* allocator)
+    : allocator_(allocator), memory_iter_(allocator->memory_allocator()) {}
+
+scoped_ptr<HistogramBase>
+PersistentHistogramAllocator::Iterator::GetNextWithIgnore(Reference ignore) {
+  PersistentMemoryAllocator::Reference ref;

[Ilya Sherman, 2016/03/24 02:03:58]

nit: s/PersistentMemoryAllocator::Reference/Reference

[bcwhite, 2016/03/24 14:22:34]

PHA::Reference is implemented as a PMA::Reference but they're not conceptually
the same thing.  In this method, I'm iterating over a PMA so want to use
PMA::Reference.  I'll add a comment about this.

+  while ((ref = memory_iter_.GetNextOfType(kTypeIdHistogram)) != 0) {
+    if (ref != ignore)
+      return allocator_->GetHistogram(ref);
+  }
+  return nullptr;
+}
+
 PersistentHistogramAllocator::PersistentHistogramAllocator(
     scoped_ptr<PersistentMemoryAllocator> memory)
     : memory_allocator_(std::move(memory)) {}
 
 PersistentHistogramAllocator::~PersistentHistogramAllocator() {}
 
-void PersistentHistogramAllocator::CreateIterator(Iterator* iter) {
-  memory_allocator_->CreateIterator(&iter->memory_iter);
-}
-
 void PersistentHistogramAllocator::CreateTrackingHistograms(StringPiece name) {
   memory_allocator_->CreateTrackingHistograms(name);
 }
@@ -205,27 +215,23 @@ PersistentHistogramAllocator::ReleaseGlobalAllocatorForTesting() {
   // Before releasing the memory, it's necessary to have the Statistics-
   // Recorder forget about the histograms contained therein; otherwise,
   // some operations will try to access them and the released memory.
-  PersistentMemoryAllocator::Iterator iter;
+  PersistentMemoryAllocator::Iterator iter(memory_allocator);
   PersistentMemoryAllocator::Reference ref;
-  uint32_t type_id;
-  memory_allocator->CreateIterator(&iter);
-  while ((ref = memory_allocator->GetNextIterable(&iter, &type_id)) != 0) {
-    if (type_id == kTypeIdHistogram) {
-      PersistentHistogramData* histogram_data =
-          memory_allocator->GetAsObject<PersistentHistogramData>(
-              ref, kTypeIdHistogram);
-      DCHECK(histogram_data);
-      StatisticsRecorder::ForgetHistogramForTesting(histogram_data->name);
-
-      // If a test breaks here then a memory region containing a histogram
-      // actively used by this code is being released back to the test.
-      // If that memory segment were to be deleted, future calls to create
-      // persistent histograms would crash. To avoid this, have the test call
-      // the method GetCreateHistogramResultHistogram() *before* setting
-      // the (temporary) memory allocator via SetGlobalAllocator() so that
-      // histogram is instead allocated from the process heap.
-      DCHECK_NE(kResultHistogram, histogram_data->name);
-    }
+  while ((ref = iter.GetNextOfType(kTypeIdHistogram)) != 0) {
+    PersistentHistogramData* histogram_data =
+        memory_allocator->GetAsObject<PersistentHistogramData>(
+            ref, kTypeIdHistogram);
+    DCHECK(histogram_data);
+    StatisticsRecorder::ForgetHistogramForTesting(histogram_data->name);
+
+    // If a test breaks here then a memory region containing a histogram
+    // actively used by this code is being released back to the test.
+    // If that memory segment were to be deleted, future calls to create
+    // persistent histograms would crash. To avoid this, have the test call
+    // the method GetCreateHistogramResultHistogram() *before* setting
+    // the (temporary) memory allocator via SetGlobalAllocator() so that
+    // histogram is instead allocated from the process heap.
+    DCHECK_NE(kResultHistogram, histogram_data->name);
   }
 
   g_allocator = nullptr;
@@ -413,21 +419,6 @@ scoped_ptr<HistogramBase> PersistentHistogramAllocator::GetHistogram(
   return CreateHistogram(histogram_data);
 }
 
-scoped_ptr<HistogramBase>
-PersistentHistogramAllocator::GetNextHistogramWithIgnore(Iterator* iter,
-                                                         Reference ignore) {
-  PersistentMemoryAllocator::Reference ref;
-  uint32_t type_id;
-  while ((ref = memory_allocator_->GetNextIterable(&iter->memory_iter,
-                                                   &type_id)) != 0) {
-    if (ref == ignore)
-      continue;
-    if (type_id == kTypeIdHistogram)
-      return GetHistogram(ref);
-  }
-  return nullptr;
-}
-
 void PersistentHistogramAllocator::FinalizeHistogram(Reference ref,
                                                      bool registered) {
   // If the created persistent histogram was registered then it needs to
@@ -555,15 +546,17 @@ void PersistentHistogramAllocator::ImportGlobalHistograms() {
   static base::LazyInstance<base::Lock>::Leaky lock = LAZY_INSTANCE_INITIALIZER;
 
   if (g_allocator) {
-    // TODO(bcwhite): Investigate a lock-free, thread-safe iterator.
+    // Even though the iterator is thread-safe, the rest of this code is not
+    // and has to be protected by a lock. Otherwise, there is no guarantee
+    // that multiple threads would process histograms in the same order
+    // they get returned by the iterator. That is important because it
+    // guarantees all processes will always have the same state.
     base::AutoLock auto_lock(lock.Get());
 
     // Each call resumes from where it last left off so a persistant iterator
     // is needed. This class has a constructor so even the definition has to
     // be protected by the lock in order to be thread-safe.
-    static Iterator iter;
-    if (iter.is_clear())
-      g_allocator->CreateIterator(&iter);
+    static Iterator iter(g_allocator);
 
     // Skip the import if it's the histogram that was last created. Should a
     // race condition cause the "last created" to be overwritten before it
@@ -576,7 +569,7 @@ void PersistentHistogramAllocator::ImportGlobalHistograms() {
 
     while (true) {
       scoped_ptr<HistogramBase> histogram =
-          g_allocator->GetNextHistogramWithIgnore(&iter, last_created);
+          iter.GetNextWithIgnore(last_created);
       if (!histogram)
         break;
       StatisticsRecorder::RegisterOrDeleteDuplicate(histogram.release());