From: Adam Simpkins Date: Mon, 20 Aug 2012 21:58:25 +0000 (-0700) Subject: BucketedTimeSeries: fix type converison issues computing avg() X-Git-Tag: v0.22.0~1199 X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=14223eae3f3cc0059f2d129ceec84b48506a6434;p=folly.git BucketedTimeSeries: fix type converison issues computing avg() Summary: D527040 had a bug in the code to compute the average: it incorrectly performed unsigned division when ValueType was a signed integer type. As a result, the average was reported incorrectly for stats with negative values. This makes the average code more intelligent when handling type conversions: if the caller wants a floating point value, or if the input type is floating point, floating point division is always returned. Otherwise, if the input is a signed type, signed integer division is performed. Otherwise, unsigned integer division is performed. Test Plan: beholdunittests Reviewed By: lars@fb.com FB internal diff: D553583 --- diff --git a/folly/detail/Stats.h b/folly/detail/Stats.h index 1b74aa4d..b3fa7e68 100644 --- a/folly/detail/Stats.h +++ b/folly/detail/Stats.h @@ -21,6 +21,53 @@ namespace folly { namespace detail { +/* + * Helper functions for how to perform division based on the desired + * 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 (count == 0) { return ReturnType(0); } + return sum / static_cast(count); +} + +// For unsigned integer input types, do unsigned division +template +typename std::enable_if::value && + !std::is_floating_point::value && + std::is_unsigned::value, + ReturnType>::type +avgHelper(ValueType sum, uint64_t count) { + if (count == 0) { return ReturnType(0); } + return sum / count; +} + + template struct Bucket { public: @@ -55,9 +102,7 @@ struct Bucket { template ReturnType avg() const { - return (count ? - static_cast(sum) / count : - ReturnType(0)); + return avgHelper(sum, count); } ValueType sum; diff --git a/folly/stats/BucketedTimeSeries-defs.h b/folly/stats/BucketedTimeSeries-defs.h index e65b197d..61bf7f61 100644 --- a/folly/stats/BucketedTimeSeries-defs.h +++ b/folly/stats/BucketedTimeSeries-defs.h @@ -233,7 +233,7 @@ ReturnType BucketedTimeSeries::avg(TimeType start, TimeType end) const { return ReturnType(0); } - return static_cast(sum) / count; + return detail::avgHelper(sum, count); } /* diff --git a/folly/test/TimeseriesTest.cpp b/folly/test/TimeseriesTest.cpp index 84b64b14..c052b706 100644 --- a/folly/test/TimeseriesTest.cpp +++ b/folly/test/TimeseriesTest.cpp @@ -19,6 +19,8 @@ #include #include +#include "folly/Foreach.h" + using std::chrono::seconds; using std::string; using std::vector; @@ -319,6 +321,116 @@ TEST(BucketedTimeSeries, rate) { EXPECT_NEAR(1.5, ts.countRate(), 0.005); } +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. + + { + // Simple sanity tests for small positive integer values + BucketedTimeSeries ts(60, seconds(600)); + ts.addValue(seconds(0), 4, 100); + 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); + } + + { + // Test signed integer types with negative values + BucketedTimeSeries ts(60, seconds(600)); + ts.addValue(seconds(0), -100); + ts.addValue(seconds(0), -200); + 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); + } + + { + // Test uint64_t values that would overflow int64_t + BucketedTimeSeries ts(60, seconds(600)); + ts.addValueAggregated(seconds(0), + 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); + } + + { + // Test doubles with small-ish values that will fit in integer types + BucketedTimeSeries ts(60, seconds(600)); + ts.addValue(seconds(0), 4.0, 100); + 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); + } + + { + // Test doubles with huge values + BucketedTimeSeries ts(60, seconds(600)); + ts.addValue(seconds(0), 1e19, 100); + ts.addValue(seconds(0), 2e19, 200); + ts.addValue(seconds(0), 3e19, 100); + + EXPECT_DOUBLE_EQ(ts.avg(), 2e19); + EXPECT_NEAR(ts.avg(), 2e19, 1e11); + } + + { + // Test doubles where the sum adds up larger than a uint64_t, + // but the average fits in an int64_t + BucketedTimeSeries ts(60, seconds(600)); + uint64_t value = 0x3fffffffffffffff; + FOR_EACH_RANGE(i, 0, 16) { + 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); + } + + { + // Test BucketedTimeSeries with a smaller integer type + BucketedTimeSeries ts(60, seconds(600)); + FOR_EACH_RANGE(i, 0, 101) { + 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); + } +} + TEST(BucketedTimeSeries, forEachBucket) { typedef BucketedTimeSeries::Bucket Bucket; struct BucketInfo {