Run SR tests both with and without persistent allocator.

base/metrics/statistics_recorder.cc

 // Copyright (c) 2012 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/statistics_recorder.h"
 
 #include <memory>
 
 #include "base/at_exit.h"
 #include "base/debug/leak_annotations.h"
@@ skipping 19 matching lines @@
 }
 
 }  // namespace
 
 namespace base {
 
 StatisticsRecorder::HistogramIterator::HistogramIterator(
     const HistogramMap::iterator& iter, bool include_persistent)
     : iter_(iter),
       include_persistent_(include_persistent) {
+  // The starting location could point to a persistent histogram when such
+  // is not wanted. If so, skip it.
+  if (!include_persistent_ && iter_ != histograms_->end() &&

[Alexei Svitkine (slow), 2016/04/11 14:51:34]

Shouldn't this be a loop? What if the second entry is also persistent? (If so,
also add a test.)

[bcwhite, 2016/04/12 00:32:02]

operator++ already includes a loop that skips as appropriate.

[Alexei Svitkine (slow), 2016/04/12 21:02:08]

Makes sense. Maybe add a comment so it's clear when reading this code.

[bcwhite, 2016/04/13 12:59:45]

Done.

+      (iter_->second->flags() & HistogramBase::kIsPersistent)) {
+    operator++();
+  }
 }
 
 StatisticsRecorder::HistogramIterator::HistogramIterator(
     const HistogramIterator& rhs)
     : iter_(rhs.iter_),
       include_persistent_(rhs.include_persistent_) {
 }
 
 StatisticsRecorder::HistogramIterator::~HistogramIterator() {}
 
@@ skipping 230 matching lines @@
 
   for (const auto& entry : *ranges_) {
     for (const auto& range_entry : *entry.second) {
       output->push_back(range_entry);
     }
   }
 }
 
 // static
 HistogramBase* StatisticsRecorder::FindHistogram(base::StringPiece name) {
-  if (lock_ == NULL)
-    return NULL;
-
-  // Import histograms from known persistent storage. Histograms could have
-  // been added by other processes and they must be fetched and recognized
-  // locally. If the persistent memory segment is not shared between processes,
-  // this call does nothing.
   // This must be called *before* the lock is acquired below because it will
   // call back into this object to register histograms. Those called methods
   // will acquire the lock at that time.
-  GlobalHistogramAllocator* allocator = GlobalHistogramAllocator::Get();
-  if (allocator)
-    allocator->ImportHistogramsToStatisticsRecorder();
+  ImportGlobalPersistentHistograms();
 
+  if (lock_ == NULL)
+    return NULL;
   base::AutoLock auto_lock(*lock_);
   if (histograms_ == NULL)
     return NULL;
 
   HistogramMap::iterator it = histograms_->find(name);
   if (histograms_->end() == it)
     return NULL;
   return it->second;
 }
 
 // static
 StatisticsRecorder::HistogramIterator StatisticsRecorder::begin(
     bool include_persistent) {
+  ImportGlobalPersistentHistograms();
   return HistogramIterator(histograms_->begin(), include_persistent);
 }
 
 // static
 StatisticsRecorder::HistogramIterator StatisticsRecorder::end() {
   return HistogramIterator(histograms_->end(), true);
 }
 
 // static
 void StatisticsRecorder::GetSnapshot(const std::string& query,
@@ skipping 89 matching lines @@
   // can't "delete" it; call the destructor explicitly.
   DCHECK(g_statistics_recorder_.private_instance_);
   g_statistics_recorder_.Get().~StatisticsRecorder();
 
   // Now the ugly part. There's no official way to release a LazyInstance once
   // created so it's necessary to clear out an internal variable which
   // shouldn't be publicly visible but is for initialization reasons.
   g_statistics_recorder_.private_instance_ = 0;
 }
 
+// static
+void StatisticsRecorder::ImportGlobalPersistentHistograms() {
+  if (lock_ == NULL)
+    return;
+
+  // Import histograms from known persistent storage. Histograms could have
+  // been added by other processes and they must be fetched and recognized
+  // locally. If the persistent memory segment is not shared between processes,
+  // this call does nothing.
+  GlobalHistogramAllocator* allocator = GlobalHistogramAllocator::Get();
+  if (allocator)
+    allocator->ImportHistogramsToStatisticsRecorder();
+}
+
 // This singleton instance should be started during the single threaded portion
 // of main(), and hence it is not thread safe.  It initializes globals to
 // provide support for all future calls.
 StatisticsRecorder::StatisticsRecorder() {
   if (lock_ == NULL) {
     // This will leak on purpose. It's the only way to make sure we won't race
     // against the static uninitialization of the module while one of our
     // static methods relying on the lock get called at an inappropriate time
     // during the termination phase. Since it's a static data member, we will
     // leak one per process, which would be similar to the instance allocated
@@ skipping 49 matching lines @@
 // static
 StatisticsRecorder::HistogramMap* StatisticsRecorder::histograms_ = NULL;
 // static
 StatisticsRecorder::CallbackMap* StatisticsRecorder::callbacks_ = NULL;
 // static
 StatisticsRecorder::RangesMap* StatisticsRecorder::ranges_ = NULL;
 // static
 base::Lock* StatisticsRecorder::lock_ = NULL;
 
 }  // namespace base