From: Adam Simpkins Date: Wed, 22 Aug 2012 03:56:09 +0000 (-0700) Subject: folly: simplify the stats avgHelper() function X-Git-Tag: v0.22.0~1198 X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=6bdcff461de752870654a59baeafe1141104d760;p=folly.git folly: simplify the stats avgHelper() function Summary: When the input type is a long double, perform division using long double. In all other cases, divide using double precision. Also fix the EXPECT_EQ() usage in the test case. Test Plan: fbconfig -r common/stats folly && fbmake runtests Reviewed By: andrei.alexandrescu@fb.com FB internal diff: D555433 --- diff --git a/folly/detail/Stats.h b/folly/detail/Stats.h index b3fa7e68..08218256 100644 --- a/folly/detail/Stats.h +++ b/folly/detail/Stats.h @@ -18,53 +18,35 @@ #define FOLLY_DETAIL_STATS_H_ #include +#include namespace folly { namespace detail { /* - * Helper functions for how to perform division based on the desired + * Helper function to compute the average, given a specified input type and * return type. */ -// For floating point input types, do floating point division -template -typename std::enable_if::value, - ReturnType>::type -avgHelper(ValueType sum, uint64_t count) { - if (count == 0) { return ReturnType(0); } - return static_cast(sum / count); -} - -// For floating point return types, do floating point division -template -typename std::enable_if::value && - !std::is_floating_point::value, - ReturnType>::type -avgHelper(ValueType sum, uint64_t count) { - if (count == 0) { return ReturnType(0); } - return static_cast(sum) / count; -} - -// For signed integer input types, do signed division -template -typename std::enable_if::value && - !std::is_floating_point::value && - std::is_signed::value, - ReturnType>::type -avgHelper(ValueType sum, uint64_t count) { +// If the input is long double, divide using long double to avoid losing +// precision. +template +ReturnType avgHelper(long double sum, uint64_t count) { if (count == 0) { return ReturnType(0); } - return sum / static_cast(count); + const long double countf = count; + return static_cast(sum / countf); } -// For unsigned integer input types, do unsigned division +// In all other cases divide using double precision. +// This should be relatively fast, and accurate enough for most use cases. template -typename std::enable_if::value && - !std::is_floating_point::value && - std::is_unsigned::value, +typename std::enable_if::type, + long double>::value, ReturnType>::type avgHelper(ValueType sum, uint64_t count) { if (count == 0) { return ReturnType(0); } - return sum / count; + const double sumf = sum; + const double countf = count; + return static_cast(sumf / countf); } diff --git a/folly/test/TimeseriesTest.cpp b/folly/test/TimeseriesTest.cpp index c052b706..69dbe9ad 100644 --- a/folly/test/TimeseriesTest.cpp +++ b/folly/test/TimeseriesTest.cpp @@ -322,9 +322,8 @@ TEST(BucketedTimeSeries, rate) { } TEST(BucketedTimeSeries, avgTypeConversion) { - // The average code has many different code paths to decide what type of - // division to perform (floating point, signed integer, unsigned integer). - // Test the various code paths. + // Make sure the computed average values are accurate regardless + // of the input type and return type. { // Simple sanity tests for small positive integer values @@ -333,14 +332,14 @@ TEST(BucketedTimeSeries, avgTypeConversion) { ts.addValue(seconds(0), 10, 200); ts.addValue(seconds(0), 16, 100); - EXPECT_DOUBLE_EQ(ts.avg(), 10.0); - EXPECT_DOUBLE_EQ(ts.avg(), 10.0); - EXPECT_EQ(ts.avg(), 10); - EXPECT_EQ(ts.avg(), 10); - EXPECT_EQ(ts.avg(), 10); - EXPECT_EQ(ts.avg(), 10); - EXPECT_EQ(ts.avg(), 10); - EXPECT_EQ(ts.avg(), 10); + EXPECT_DOUBLE_EQ(10.0, ts.avg()); + EXPECT_DOUBLE_EQ(10.0, ts.avg()); + EXPECT_EQ(10, ts.avg()); + EXPECT_EQ(10, ts.avg()); + EXPECT_EQ(10, ts.avg()); + EXPECT_EQ(10, ts.avg()); + EXPECT_EQ(10, ts.avg()); + EXPECT_EQ(10, ts.avg()); } { @@ -351,11 +350,11 @@ TEST(BucketedTimeSeries, avgTypeConversion) { ts.addValue(seconds(0), -300); ts.addValue(seconds(0), -200, 65535); - EXPECT_DOUBLE_EQ(ts.avg(), -200.0); - EXPECT_DOUBLE_EQ(ts.avg(), -200.0); - EXPECT_EQ(ts.avg(), -200); - EXPECT_EQ(ts.avg(), -200); - EXPECT_EQ(ts.avg(), -200); + EXPECT_DOUBLE_EQ(-200.0, ts.avg()); + EXPECT_DOUBLE_EQ(-200.0, ts.avg()); + EXPECT_EQ(-200, ts.avg()); + EXPECT_EQ(-200, ts.avg()); + EXPECT_EQ(-200, ts.avg()); } { @@ -365,11 +364,11 @@ TEST(BucketedTimeSeries, avgTypeConversion) { std::numeric_limits::max(), std::numeric_limits::max()); - EXPECT_DOUBLE_EQ(ts.avg(), 1.0); - EXPECT_DOUBLE_EQ(ts.avg(), 1.0); - EXPECT_EQ(ts.avg(), 1); - EXPECT_EQ(ts.avg(), 1); - EXPECT_EQ(ts.avg(), 1); + EXPECT_DOUBLE_EQ(1.0, ts.avg()); + EXPECT_DOUBLE_EQ(1.0, ts.avg()); + EXPECT_EQ(1, ts.avg()); + EXPECT_EQ(1, ts.avg()); + EXPECT_EQ(1, ts.avg()); } { @@ -379,14 +378,14 @@ TEST(BucketedTimeSeries, avgTypeConversion) { ts.addValue(seconds(0), 10.0, 200); ts.addValue(seconds(0), 16.0, 100); - EXPECT_DOUBLE_EQ(ts.avg(), 10.0); - EXPECT_DOUBLE_EQ(ts.avg(), 10.0); - EXPECT_EQ(ts.avg(), 10); - EXPECT_EQ(ts.avg(), 10); - EXPECT_EQ(ts.avg(), 10); - EXPECT_EQ(ts.avg(), 10); - EXPECT_EQ(ts.avg(), 10); - EXPECT_EQ(ts.avg(), 10); + EXPECT_DOUBLE_EQ(10.0, ts.avg()); + EXPECT_DOUBLE_EQ(10.0, ts.avg()); + EXPECT_EQ(10, ts.avg()); + EXPECT_EQ(10, ts.avg()); + EXPECT_EQ(10, ts.avg()); + EXPECT_EQ(10, ts.avg()); + EXPECT_EQ(10, ts.avg()); + EXPECT_EQ(10, ts.avg()); } { @@ -409,10 +408,12 @@ TEST(BucketedTimeSeries, avgTypeConversion) { ts.addValue(seconds(0), value); } - EXPECT_DOUBLE_EQ(ts.avg(), value); - EXPECT_DOUBLE_EQ(ts.avg(), value); - EXPECT_DOUBLE_EQ(ts.avg(), value); - EXPECT_DOUBLE_EQ(ts.avg(), value); + EXPECT_DOUBLE_EQ(value, ts.avg()); + EXPECT_DOUBLE_EQ(value, ts.avg()); + // Some precision is lost here due to the huge sum, so the + // integer average returned is off by one. + EXPECT_NEAR(value, ts.avg(), 1); + EXPECT_NEAR(value, ts.avg(), 1); } { @@ -422,12 +423,39 @@ TEST(BucketedTimeSeries, avgTypeConversion) { ts.addValue(seconds(0), i); } - EXPECT_DOUBLE_EQ(ts.avg(), 50.0); - EXPECT_DOUBLE_EQ(ts.avg(), 50.0); - EXPECT_DOUBLE_EQ(ts.avg(), 50); - EXPECT_DOUBLE_EQ(ts.avg(), 50); - EXPECT_DOUBLE_EQ(ts.avg(), 50); - EXPECT_DOUBLE_EQ(ts.avg(), 50); + EXPECT_DOUBLE_EQ(50.0, ts.avg()); + EXPECT_DOUBLE_EQ(50.0, ts.avg()); + EXPECT_EQ(50, ts.avg()); + EXPECT_EQ(50, ts.avg()); + EXPECT_EQ(50, ts.avg()); + EXPECT_EQ(50, ts.avg()); + } + + { + // Test BucketedTimeSeries with long double input + BucketedTimeSeries ts(60, seconds(600)); + ts.addValueAggregated(seconds(0), 1000.0L, 7); + + long double expected = 1000.0L / 7.0L; + EXPECT_DOUBLE_EQ(static_cast(expected), ts.avg()); + EXPECT_DOUBLE_EQ(static_cast(expected), ts.avg()); + EXPECT_DOUBLE_EQ(expected, ts.avg()); + EXPECT_EQ(static_cast(expected), ts.avg()); + EXPECT_EQ(static_cast(expected), ts.avg()); + } + + { + // Test BucketedTimeSeries with int64_t values, + // but using an average that requires a fair amount of precision. + BucketedTimeSeries ts(60, seconds(600)); + ts.addValueAggregated(seconds(0), 1000, 7); + + long double expected = 1000.0L / 7.0L; + EXPECT_DOUBLE_EQ(static_cast(expected), ts.avg()); + EXPECT_DOUBLE_EQ(static_cast(expected), ts.avg()); + EXPECT_DOUBLE_EQ(expected, ts.avg()); + EXPECT_EQ(static_cast(expected), ts.avg()); + EXPECT_EQ(static_cast(expected), ts.avg()); } }