Commit 3364055a authored by Nick Sukhanov's avatar Nick Sukhanov Committed by Facebook Github Bot

Other approach to fix issues with double comparisons

Summary:
So it seems that my previous approach doesn't solve the issue completely, we are still seeing these errors, because std::numeric_limits<T>::epsilon() precision is not enough. We can do a different epsilon i.e 1e-6 but it will probably create other problems when values are very big or very small.

I've read about how to do a proper comparison here: https://www.boost.org/doc/libs/1_66_0/libs/test/doc/html/boost_test/testing_tools/extended_comparison/floating_point/floating_points_comparison_theory.html
but this seems to bee too complicated and slow to introduce relative error comparisons here.

So I decided that we shouldn't do this comparison for floating types at all, because it doesn't make much sense, because we are trying to capture integer overflow with these guards.

Example:
"E0116 06:30:24.439344 3368082 Histogram.h:233] Called: lhs: 3.999999999999005240 rhs:4.000000000000000000 eps: 0.000000000000000222 diff: 0.000000000000994760"

Reviewed By: yfeldblum

Differential Revision: D13693535

fbshipit-source-id: e3e7dafa4517612c63bc8e22b62eeeb053677cb8
parent 49ec4f6d
......@@ -171,7 +171,7 @@ T HistogramBuckets<T, BucketType>::getPercentileEstimate(
ValueType low;
ValueType high;
if (bucketIdx == 0) {
if (less(min_, avg)) {
if (kIsExact && min_ < avg) {
// This normally shouldn't happen. This bucket is only supposed to track
// values less than min_. Most likely this means that integer overflow
// occurred, and the code in avgFromBucket() returned a huge value
......@@ -194,7 +194,7 @@ T HistogramBuckets<T, BucketType>::getPercentileEstimate(
low = std::numeric_limits<ValueType>::min();
}
} else if (bucketIdx == buckets_.size() - 1) {
if (less(avg, max_)) {
if (kIsExact && avg < max_) {
// Most likely this means integer overflow occurred. See the comments
// above in the minimum case.
LOG(ERROR) << "invalid average value in histogram maximum bucket: " << avg
......@@ -212,7 +212,7 @@ T HistogramBuckets<T, BucketType>::getPercentileEstimate(
} else {
low = getBucketMin(bucketIdx);
high = getBucketMax(bucketIdx);
if (less(avg, low) || less(high, avg)) {
if (kIsExact && (avg < low || avg > high)) {
// Most likely this means an integer overflow occurred.
// See the comments above. Return the midpoint between low and high
// as a best guess, since avg is meaningless.
......@@ -230,7 +230,7 @@ T HistogramBuckets<T, BucketType>::getPercentileEstimate(
// Assume that the median value in this bucket is the same as the average
// value.
double medianPct = (lowPct + highPct) / 2.0;
if (less(pct, medianPct)) {
if (pct < medianPct) {
// Assume that the data points lower than the median of this bucket
// are uniformly distributed between low and avg
double pctThroughSection = (pct - lowPct) / (medianPct - lowPct);
......
......@@ -223,12 +223,7 @@ class HistogramBuckets {
}
private:
template <typename V>
bool less(const V& lhs, const V& rhs) const {
using nl = std::numeric_limits<V>;
return lhs < rhs && (nl::is_integer || rhs - lhs > nl::epsilon());
}
static constexpr bool kIsExact = std::numeric_limits<ValueType>::is_exact;
ValueType bucketSize_;
ValueType min_;
ValueType max_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment