Refactor of Histogram related code

base/metrics/statistics_recorder.cc

Index: base/metrics/statistics_recorder.cc
===================================================================
--- base/metrics/statistics_recorder.cc	(revision 148055)
+++ base/metrics/statistics_recorder.cc	(working copy)
@@ -6,14 +6,18 @@
 
 #include "base/debug/leak_annotations.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/stringprintf.h"
 #include "base/synchronization/lock.h"
 
+using std::list;
+using std::string;
+
 namespace {
 // Initialize histogram statistics gathering system.
-base::LazyInstance<base::StatisticsRecorder>::Leaky
-    g_statistics_recorder_ = LAZY_INSTANCE_INITIALIZER;
+base::LazyInstance<base::StatisticsRecorder>::Leaky g_statistics_recorder_ =
+    LAZY_INSTANCE_INITIALIZER;
 }  // namespace
 
 namespace base {
@@ -25,53 +29,6 @@
 // Collect the number of ranges_ elements saved because of caching ranges.
 static size_t saved_ranges_size_ = 0;
 
-// 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() {
-  DCHECK(!histograms_);
-  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
-    // during static initialization and released only on  process termination.
-    lock_ = new base::Lock;
-  }
-  base::AutoLock auto_lock(*lock_);
-  histograms_ = new HistogramMap;
-  ranges_ = new RangesMap;
-}
-
-StatisticsRecorder::~StatisticsRecorder() {
-  DCHECK(histograms_ && lock_);
-
-  if (dump_on_exit_) {
-    std::string output;
-    WriteGraph("", &output);
-    DLOG(INFO) << output;
-  }
-  // Clean up.
-  HistogramMap* histograms = NULL;
-  {
-    base::AutoLock auto_lock(*lock_);
-    histograms = histograms_;
-    histograms_ = NULL;
-  }
-  RangesMap* ranges = NULL;
-  {
-    base::AutoLock auto_lock(*lock_);
-    ranges = ranges_;
-    ranges_ = NULL;
-  }
-  // We are going to leak the histograms and the ranges.
-  delete histograms;
-  delete ranges;
-  // We don't delete lock_ on purpose to avoid having to properly protect
-  // against it going away after we checked for NULL in the static methods.
-}
-
 // static
 void StatisticsRecorder::Initialize() {
   // Ensure that an instance of the StatisticsRecorder object is created.
@@ -87,77 +44,94 @@
   return NULL != histograms_;
 }
 
+// static
 Histogram* StatisticsRecorder::RegisterOrDeleteDuplicate(Histogram* histogram) {
   // As per crbug.com/79322 the histograms are intentionally leaked, so we need
   // to annotate them. Because ANNOTATE_LEAKING_OBJECT_PTR may be used only once
   // for an object, the duplicates should not be annotated.
   // Callers are responsible for not calling RegisterOrDeleteDuplicate(ptr)
   // twice if (lock_ == NULL) || (!histograms_).
-  DCHECK(histogram->HasValidRangeChecksum());
   if (lock_ == NULL) {
     ANNOTATE_LEAKING_OBJECT_PTR(histogram);  // see crbug.com/79322
     return histogram;
   }
-  base::AutoLock auto_lock(*lock_);
-  if (!histograms_) {
-    ANNOTATE_LEAKING_OBJECT_PTR(histogram);  // see crbug.com/79322
-    return histogram;
+
+  Histogram* histogram_to_delete = NULL;
+  Histogram* histogram_to_return = NULL;
+  {
+    base::AutoLock auto_lock(*lock_);
+    if (histograms_ == NULL) {
+      ANNOTATE_LEAKING_OBJECT_PTR(histogram);  // see crbug.com/79322
+      histogram_to_return = histogram;
+    } else {
+      const string& name = histogram->histogram_name();
+      HistogramMap::iterator it = histograms_->find(name);
+      if (histograms_->end() == it) {
+        (*histograms_)[name] = histogram;
+        ANNOTATE_LEAKING_OBJECT_PTR(histogram);  // see crbug.com/79322
+        ++number_of_histograms_;
+        histogram_to_return = histogram;
+      } else if (histogram == it->second) {
+        // The histogram was registered before.
+        histogram_to_return = histogram;
+      } else {
+        // We already have one histogram with this name.
+        histogram_to_return = it->second;
+        histogram_to_delete = histogram;
+      }
+    }
   }
-  const std::string name = histogram->histogram_name();
-  HistogramMap::iterator it = histograms_->find(name);
-  // Avoid overwriting a previous registration.
-  if (histograms_->end() == it) {
-    (*histograms_)[name] = histogram;
-    ANNOTATE_LEAKING_OBJECT_PTR(histogram);  // see crbug.com/79322
-    RegisterOrDeleteDuplicateRanges(histogram);
-    ++number_of_histograms_;
-  } else {
-    delete histogram;  // We already have one by this name.
-    histogram = it->second;
-  }
-  return histogram;
+  delete histogram_to_delete;
+  return histogram_to_return;
 }
 
 // static
-void StatisticsRecorder::RegisterOrDeleteDuplicateRanges(Histogram* histogram) {
-  DCHECK(histogram);
-  BucketRanges* histogram_ranges = histogram->bucket_ranges();
-  DCHECK(histogram_ranges);
-  uint32 checksum = histogram->range_checksum();
-  histogram_ranges->set_checksum(checksum);
+const BucketRanges* StatisticsRecorder::RegisterOrDeleteDuplicateRanges(
+    const BucketRanges* ranges) {
+  DCHECK(ranges->HasValidChecksum());
+  if (lock_ == NULL) {
+    ANNOTATE_LEAKING_OBJECT_PTR(ranges);
+    return ranges;
+  }
 
-  RangesMap::iterator ranges_it = ranges_->find(checksum);
+  scoped_ptr<const BucketRanges> ranges_deleter(ranges);

[jar (doing other things), 2012/08/01 16:41:25]

IMO, you should wait till the last moment to push the value |ranges| into this
ptr. You should do it at the point where you decide to do the delete, but want
it deferred until the lock is released.  Initializing it now complicates the
code in lines 101, and 122.

IMO, you should leave this scoped_ptr initialized to NULL, and then set its
value only after line 123.

[kaiwang, 2012/08/01 19:48:19]

Done.

+  base::AutoLock auto_lock(*lock_);
+  if (ranges_ == NULL) {
+    ANNOTATE_LEAKING_OBJECT_PTR(ranges);
+    return ranges_deleter.release();
+  }
+
+  list<const BucketRanges*>* checksum_matching_list;
+  RangesMap::iterator ranges_it = ranges_->find(ranges->checksum());
   if (ranges_->end() == ranges_it) {
-    // Register the new BucketRanges.
-    std::list<BucketRanges*>* checksum_matching_list(
-        new std::list<BucketRanges*>());
-    checksum_matching_list->push_front(histogram_ranges);
-    (*ranges_)[checksum] = checksum_matching_list;
-    return;
+    // Add a new matching list to map.
+    checksum_matching_list = new list<const BucketRanges*>();
+    ANNOTATE_LEAKING_OBJECT_PTR(checksum_matching_list);
+    (*ranges_)[ranges->checksum()] = checksum_matching_list;
+  } else {
+    checksum_matching_list = ranges_it->second;
   }
 
-  // Use the registered BucketRanges if the registered BucketRanges has same
-  // ranges_ as |histogram|'s BucketRanges.
-  std::list<BucketRanges*>* checksum_matching_list = ranges_it->second;
-  std::list<BucketRanges*>::iterator checksum_matching_list_it;
+  list<const BucketRanges*>::iterator checksum_matching_list_it;
   for (checksum_matching_list_it = checksum_matching_list->begin();
        checksum_matching_list_it != checksum_matching_list->end();
        ++checksum_matching_list_it) {
-    BucketRanges* existing_histogram_ranges = *checksum_matching_list_it;
-    DCHECK(existing_histogram_ranges);
-    if (existing_histogram_ranges->Equals(histogram_ranges)) {
-      histogram->set_bucket_ranges(existing_histogram_ranges);
-      ++number_of_vectors_saved_;
-      saved_ranges_size_ += histogram_ranges->size();
-      delete histogram_ranges;
-      return;
+    const BucketRanges* existing_ranges = *checksum_matching_list_it;
+    if (existing_ranges->Equals(ranges)) {
+      if (existing_ranges == ranges) {
+        return ranges_deleter.release();
+      } else {
+        ++number_of_vectors_saved_;
+        saved_ranges_size_ += ranges->size();
+        return existing_ranges;
+      }
     }
   }
-
   // We haven't found a BucketRanges which has the same ranges. Register the
   // new BucketRanges.
-  DCHECK(checksum_matching_list_it == checksum_matching_list->end());
-  checksum_matching_list->push_front(histogram_ranges);
+  checksum_matching_list->push_front(ranges);
+  ANNOTATE_LEAKING_OBJECT_PTR(ranges);
+  return ranges_deleter.release();
 }
 
 // static
@@ -246,8 +220,9 @@
   if (lock_ == NULL)
     return;
   base::AutoLock auto_lock(*lock_);
-  if (!histograms_)
+  if (histograms_ == NULL)
     return;
+
   for (HistogramMap::iterator it = histograms_->begin();
        histograms_->end() != it;
        ++it) {
@@ -257,12 +232,35 @@
 }
 
 // static
+void StatisticsRecorder::GetBucketRanges(
+    std::vector<const BucketRanges*>* output) {
+  if (lock_ == NULL)
+    return;
+  base::AutoLock auto_lock(*lock_);
+  if (ranges_ == NULL)
+    return;
+
+  for (RangesMap::iterator it = ranges_->begin();
+       ranges_->end() != it;
+       ++it) {
+    list<const BucketRanges*>* ranges_list = it->second;
+    list<const BucketRanges*>::iterator ranges_list_it;
+    for (ranges_list_it = ranges_list->begin();
+         ranges_list_it != ranges_list->end();
+         ++ranges_list_it) {
+      output->push_back(*ranges_list_it);
+    }
+  }
+}
+
+// static
 Histogram* StatisticsRecorder::FindHistogram(const std::string& name) {
   if (lock_ == NULL)
     return NULL;
   base::AutoLock auto_lock(*lock_);
-  if (!histograms_)
+  if (histograms_ == NULL)
     return NULL;
+
   HistogramMap::iterator it = histograms_->find(name);
   if (histograms_->end() == it)
     return NULL;
@@ -275,8 +273,9 @@
   if (lock_ == NULL)
     return;
   base::AutoLock auto_lock(*lock_);
-  if (!histograms_)
+  if (histograms_ == NULL)
     return;
+
   for (HistogramMap::iterator it = histograms_->begin();
        histograms_->end() != it;
        ++it) {
@@ -285,6 +284,49 @@
   }
 }
 
+// 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() {
+  DCHECK(!histograms_);
+  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
+    // during static initialization and released only on  process termination.
+    lock_ = new base::Lock;
+  }
+  base::AutoLock auto_lock(*lock_);
+  histograms_ = new HistogramMap;
+  ranges_ = new RangesMap;
+}
+
+StatisticsRecorder::~StatisticsRecorder() {
+  DCHECK(histograms_ && ranges_ && lock_);
+
+  scoped_ptr<HistogramMap> histograms_deleter(histograms_);
+  scoped_ptr<RangesMap> ranges_deleter(ranges_);

[jar (doing other things), 2012/08/01 16:41:25]

For both of these, you should wait until you are inside the lock (lines 323) to
snatch the values and put it into the scoped_ptr instances.

[kaiwang, 2012/08/01 19:48:19]

Done.

+  // We don't delete lock_ on purpose to avoid having to properly protect
+  // against it going away after we checked for NULL in the static methods.
+
+  if (dump_on_exit_) {
+    string output;
+    WriteGraph("", &output);
+    DLOG(INFO) << output;
+  }
+
+  // Clean up.
+  {
+    base::AutoLock auto_lock(*lock_);
+    histograms_ = NULL;
+    ranges_ = NULL;
+  }
+  // We are going to leak the histograms and the ranges.
+}
+
+
 // static
 StatisticsRecorder::HistogramMap* StatisticsRecorder::histograms_ = NULL;
 // static