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}
diff --git a/webrtc/system_wrappers/include/metrics.h b/webrtc/system_wrappers/include/metrics.h
index 5e8ca11..4cd74c5 100644
--- a/webrtc/system_wrappers/include/metrics.h
+++ b/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,24 +100,57 @@
#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, \
- factory_get_invocation) \
+// 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 { \
+ 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, constant_name, sample); \
+ webrtc::metrics::HistogramAdd(histogram_pointer, name, sample); \
} while (0)
namespace webrtc {
diff --git a/webrtc/system_wrappers/source/metrics_unittest.cc b/webrtc/system_wrappers/source/metrics_unittest.cc
new file mode 100644
index 0000000..8319b78
--- /dev/null
+++ b/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
diff --git a/webrtc/system_wrappers/system_wrappers_tests.gyp b/webrtc/system_wrappers/system_wrappers_tests.gyp
index f668dfa..a0ae14d 100644
--- a/webrtc/system_wrappers/system_wrappers_tests.gyp
+++ b/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',
diff --git a/webrtc/test/histogram.cc b/webrtc/test/histogram.cc
index 6fcdb68..2893e43 100644
--- a/webrtc/test/histogram.cc
+++ b/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 @@
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 @@
if (it == histograms_.end()) {
return -1;
}
- return it->second.last;
+ return it->second.last_;
}
int NumHistogramSamples(const std::string& name) {
@@ -70,13 +83,15 @@
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
-
diff --git a/webrtc/test/histogram.h b/webrtc/test/histogram.h
index 44ce32b..3c8e743 100644
--- a/webrtc/test/histogram.h
+++ b/webrtc/test/histogram.h
@@ -23,7 +23,7 @@
// 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