From 8c68845090b8ff6987866638c3891e4aacabc501 Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Tue, 11 Sep 2018 13:46:22 +0200 Subject: [PATCH] Move variance calculation in SampleCounter to a new extension class Variance calculation isn't currently used but overflow checks there may cause unnecessary crash. Instead of completely deleting useful feature it's now easy to disable it by choosing an appropriate Counter class. Bug: None Change-Id: Ifa8bbf2d023553504caa768e08e59ebccfb2fbb4 Reviewed-on: https://webrtc-review.googlesource.com/99561 Reviewed-by: Karl Wiberg Commit-Queue: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#24699} --- rtc_base/numerics/sample_counter.cc | 39 ++++++++++++++------ rtc_base/numerics/sample_counter.h | 19 ++++++++-- rtc_base/numerics/sample_counter_unittest.cc | 32 ++++++++++++++-- 3 files changed, 72 insertions(+), 18 deletions(-) diff --git a/rtc_base/numerics/sample_counter.cc b/rtc_base/numerics/sample_counter.cc index d9244c385a..47d9eafa9e 100644 --- a/rtc_base/numerics/sample_counter.cc +++ b/rtc_base/numerics/sample_counter.cc @@ -26,11 +26,6 @@ void SampleCounter::Add(int sample) { RTC_DCHECK_GE(sample, std::numeric_limits::min() - sum_); } sum_ += sample; - // Prevent overflow in squaring. - RTC_DCHECK_GT(sample, std::numeric_limits::min()); - RTC_DCHECK_LE(int64_t{sample} * sample, - std::numeric_limits::max() - sum_squared_); - sum_squared_ += int64_t{sample} * sample; ++num_samples_; if (!max_ || sample > *max_) { max_ = sample; @@ -44,9 +39,6 @@ void SampleCounter::Add(const SampleCounter& other) { RTC_DCHECK_GE(other.sum_, std::numeric_limits::min() - sum_); } sum_ += other.sum_; - RTC_DCHECK_LE(other.sum_squared_, - std::numeric_limits::max() - sum_squared_); - sum_squared_ += other.sum_squared_; RTC_DCHECK_LE(other.num_samples_, std::numeric_limits::max() - num_samples_); num_samples_ += other.num_samples_; @@ -61,7 +53,18 @@ absl::optional SampleCounter::Avg(int64_t min_required_samples) const { return rtc::dchecked_cast(sum_ / num_samples_); } -absl::optional SampleCounter::Variance( +absl::optional SampleCounter::Max() const { + return max_; +} + +void SampleCounter::Reset() { + *this = {}; +} + +SampleCounterWithVariance::SampleCounterWithVariance() = default; +SampleCounterWithVariance::~SampleCounterWithVariance() = default; + +absl::optional SampleCounterWithVariance::Variance( int64_t min_required_samples) const { RTC_DCHECK_GT(min_required_samples, 0); if (num_samples_ < min_required_samples) @@ -71,11 +74,23 @@ absl::optional SampleCounter::Variance( return sum_squared_ / num_samples_ - mean * mean; } -absl::optional SampleCounter::Max() const { - return max_; +void SampleCounterWithVariance::Add(int sample) { + SampleCounter::Add(sample); + // Prevent overflow in squaring. + RTC_DCHECK_GT(sample, std::numeric_limits::min()); + RTC_DCHECK_LE(int64_t{sample} * sample, + std::numeric_limits::max() - sum_squared_); + sum_squared_ += int64_t{sample} * sample; } -void SampleCounter::Reset() { +void SampleCounterWithVariance::Add(const SampleCounterWithVariance& other) { + SampleCounter::Add(other); + RTC_DCHECK_LE(other.sum_squared_, + std::numeric_limits::max() - sum_squared_); + sum_squared_ += other.sum_squared_; +} + +void SampleCounterWithVariance::Reset() { *this = {}; } diff --git a/rtc_base/numerics/sample_counter.h b/rtc_base/numerics/sample_counter.h index 643754e8f9..0a212bc5e2 100644 --- a/rtc_base/numerics/sample_counter.h +++ b/rtc_base/numerics/sample_counter.h @@ -23,19 +23,32 @@ class SampleCounter { ~SampleCounter(); void Add(int sample); absl::optional Avg(int64_t min_required_samples) const; - absl::optional Variance(int64_t min_required_samples) const; absl::optional Max() const; void Reset(); // Adds all the samples from the |other| SampleCounter as if they were all // individually added using |Add(int)| method. void Add(const SampleCounter& other); - private: + protected: int64_t sum_ = 0; - int64_t sum_squared_ = 0; int64_t num_samples_ = 0; absl::optional max_; }; +class SampleCounterWithVariance : public SampleCounter { + public: + SampleCounterWithVariance(); + ~SampleCounterWithVariance(); + void Add(int sample); + absl::optional Variance(int64_t min_required_samples) const; + void Reset(); + // Adds all the samples from the |other| SampleCounter as if they were all + // individually added using |Add(int)| method. + void Add(const SampleCounterWithVariance& other); + + private: + int64_t sum_squared_ = 0; +}; + } // namespace rtc #endif // RTC_BASE_NUMERICS_SAMPLE_COUNTER_H_ diff --git a/rtc_base/numerics/sample_counter_unittest.cc b/rtc_base/numerics/sample_counter_unittest.cc index 898f00cc62..4085370037 100644 --- a/rtc_base/numerics/sample_counter_unittest.cc +++ b/rtc_base/numerics/sample_counter_unittest.cc @@ -24,7 +24,6 @@ TEST(SampleCounterTest, ProcessesNoSamples) { constexpr int kMinSamples = 1; SampleCounter counter; EXPECT_THAT(counter.Avg(kMinSamples), Eq(absl::nullopt)); - EXPECT_THAT(counter.Avg(kMinSamples), Eq(absl::nullopt)); EXPECT_THAT(counter.Max(), Eq(absl::nullopt)); } @@ -35,7 +34,6 @@ TEST(SampleCounterTest, NotEnoughSamples) { counter.Add(value); } EXPECT_THAT(counter.Avg(kMinSamples), Eq(absl::nullopt)); - EXPECT_THAT(counter.Avg(kMinSamples), Eq(absl::nullopt)); EXPECT_THAT(counter.Max(), Eq(5)); } @@ -46,8 +44,36 @@ TEST(SampleCounterTest, EnoughSamples) { counter.Add(value); } EXPECT_THAT(counter.Avg(kMinSamples), Eq(3)); - EXPECT_THAT(counter.Variance(kMinSamples), Eq(2)); EXPECT_THAT(counter.Max(), Eq(5)); } +TEST(SampleCounterTest, ComputesVariance) { + constexpr int kMinSamples = 5; + SampleCounterWithVariance counter; + for (int value : {1, 2, 3, 4, 5}) { + counter.Add(value); + } + EXPECT_THAT(counter.Variance(kMinSamples), Eq(2)); +} + +TEST(SampleCounterTest, AggregatesTwoCounters) { + constexpr int kMinSamples = 5; + SampleCounterWithVariance counter1; + for (int value : {1, 2, 3}) { + counter1.Add(value); + } + SampleCounterWithVariance counter2; + for (int value : {4, 5}) { + counter2.Add(value); + } + // Before aggregation there is not enough samples. + EXPECT_THAT(counter1.Avg(kMinSamples), Eq(absl::nullopt)); + EXPECT_THAT(counter1.Variance(kMinSamples), Eq(absl::nullopt)); + // Aggregate counter2 in counter1. + counter1.Add(counter2); + EXPECT_THAT(counter1.Avg(kMinSamples), Eq(3)); + EXPECT_THAT(counter1.Max(), Eq(5)); + EXPECT_THAT(counter1.Variance(kMinSamples), Eq(2)); +} + } // namespace rtc