From 71b8393b6a8bfc0f778fb77ef166fb574e1fa25b Mon Sep 17 00:00:00 2001 From: sakal Date: Fri, 16 Sep 2016 06:56:15 -0700 Subject: [PATCH] Remove name parameter from HistogramAdd function. This name is only used for a DCHECK. Having it as a parameter leads to unnecessary string copying even on release builds. This CL instead adds a GetHistogramName function that is only called on debug builds. Checking if pointer is null is also moved outside HistogramAdd function. Having it there is confusing since Chromium implementation doesn't have it there. BUG=webrtc:6329 Review-Url: https://codereview.webrtc.org/2337883003 Cr-Commit-Position: refs/heads/master@{#14263} --- webrtc/api/android/jni/androidmetrics_jni.cc | 6 +- webrtc/system_wrappers/include/metrics.h | 55 +++++++++++-------- .../system_wrappers/include/metrics_default.h | 3 - .../system_wrappers/source/metrics_default.cc | 14 +---- 4 files changed, 37 insertions(+), 41 deletions(-) diff --git a/webrtc/api/android/jni/androidmetrics_jni.cc b/webrtc/api/android/jni/androidmetrics_jni.cc index 833066a66f..68b0b73bbe 100644 --- a/webrtc/api/android/jni/androidmetrics_jni.cc +++ b/webrtc/api/android/jni/androidmetrics_jni.cc @@ -68,8 +68,10 @@ JOW(jlong, Metrics_00024Histogram_nativeCreateCounts) JOW(void, Metrics_00024Histogram_nativeAddSample) (JNIEnv* jni, jclass, jlong histogram, jint sample) { - HistogramAdd(reinterpret_cast(histogram), - sample); + if (histogram) { + HistogramAdd(reinterpret_cast(histogram), + sample); + } } } // namespace webrtc_jni diff --git a/webrtc/system_wrappers/include/metrics.h b/webrtc/system_wrappers/include/metrics.h index ceddc95eae..9b14736ec2 100644 --- a/webrtc/system_wrappers/include/metrics.h +++ b/webrtc/system_wrappers/include/metrics.h @@ -110,35 +110,41 @@ // 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 { \ +#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(nullptr), \ - histogram_pointer); \ - RTC_DCHECK(prev_pointer == nullptr || \ - prev_pointer == histogram_pointer); \ - } \ - webrtc::metrics::HistogramAdd(histogram_pointer, constant_name, sample); \ + 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(nullptr), \ + histogram_pointer); \ + RTC_DCHECK(prev_pointer == nullptr || \ + prev_pointer == histogram_pointer); \ + } \ + if (histogram_pointer) { \ + RTC_DCHECK_EQ(constant_name, \ + webrtc::metrics::GetHistogramName(histogram_pointer)) \ + << "The name should not vary."; \ + webrtc::metrics::HistogramAdd(histogram_pointer, sample); \ + } \ } while (0) // Deprecated. // The histogram is constructed/found for each call. -// May be used for histograms with infrequent updates. +// 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); \ + do { \ + webrtc::metrics::Histogram* histogram_pointer = factory_get_invocation; \ + if (histogram_pointer) { \ + webrtc::metrics::HistogramAdd(histogram_pointer, sample); \ + } \ } while (0) - // Helper macros. // Macros for calling a histogram with varying name (e.g. when using a metric // in different modes such as real-time vs screenshare). @@ -212,10 +218,11 @@ Histogram* HistogramFactoryGetCounts( Histogram* HistogramFactoryGetEnumeration( const std::string& name, int boundary); +// Returns name of the histogram. +const std::string& GetHistogramName(Histogram* histogram_pointer); + // Function for adding a |sample| to a histogram. -// |name| can be used to verify that it matches the histogram name. -void HistogramAdd( - Histogram* histogram_pointer, const std::string& name, int sample); +void HistogramAdd(Histogram* histogram_pointer, int sample); } // namespace metrics } // namespace webrtc diff --git a/webrtc/system_wrappers/include/metrics_default.h b/webrtc/system_wrappers/include/metrics_default.h index 94ba3cda32..e262198e57 100644 --- a/webrtc/system_wrappers/include/metrics_default.h +++ b/webrtc/system_wrappers/include/metrics_default.h @@ -55,9 +55,6 @@ int NumSamples(const std::string& name); // Returns the minimum sample value (or -1 if the histogram has no samples). int MinSample(const std::string& name); -// Function for adding a |sample| to a histogram without checkking the name. -void HistogramAdd(Histogram* histogram_pointer, int sample); - } // namespace metrics } // namespace webrtc diff --git a/webrtc/system_wrappers/source/metrics_default.cc b/webrtc/system_wrappers/source/metrics_default.cc index fd7610b113..8ee10bacbc 100644 --- a/webrtc/system_wrappers/source/metrics_default.cc +++ b/webrtc/system_wrappers/source/metrics_default.cc @@ -232,23 +232,13 @@ Histogram* HistogramFactoryGetEnumeration(const std::string& name, return map->GetEnumerationHistogram(name, boundary); } -// Fast path. Adds |sample| to cached |histogram_pointer|. -void HistogramAdd(Histogram* histogram_pointer, - const std::string& name, - int sample) { - if (!histogram_pointer) - return; - +const std::string& GetHistogramName(Histogram* histogram_pointer) { RtcHistogram* ptr = reinterpret_cast(histogram_pointer); - RTC_DCHECK_EQ(name, ptr->name()) << "The name should not vary."; - ptr->Add(sample); + return ptr->name(); } // Fast path. Adds |sample| to cached |histogram_pointer|. void HistogramAdd(Histogram* histogram_pointer, int sample) { - if (!histogram_pointer) - return; - RtcHistogram* ptr = reinterpret_cast(histogram_pointer); ptr->Add(sample); }