Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

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
  • Loading branch information...
commit 14223eae3f3cc0059f2d129ceec84b48506a6434 1 parent 4f80222
@simpkins simpkins authored tudor committed
View
51 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 ReturnType, typename ValueType>
+typename std::enable_if<std::is_floating_point<ValueType>::value,
+ ReturnType>::type
+avgHelper(ValueType sum, uint64_t count) {
+ if (count == 0) { return ReturnType(0); }
+ return static_cast<ReturnType>(sum / count);
+}
+
+// For floating point return types, do floating point division
+template <typename ReturnType, typename ValueType>
+typename std::enable_if<std::is_floating_point<ReturnType>::value &&
+ !std::is_floating_point<ValueType>::value,
+ ReturnType>::type
+avgHelper(ValueType sum, uint64_t count) {
+ if (count == 0) { return ReturnType(0); }
+ return static_cast<ReturnType>(sum) / count;
+}
+
+// For signed integer input types, do signed division
+template <typename ReturnType, typename ValueType>
+typename std::enable_if<!std::is_floating_point<ReturnType>::value &&
+ !std::is_floating_point<ValueType>::value &&
+ std::is_signed<ValueType>::value,
+ ReturnType>::type
+avgHelper(ValueType sum, uint64_t count) {
+ if (count == 0) { return ReturnType(0); }
+ return sum / static_cast<int64_t>(count);
+}
+
+// For unsigned integer input types, do unsigned division
+template <typename ReturnType, typename ValueType>
+typename std::enable_if<!std::is_floating_point<ReturnType>::value &&
+ !std::is_floating_point<ValueType>::value &&
+ std::is_unsigned<ValueType>::value,
+ ReturnType>::type
+avgHelper(ValueType sum, uint64_t count) {
+ if (count == 0) { return ReturnType(0); }
+ return sum / count;
+}
+
+
template<typename T>
struct Bucket {
public:
@@ -55,9 +102,7 @@ struct Bucket {
template <typename ReturnType>
ReturnType avg() const {
- return (count ?
- static_cast<ReturnType>(sum) / count :
- ReturnType(0));
+ return avgHelper<ReturnType>(sum, count);
}
ValueType sum;
View
2  folly/stats/BucketedTimeSeries-defs.h
@@ -233,7 +233,7 @@ ReturnType BucketedTimeSeries<VT, TT>::avg(TimeType start, TimeType end) const {
return ReturnType(0);
}
- return static_cast<ReturnType>(sum) / count;
+ return detail::avgHelper<ReturnType>(sum, count);
}
/*
View
112 folly/test/TimeseriesTest.cpp
@@ -19,6 +19,8 @@
#include <glog/logging.h>
#include <gtest/gtest.h>
+#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<int64_t> 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<float>(), 10.0);
+ EXPECT_EQ(ts.avg<uint64_t>(), 10);
+ EXPECT_EQ(ts.avg<int64_t>(), 10);
+ EXPECT_EQ(ts.avg<int32_t>(), 10);
+ EXPECT_EQ(ts.avg<int16_t>(), 10);
+ EXPECT_EQ(ts.avg<int8_t>(), 10);
+ EXPECT_EQ(ts.avg<uint8_t>(), 10);
+ }
+
+ {
+ // Test signed integer types with negative values
+ BucketedTimeSeries<int64_t> 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<float>(), -200.0);
+ EXPECT_EQ(ts.avg<int64_t>(), -200);
+ EXPECT_EQ(ts.avg<int32_t>(), -200);
+ EXPECT_EQ(ts.avg<int16_t>(), -200);
+ }
+
+ {
+ // Test uint64_t values that would overflow int64_t
+ BucketedTimeSeries<uint64_t> ts(60, seconds(600));
+ ts.addValueAggregated(seconds(0),
+ std::numeric_limits<uint64_t>::max(),
+ std::numeric_limits<uint64_t>::max());
+
+ EXPECT_DOUBLE_EQ(ts.avg(), 1.0);
+ EXPECT_DOUBLE_EQ(ts.avg<float>(), 1.0);
+ EXPECT_EQ(ts.avg<uint64_t>(), 1);
+ EXPECT_EQ(ts.avg<int64_t>(), 1);
+ EXPECT_EQ(ts.avg<int8_t>(), 1);
+ }
+
+ {
+ // Test doubles with small-ish values that will fit in integer types
+ BucketedTimeSeries<double> 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<float>(), 10.0);
+ EXPECT_EQ(ts.avg<uint64_t>(), 10);
+ EXPECT_EQ(ts.avg<int64_t>(), 10);
+ EXPECT_EQ(ts.avg<int32_t>(), 10);
+ EXPECT_EQ(ts.avg<int16_t>(), 10);
+ EXPECT_EQ(ts.avg<int8_t>(), 10);
+ EXPECT_EQ(ts.avg<uint8_t>(), 10);
+ }
+
+ {
+ // Test doubles with huge values
+ BucketedTimeSeries<double> 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<float>(), 2e19, 1e11);
+ }
+
+ {
+ // Test doubles where the sum adds up larger than a uint64_t,
+ // but the average fits in an int64_t
+ BucketedTimeSeries<double> 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<float>(), value);
+ EXPECT_DOUBLE_EQ(ts.avg<uint64_t>(), value);
+ EXPECT_DOUBLE_EQ(ts.avg<int64_t>(), value);
+ }
+
+ {
+ // Test BucketedTimeSeries with a smaller integer type
+ BucketedTimeSeries<int16_t> 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<float>(), 50.0);
+ EXPECT_DOUBLE_EQ(ts.avg<uint64_t>(), 50);
+ EXPECT_DOUBLE_EQ(ts.avg<int64_t>(), 50);
+ EXPECT_DOUBLE_EQ(ts.avg<int16_t>(), 50);
+ EXPECT_DOUBLE_EQ(ts.avg<int8_t>(), 50);
+ }
+}
+
TEST(BucketedTimeSeries, forEachBucket) {
typedef BucketedTimeSeries<int64_t>::Bucket Bucket;
struct BucketInfo {
Please sign in to comment.
Something went wrong with that request. Please try again.