Add implementation in metrics.h that uses atomic pointer.

Update test implementation (test/histograms.h) to be more similar a real implementation (where histogram get functions return a Histogram pointer). Add check that the name of a histogram does not change.

BUG=webrtc:5283

Review URL: https://codereview.webrtc.org/1528403003

Cr-Commit-Position: refs/heads/master@{#11161}

webrtc/system_wrappers/include/metrics.h

@@ -13,6 +13,8 @@
 
 #include <string>
 
+#include "webrtc/base/atomicops.h"
+#include "webrtc/base/checks.h"
 #include "webrtc/common_types.h"
 
 // Macros for allowing WebRTC clients (e.g. Chrome) to gather and aggregate
@@ -58,17 +60,29 @@
 
 
 // Macros for adding samples to a named histogram.
-//
-// NOTE: this is a temporary solution.
-// The aim is to mimic the behaviour in Chromium's src/base/metrics/histograms.h
-// However as atomics are not supported in webrtc, this is for now a modified
-// and temporary solution. Note that the histogram is constructed/found for
-// each call. Therefore, for now only use this implementation for metrics
-// that do not need to be updated frequently.
-// TODO(asapersson): Change implementation when atomics are supported.
-// Also consider changing string to const char* when switching to atomics.
 
-// Histogram for counters.
+// Histogram for counters (exponentially spaced buckets).
+#define RTC_HISTOGRAM_COUNTS_100(name, sample) \
+  RTC_HISTOGRAM_COUNTS(name, sample, 1, 100, 50)
+
+#define RTC_HISTOGRAM_COUNTS_200(name, sample) \
+  RTC_HISTOGRAM_COUNTS(name, sample, 1, 200, 50)
+
+#define RTC_HISTOGRAM_COUNTS_1000(name, sample) \
+  RTC_HISTOGRAM_COUNTS(name, sample, 1, 1000, 50)
+
+#define RTC_HISTOGRAM_COUNTS_10000(name, sample) \
+  RTC_HISTOGRAM_COUNTS(name, sample, 1, 10000, 50)
+
+#define RTC_HISTOGRAM_COUNTS_100000(name, sample) \
+  RTC_HISTOGRAM_COUNTS(name, sample, 1, 100000, 50)
+
+#define RTC_HISTOGRAM_COUNTS(name, sample, min, max, bucket_count) \
+  RTC_HISTOGRAM_COMMON_BLOCK(name, sample, \
+      webrtc::metrics::HistogramFactoryGetCounts(name, min, max, bucket_count))
+
+// Deprecated.
+// TODO(asapersson): Remove.
 #define RTC_HISTOGRAM_COUNTS_SPARSE_100(name, sample) \
   RTC_HISTOGRAM_COUNTS_SPARSE(name, sample, 1, 100, 50)
 
@@ -86,26 +100,59 @@
 
 #define RTC_HISTOGRAM_COUNTS_SPARSE(name, sample, min, max, bucket_count) \
   RTC_HISTOGRAM_COMMON_BLOCK_SLOW(name, sample, \
-      webrtc::metrics::HistogramFactoryGetCounts( \
-          name, min, max, bucket_count))
+      webrtc::metrics::HistogramFactoryGetCounts(name, min, max, bucket_count))
 
-// Histogram for percentage.
+// Histogram for percentage (evenly spaced buckets).
+#define RTC_HISTOGRAM_PERCENTAGE(name, sample) \
+  RTC_HISTOGRAM_ENUMERATION(name, sample, 101)
+
+// Deprecated.
+// TODO(asapersson): Remove.
 #define RTC_HISTOGRAM_PERCENTAGE_SPARSE(name, sample) \
   RTC_HISTOGRAM_ENUMERATION_SPARSE(name, sample, 101)
 
-// Histogram for enumerators.
+// Histogram for enumerators (evenly spaced buckets).
 // |boundary| should be above the max enumerator sample.
+#define RTC_HISTOGRAM_ENUMERATION(name, sample, boundary) \
+  RTC_HISTOGRAM_COMMON_BLOCK(name, sample, \
+      webrtc::metrics::HistogramFactoryGetEnumeration(name, boundary))
+
+// Deprecated.
+// TODO(asapersson): Remove.
 #define RTC_HISTOGRAM_ENUMERATION_SPARSE(name, sample, boundary) \
   RTC_HISTOGRAM_COMMON_BLOCK_SLOW(name, sample, \
       webrtc::metrics::HistogramFactoryGetEnumeration(name, boundary))
 
-#define RTC_HISTOGRAM_COMMON_BLOCK_SLOW(constant_name, sample, \
+// The name of the histogram should not vary.
+// TODO(asapersson): Consider changing string to const char*.
+#define RTC_HISTOGRAM_COMMON_BLOCK(constant_name, sample, \
                                    factory_get_invocation) \
   do { \
-    webrtc::metrics::Histogram* histogram_pointer = factory_get_invocation; \
+    static webrtc::metrics::Histogram* atomic_histogram_pointer = nullptr; \
+    webrtc::metrics::Histogram* histogram_pointer = \
+        rtc::AtomicOps::AcquireLoadPtr(&atomic_histogram_pointer); \
+    if (!histogram_pointer) { \
+      histogram_pointer = factory_get_invocation; \
+      webrtc::metrics::Histogram* prev_pointer = \
+          rtc::AtomicOps::CompareAndSwapPtr( \
+              &atomic_histogram_pointer, \
+              static_cast<webrtc::metrics::Histogram*>(nullptr), \
+              histogram_pointer); \
+      RTC_DCHECK(prev_pointer == nullptr || \
+                 prev_pointer == histogram_pointer); \
+    } \
     webrtc::metrics::HistogramAdd(histogram_pointer, constant_name, sample); \
   } while (0)
 
+// Deprecated.
+// The histogram is constructed/found for each call.
+// May be used for histograms with infrequent updates.
+#define RTC_HISTOGRAM_COMMON_BLOCK_SLOW(name, sample, factory_get_invocation) \
+  do { \
+    webrtc::metrics::Histogram* histogram_pointer = factory_get_invocation; \
+    webrtc::metrics::HistogramAdd(histogram_pointer, name, sample); \
+  } while (0)
+
 namespace webrtc {
 namespace metrics {
 

webrtc/system_wrappers/source/metrics_unittest.cc

@@ -0,0 +1,91 @@
+/*
+ *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+#include "webrtc/system_wrappers/include/metrics.h"
+#include "webrtc/test/histogram.h"
+
+namespace webrtc {
+namespace {
+const int kSample = 22;
+const std::string kName = "Name";
+
+void AddSparseSample(const std::string& name, int sample) {
+  RTC_HISTOGRAM_COUNTS_SPARSE_100(name, sample);
+}
+#if GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+void AddSample(const std::string& name, int sample) {
+  RTC_HISTOGRAM_COUNTS_100(name, sample);
+}
+#endif  // GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+}  // namespace
+
+TEST(MetricsTest, InitiallyNoSamples) {
+  test::ClearHistograms();
+  EXPECT_EQ(0, test::NumHistogramSamples(kName));
+  EXPECT_EQ(-1, test::LastHistogramSample(kName));
+}
+
+TEST(MetricsTest, RtcHistogramPercent_AddSample) {
+  test::ClearHistograms();
+  RTC_HISTOGRAM_PERCENTAGE(kName, kSample);
+  EXPECT_EQ(1, test::NumHistogramSamples(kName));
+  EXPECT_EQ(kSample, test::LastHistogramSample(kName));
+}
+
+TEST(MetricsTest, RtcHistogramEnumeration_AddSample) {
+  test::ClearHistograms();
+  RTC_HISTOGRAM_ENUMERATION(kName, kSample, kSample + 1);
+  EXPECT_EQ(1, test::NumHistogramSamples(kName));
+  EXPECT_EQ(kSample, test::LastHistogramSample(kName));
+}
+
+TEST(MetricsTest, RtcHistogramCountsSparse_AddSample) {
+  test::ClearHistograms();
+  RTC_HISTOGRAM_COUNTS_SPARSE_100(kName, kSample);
+  EXPECT_EQ(1, test::NumHistogramSamples(kName));
+  EXPECT_EQ(kSample, test::LastHistogramSample(kName));
+}
+
+TEST(MetricsTest, RtcHistogramCounts_AddSample) {
+  test::ClearHistograms();
+  RTC_HISTOGRAM_COUNTS_100(kName, kSample);
+  EXPECT_EQ(1, test::NumHistogramSamples(kName));
+  EXPECT_EQ(kSample, test::LastHistogramSample(kName));
+}
+
+TEST(MetricsTest, RtcHistogramCounts_AddMultipleSamples) {
+  test::ClearHistograms();
+  const int kNumSamples = 10;
+  for (int i = 0; i < kNumSamples; ++i) {
+    RTC_HISTOGRAM_COUNTS_100(kName, i);
+  }
+  EXPECT_EQ(kNumSamples, test::NumHistogramSamples(kName));
+  EXPECT_EQ(kNumSamples - 1, test::LastHistogramSample(kName));
+}
+
+TEST(MetricsTest, RtcHistogramSparse_NonConstantNameWorks) {
+  test::ClearHistograms();
+  AddSparseSample("Name1", kSample);
+  AddSparseSample("Name2", kSample);
+  EXPECT_EQ(1, test::NumHistogramSamples("Name1"));
+  EXPECT_EQ(1, test::NumHistogramSamples("Name2"));
+}
+
+#if GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+TEST(MetricsTest, RtcHistogram_FailsForNonConstantName) {
+  test::ClearHistograms();
+  AddSample("Name1", kSample);
+  EXPECT_DEATH(AddSample("Name2", kSample), "");
+}
+#endif  // GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+
+}  // namespace webrtc

webrtc/system_wrappers/system_wrappers_tests.gyp

@@ -15,6 +15,7 @@
       'dependencies': [
         '<(DEPTH)/testing/gtest.gyp:gtest',
         '<(webrtc_root)/system_wrappers/system_wrappers.gyp:system_wrappers',
+        '<(webrtc_root)/test/test.gyp:histogram',
         '<(webrtc_root)/test/test.gyp:test_support_main',
       ],
       'sources': [
@@ -29,6 +30,7 @@
         'source/data_log_helpers_unittest.cc',
         'source/data_log_c_helpers_unittest.c',
         'source/data_log_c_helpers_unittest.h',
+        'source/metrics_unittest.cc',
         'source/ntp_time_unittest.cc',
         'source/rtp_to_ntp_unittest.cc',
         'source/scoped_vector_unittest.cc',

webrtc/test/histogram.cc

@@ -12,6 +12,7 @@
 
 #include <map>
 
+#include "webrtc/base/checks.h"
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/thread_annotations.h"
 #include "webrtc/system_wrappers/include/metrics.h"
@@ -22,10 +23,10 @@
 namespace webrtc {
 namespace {
 struct SampleInfo {
-  SampleInfo(int sample)
-      : last(sample), total(1) {}
-  int last;   // Last added sample.
-  int total;  // Total number of added samples.
+  SampleInfo(const std::string& name) : name_(name), last_(-1), total_(0) {}
+  const std::string name_;
+  int last_;   // Last added sample.
+  int total_;  // Total number of added samples.
 };
 
 rtc::CriticalSection histogram_crit_;
@@ -36,21 +37,33 @@ std::map<std::string, SampleInfo> histograms_ GUARDED_BY(histogram_crit_);
 
 namespace metrics {
 Histogram* HistogramFactoryGetCounts(const std::string& name, int min, int max,
-    int bucket_count) { return NULL; }
+                                     int bucket_count) {
+  rtc::CritScope cs(&histogram_crit_);
+  if (histograms_.find(name) == histograms_.end()) {
+    histograms_.insert(std::make_pair(name, SampleInfo(name)));
+  }
+  auto it = histograms_.find(name);
+  return reinterpret_cast<Histogram*>(&it->second);
+}
 
 Histogram* HistogramFactoryGetEnumeration(const std::string& name,
-    int boundary) { return NULL; }
+                                          int boundary) {
+  rtc::CritScope cs(&histogram_crit_);
+  if (histograms_.find(name) == histograms_.end()) {
+    histograms_.insert(std::make_pair(name, SampleInfo(name)));
+  }
+  auto it = histograms_.find(name);
+  return reinterpret_cast<Histogram*>(&it->second);
+}
 
 void HistogramAdd(
     Histogram* histogram_pointer, const std::string& name, int sample) {
   rtc::CritScope cs(&histogram_crit_);
-  auto it = histograms_.find(name);
-  if (it == histograms_.end()) {
-    histograms_.insert(std::make_pair(name, SampleInfo(sample)));
-    return;
-  }
-  it->second.last = sample;
-  ++it->second.total;
+  SampleInfo* ptr = reinterpret_cast<SampleInfo*>(histogram_pointer);
+  // The name should not vary.
+  RTC_CHECK(ptr->name_ == name);
+  ptr->last_ = sample;
+  ++ptr->total_;
 }
 }  // namespace metrics
 
@@ -61,7 +74,7 @@ int LastHistogramSample(const std::string& name) {
   if (it == histograms_.end()) {
     return -1;
   }
-  return it->second.last;
+  return it->second.last_;
 }
 
 int NumHistogramSamples(const std::string& name) {
@@ -70,13 +83,15 @@ int NumHistogramSamples(const std::string& name) {
   if (it == histograms_.end()) {
     return 0;
   }
-  return it->second.total;
+  return it->second.total_;
 }
 
 void ClearHistograms() {
   rtc::CritScope cs(&histogram_crit_);
-  histograms_.clear();
+  for (auto& it : histograms_) {
+    it.second.last_ = -1;
+    it.second.total_ = 0;
+  }
 }
 }  // namespace test
 }  // namespace webrtc
-

webrtc/test/histogram.h

@@ -23,7 +23,7 @@ int LastHistogramSample(const std::string& name);
 // Returns the number of added samples to a histogram.
 int NumHistogramSamples(const std::string& name);
 
-// Removes all histograms.
+// Removes all histogram samples.
 void ClearHistograms();
 
 }  // namespace test