Improved iterator for persistent memory allocator.

base/metrics/persistent_histogram_allocator.cc

 // Copyright 2016 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.
 
 #include "base/metrics/persistent_histogram_allocator.h"
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
@@ skipping 91 matching lines @@
   PersistentMemoryAllocator::Reference counts_ref;
   HistogramSamples::Metadata samples_metadata;
   HistogramSamples::Metadata logged_metadata;
 
   // Space for the histogram name will be added during the actual allocation
   // request. This must be the last field of the structure. A zero-size array
   // or a "flexible" array would be preferred but is not (yet) valid C++.
   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);
 }
 
 void PersistentHistogramAllocator::UpdateTrackingHistograms() {
   memory_allocator_->UpdateTrackingHistograms();
 }
 
 // static
 HistogramBase*
@@ skipping 66 matching lines @@
 PersistentHistogramAllocator::ReleaseGlobalAllocatorForTesting() {
   PersistentHistogramAllocator* histogram_allocator = g_allocator;
   if (!histogram_allocator)
     return nullptr;
   PersistentMemoryAllocator* memory_allocator =
       histogram_allocator->memory_allocator();
 
   // 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);
+  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);
-    }
+    // 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;
   return make_scoped_ptr(histogram_allocator);
 };
 
 // static
 void PersistentHistogramAllocator::CreateGlobalAllocatorOnPersistentMemory(
     void* base,
     size_t size,
@@ skipping 167 matching lines @@
   size_t length = memory_allocator_->GetAllocSize(ref);
   if (!histogram_data ||
       reinterpret_cast<char*>(histogram_data)[length - 1] != '\0') {
     RecordCreateHistogramResult(CREATE_HISTOGRAM_INVALID_METADATA);
     NOTREACHED();
     return nullptr;
   }
   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
   // be marked as "iterable" in order to be found by other processes.
   if (registered)
     memory_allocator_->MakeIterable(ref);
   // If it wasn't registered then a race condition must have caused
   // two to be created. The allocator does not support releasing the
   // acquired memory so just change the type to be empty.
   else
@@ skipping 107 matching lines @@
   return nullptr;
 }
 
 // static
 void PersistentHistogramAllocator::ImportGlobalHistograms() {
   // The lock protects against concurrent access to the iterator and is created
   // in a thread-safe manner when needed.
   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
     // is recognized here then the histogram will be created and be ignored
     // when it is detected as a duplicate by the statistics-recorder. This
     // simple check reduces the time of creating persistent histograms by
     // about 40%.
     Reference last_created =
         subtle::NoBarrier_Load(&g_allocator->last_created_);
 
     while (true) {
       scoped_ptr<HistogramBase> histogram =
-          g_allocator->GetNextHistogramWithIgnore(&iter, last_created);
+          iter.GetNextWithIgnore(last_created);
       if (!histogram)
         break;
       StatisticsRecorder::RegisterOrDeleteDuplicate(histogram.release());
     }
   }
 }
 
 }  // namespace base