Run SR tests both with and without persistent allocator.

base/metrics/statistics_recorder_unittest.cc

Index: base/metrics/statistics_recorder_unittest.cc
diff --git a/base/metrics/statistics_recorder_unittest.cc b/base/metrics/statistics_recorder_unittest.cc
index aede0b11fbfd56f8b074024250fc56638dc1a94b..2bfb2c94e990e747b7476d03a53686e9751db976 100644
--- a/base/metrics/statistics_recorder_unittest.cc
+++ b/base/metrics/statistics_recorder_unittest.cc
@@ -19,20 +19,28 @@
 
 namespace base {
 
-class StatisticsRecorderTest : public testing::Test {
+class StatisticsRecorderTest : public testing::TestWithParam<bool> {
  protected:
-  void SetUp() override {
+  const int32_t kAllocatorMemorySize = 64 << 10;  // 64 KiB
+
+  StatisticsRecorderTest() : use_persistent_histogram_allocator_(GetParam()) {
     // Get this first so it never gets created in persistent storage and will
     // not appear in the StatisticsRecorder after it is re-initialized.
     PersistentHistogramAllocator::GetCreateHistogramResultHistogram();
+
     // Each test will have a clean state (no Histogram / BucketRanges
     // registered).
     InitializeStatisticsRecorder();
+
+    // Use persistent memory for histograms if so indicated by test parameter.
+    if (use_persistent_histogram_allocator_)

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

Nit: {}

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

Done.

+      GlobalHistogramAllocator::CreateWithLocalMemory(
+          kAllocatorMemorySize, 0, "StatisticsRecorderTest");
   }
 
-  void TearDown() override {
-    UninitializeStatisticsRecorder();
+  ~StatisticsRecorderTest() override {
     GlobalHistogramAllocator::ReleaseForTesting();
+    UninitializeStatisticsRecorder();
   }
 
   void InitializeStatisticsRecorder() {
@@ -61,10 +69,26 @@ class StatisticsRecorderTest : public testing::Test {
     delete histogram;
   }
 
+  int CountIterableHistograms(StatisticsRecorder::HistogramIterator& iter) {

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

Nit: Non-const ref as function params are discouraged.

You can just pass by ptr and deref in the body.

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

Done.

+    int count = 0;
+    for (; iter != StatisticsRecorder::end(); ++iter) {
+      ++count;
+    }
+    return count;
+  }
+
+  const bool use_persistent_histogram_allocator_;
+
   std::unique_ptr<StatisticsRecorder> statistics_recorder_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(StatisticsRecorderTest);
 };
 
-TEST_F(StatisticsRecorderTest, NotInitialized) {
+// Run all HistogramTest cases with both heap and persistent memory.
+INSTANTIATE_TEST_CASE_P(Allocator, StatisticsRecorderTest, testing::Bool());
+
+TEST_P(StatisticsRecorderTest, NotInitialized) {
   UninitializeStatisticsRecorder();
 
   ASSERT_FALSE(StatisticsRecorder::IsActive());
@@ -92,7 +116,7 @@ TEST_F(StatisticsRecorderTest, NotInitialized) {
   EXPECT_EQ(0u, registered_ranges.size());
 }
 
-TEST_F(StatisticsRecorderTest, RegisterBucketRanges) {
+TEST_P(StatisticsRecorderTest, RegisterBucketRanges) {
   std::vector<const BucketRanges*> registered_ranges;
 
   BucketRanges* ranges1 = new BucketRanges(3);
@@ -130,7 +154,7 @@ TEST_F(StatisticsRecorderTest, RegisterBucketRanges) {
   ASSERT_EQ(2u, registered_ranges.size());
 }
 
-TEST_F(StatisticsRecorderTest, RegisterHistogram) {
+TEST_P(StatisticsRecorderTest, RegisterHistogram) {
   // Create a Histogram that was not registered.
   Histogram* histogram = CreateHistogram("TestHistogram", 1, 1000, 10);
 
@@ -152,7 +176,7 @@ TEST_F(StatisticsRecorderTest, RegisterHistogram) {
   EXPECT_EQ(1u, registered_histograms.size());
 }
 
-TEST_F(StatisticsRecorderTest, FindHistogram) {
+TEST_P(StatisticsRecorderTest, FindHistogram) {
   HistogramBase* histogram1 = Histogram::FactoryGet(
       "TestHistogram1", 1, 1000, 10, HistogramBase::kNoFlags);
   HistogramBase* histogram2 = Histogram::FactoryGet(
@@ -160,10 +184,35 @@ TEST_F(StatisticsRecorderTest, FindHistogram) {
 
   EXPECT_EQ(histogram1, StatisticsRecorder::FindHistogram("TestHistogram1"));
   EXPECT_EQ(histogram2, StatisticsRecorder::FindHistogram("TestHistogram2"));
-  EXPECT_TRUE(StatisticsRecorder::FindHistogram("TestHistogram") == NULL);
+  EXPECT_FALSE(StatisticsRecorder::FindHistogram("TestHistogram"));
+
+  // Create a new global allocator using the same memory as the old one. Any
+  // old one is kept around so the memory doesn't get released. The two
+  // objects will get released in a backwards order but this is okay because
+  // the destructors aren't allowed to access the underlying memory segment.
+  std::unique_ptr<GlobalHistogramAllocator> old_global =
+      GlobalHistogramAllocator::ReleaseForTesting();
+  if (use_persistent_histogram_allocator_) {
+    GlobalHistogramAllocator::CreateWithPersistentMemory(
+        const_cast<void*>(old_global->data()), old_global->length(), 0,
+        old_global->Id(), old_global->Name());
+  }
+
+  // Reset statistics-recorder to validate operation from a clean start.
+  UninitializeStatisticsRecorder();
+  InitializeStatisticsRecorder();
+
+  if (use_persistent_histogram_allocator_) {
+    EXPECT_TRUE(StatisticsRecorder::FindHistogram("TestHistogram1"));
+    EXPECT_TRUE(StatisticsRecorder::FindHistogram("TestHistogram2"));
+  } else {
+    EXPECT_FALSE(StatisticsRecorder::FindHistogram("TestHistogram1"));
+    EXPECT_FALSE(StatisticsRecorder::FindHistogram("TestHistogram2"));
+  }
+  EXPECT_FALSE(StatisticsRecorder::FindHistogram("TestHistogram"));
 }
 
-TEST_F(StatisticsRecorderTest, GetSnapshot) {
+TEST_P(StatisticsRecorderTest, GetSnapshot) {
   Histogram::FactoryGet("TestHistogram1", 1, 1000, 10, Histogram::kNoFlags);
   Histogram::FactoryGet("TestHistogram2", 1, 1000, 10, Histogram::kNoFlags);
   Histogram::FactoryGet("TestHistogram3", 1, 1000, 10, Histogram::kNoFlags);
@@ -181,7 +230,7 @@ TEST_F(StatisticsRecorderTest, GetSnapshot) {
   EXPECT_EQ(0u, snapshot.size());
 }
 
-TEST_F(StatisticsRecorderTest, RegisterHistogramWithFactoryGet) {
+TEST_P(StatisticsRecorderTest, RegisterHistogramWithFactoryGet) {
   StatisticsRecorder::Histograms registered_histograms;
 
   StatisticsRecorder::GetHistograms(&registered_histograms);
@@ -227,7 +276,14 @@ TEST_F(StatisticsRecorderTest, RegisterHistogramWithFactoryGet) {
   EXPECT_EQ(4u, registered_histograms.size());
 }
 
-TEST_F(StatisticsRecorderTest, RegisterHistogramWithMacros) {
+TEST_P(StatisticsRecorderTest, RegisterHistogramWithMacros) {
+  // Macros cache pointers and so tests that use them can only be run once.
+  // Stop immediately if this test has run previously.
+  static bool already_run = false;
+  if (already_run)
+    return;
+  already_run = true;
+
   StatisticsRecorder::Histograms registered_histograms;
 
   HistogramBase* histogram = Histogram::FactoryGet(
@@ -248,7 +304,7 @@ TEST_F(StatisticsRecorderTest, RegisterHistogramWithMacros) {
   EXPECT_EQ(3u, registered_histograms.size());
 }
 
-TEST_F(StatisticsRecorderTest, BucketRangesSharing) {
+TEST_P(StatisticsRecorderTest, BucketRangesSharing) {
   std::vector<const BucketRanges*> ranges;
   StatisticsRecorder::GetBucketRanges(&ranges);
   EXPECT_EQ(0u, ranges.size());
@@ -266,11 +322,15 @@ TEST_F(StatisticsRecorderTest, BucketRangesSharing) {
   EXPECT_EQ(2u, ranges.size());
 }
 
-TEST_F(StatisticsRecorderTest, ToJSON) {
-  LOCAL_HISTOGRAM_COUNTS("TestHistogram1", 30);
-  LOCAL_HISTOGRAM_COUNTS("TestHistogram1", 40);
-  LOCAL_HISTOGRAM_COUNTS("TestHistogram2", 30);
-  LOCAL_HISTOGRAM_COUNTS("TestHistogram2", 40);
+TEST_P(StatisticsRecorderTest, ToJSON) {
+  Histogram::FactoryGet("TestHistogram1", 1, 1000, 50, HistogramBase::kNoFlags)
+      ->Add(30);
+  Histogram::FactoryGet("TestHistogram1", 1, 1000, 50, HistogramBase::kNoFlags)
+      ->Add(40);
+  Histogram::FactoryGet("TestHistogram2", 1, 1000, 50, HistogramBase::kNoFlags)
+      ->Add(30);
+  Histogram::FactoryGet("TestHistogram2", 1, 1000, 50, HistogramBase::kNoFlags)
+      ->Add(40);
 
   std::string json(StatisticsRecorder::ToJSON(std::string()));
 
@@ -325,20 +385,39 @@ TEST_F(StatisticsRecorderTest, ToJSON) {
   EXPECT_TRUE(json.empty());
 }
 
-TEST_F(StatisticsRecorderTest, IterationTest) {
-  StatisticsRecorder::Histograms registered_histograms;
-  LOCAL_HISTOGRAM_COUNTS("TestHistogram.IterationTest1", 30);
-  GlobalHistogramAllocator::CreateWithLocalMemory(64 << 10 /* 64 KiB */, 0, "");
-  LOCAL_HISTOGRAM_COUNTS("TestHistogram.IterationTest2", 30);
+TEST_P(StatisticsRecorderTest, IterationTest) {
+  Histogram::FactoryGet("IterationTest1", 1, 64, 16, HistogramBase::kNoFlags);
+  Histogram::FactoryGet("IterationTest2", 1, 64, 16, HistogramBase::kNoFlags);
 
   StatisticsRecorder::HistogramIterator i1 = StatisticsRecorder::begin(true);
-  EXPECT_NE(StatisticsRecorder::end(), i1);
-  EXPECT_NE(StatisticsRecorder::end(), ++i1);
-  EXPECT_EQ(StatisticsRecorder::end(), ++i1);
+  EXPECT_EQ(2, CountIterableHistograms(i1));
 
   StatisticsRecorder::HistogramIterator i2 = StatisticsRecorder::begin(false);
-  EXPECT_NE(StatisticsRecorder::end(), i2);
-  EXPECT_EQ(StatisticsRecorder::end(), ++i2);
+  EXPECT_EQ(use_persistent_histogram_allocator_ ? 0 : 2,
+            CountIterableHistograms(i2));
+
+  // Create a new global allocator using the same memory as the old one. Any
+  // old one is kept around so the memory doesn't get released. The two
+  // objects will get released in a backwards order but this is okay because
+  // the destructors aren't allowed to access the underlying memory segment.
+  std::unique_ptr<GlobalHistogramAllocator> old_global =
+      GlobalHistogramAllocator::ReleaseForTesting();
+  if (use_persistent_histogram_allocator_) {
+    GlobalHistogramAllocator::CreateWithPersistentMemory(
+        const_cast<void*>(old_global->data()), old_global->length(), 0,
+        old_global->Id(), old_global->Name());
+  }
+
+  // Reset statistics-recorder to validate operation from a clean start.
+  UninitializeStatisticsRecorder();
+  InitializeStatisticsRecorder();
+
+  StatisticsRecorder::HistogramIterator i3 = StatisticsRecorder::begin(true);
+  EXPECT_EQ(use_persistent_histogram_allocator_ ? 2 : 0,
+            CountIterableHistograms(i3));
+
+  StatisticsRecorder::HistogramIterator i4 = StatisticsRecorder::begin(false);
+  EXPECT_EQ(0, CountIterableHistograms(i4));
 }
 
 namespace {
@@ -360,7 +439,7 @@ struct CallbackCheckWrapper {
 }  // namespace
 
 // Check that you can't overwrite the callback with another.
-TEST_F(StatisticsRecorderTest, SetCallbackFailsWithoutHistogramTest) {
+TEST_P(StatisticsRecorderTest, SetCallbackFailsWithoutHistogramTest) {
   CallbackCheckWrapper callback_wrapper;
 
   bool result = base::StatisticsRecorder::SetCallback(
@@ -375,7 +454,7 @@ TEST_F(StatisticsRecorderTest, SetCallbackFailsWithoutHistogramTest) {
 }
 
 // Check that you can't overwrite the callback with another.
-TEST_F(StatisticsRecorderTest, SetCallbackFailsWithHistogramTest) {
+TEST_P(StatisticsRecorderTest, SetCallbackFailsWithHistogramTest) {
   HistogramBase* histogram = Histogram::FactoryGet("TestHistogram", 1, 1000, 10,
                                                    HistogramBase::kNoFlags);
   EXPECT_TRUE(histogram);
@@ -402,7 +481,7 @@ TEST_F(StatisticsRecorderTest, SetCallbackFailsWithHistogramTest) {
 }
 
 // Check that you can't overwrite the callback with another.
-TEST_F(StatisticsRecorderTest, ClearCallbackSuceedsWithHistogramTest) {
+TEST_P(StatisticsRecorderTest, ClearCallbackSuceedsWithHistogramTest) {
   HistogramBase* histogram = Histogram::FactoryGet("TestHistogram", 1, 1000, 10,
                                                    HistogramBase::kNoFlags);
   EXPECT_TRUE(histogram);
@@ -425,7 +504,7 @@ TEST_F(StatisticsRecorderTest, ClearCallbackSuceedsWithHistogramTest) {
 }
 
 // Check that callback is used.
-TEST_F(StatisticsRecorderTest, CallbackUsedTest) {
+TEST_P(StatisticsRecorderTest, CallbackUsedTest) {
   {
     HistogramBase* histogram = Histogram::FactoryGet(
         "TestHistogram", 1, 1000, 10, HistogramBase::kNoFlags);
@@ -499,7 +578,7 @@ TEST_F(StatisticsRecorderTest, CallbackUsedTest) {
 }
 
 // Check that setting a callback before the histogram exists works.
-TEST_F(StatisticsRecorderTest, CallbackUsedBeforeHistogramCreatedTest) {
+TEST_P(StatisticsRecorderTest, CallbackUsedBeforeHistogramCreatedTest) {
   CallbackCheckWrapper callback_wrapper;
 
   base::StatisticsRecorder::SetCallback(