From a69fa24d7b70421870cafa86340ece900489e17e Mon Sep 17 00:00:00 2001 From: Joakim Hassila Date: Thu, 30 Mar 2023 12:30:04 +0200 Subject: [PATCH] fix(patch): Clamp percentiles to actually recorded min/max instead of rounding to equivalent levels (#14) Clamp percentile result to min/max actual recorded range as we have perfect fidelity there, unexpected p0/p100 otherwise. Co-authored-by: dimlio <122263440+dimlio@users.noreply.github.com> --- Sources/Histogram/Histogram.swift | 33 ++++++++++++++++--- .../HistogramExample/HistogramExample.swift | 2 ++ .../HistogramAutosizingTests.swift | 4 ++- .../HistogramDataAccessTests.swift | 2 ++ Tests/HistogramTests/HistogramTests.swift | 17 ++++++++++ 5 files changed, 52 insertions(+), 6 deletions(-) diff --git a/Sources/Histogram/Histogram.swift b/Sources/Histogram/Histogram.swift index e1f05c2..d6112e8 100644 --- a/Sources/Histogram/Histogram.swift +++ b/Sources/Histogram/Histogram.swift @@ -326,6 +326,14 @@ public struct Histogram: Codable { * in the histogram are either larger than or equivalent to. Returns 0 if no recorded values exist. */ public func valueAtPercentile(_ percentile: Double) -> UInt64 { + guard percentile != 0.0 else { + return self.min + } + + guard percentile < 100.0 else { + return maxRecorded + } + // Truncate to 0..100%, and remove 1 ulp to avoid roundoff overruns into next bucket when we // subsequently round up to the nearest integer: let requestedPercentile = Swift.min(Swift.max(percentile.nextDown, 0.0), 100.0) @@ -342,9 +350,8 @@ public struct Histogram: Codable { totalToCurrentIndex += UInt64(counts[i]) if totalToCurrentIndex >= countAtPercentile { let valueAtIndex = valueFromIndex(i) - return (percentile == 0.0) ? - lowestEquivalentForValue(valueAtIndex) : - highestEquivalentForValue(valueAtIndex) + let returnValue = highestEquivalentForValue(valueAtIndex) + return Swift.max(Swift.min(returnValue, maxRecorded), self.min) } } return 0 @@ -435,7 +442,7 @@ public struct Histogram: Codable { } /** - * Get the lowest recorded value level in the histogram. + * Get the lowest recorded value in the histogram. * * If the histogram has no recorded values, the value returned is undefined. * @@ -446,12 +453,25 @@ public struct Histogram: Codable { } /** - * Get the highest recorded value level in the histogram. + * Get the highest recorded value in the histogram. * * If the histogram has no recorded values, the value returned is undefined. * * - Returns: The maximum value recorded in the histogram. */ + public var maxRecorded: UInt64 { + maxValue + } + + /** + * Get the highest recorded value level in the histogram. + * + * If the histogram has no recorded values, the value returned is undefined. + * + * NB this can be larger than the maximum recorded value due to precision. + * + * - Returns: The maximum value level recorded in the histogram. + */ public var max: UInt64 { maxValue == 0 ? 0 : highestEquivalentForValue(maxValue) } @@ -460,6 +480,7 @@ public struct Histogram: Codable { * Get the lowest recorded non-zero value level in the histogram. * * If the histogram has no recorded values, the value returned is undefined. + * NB this can be smaller than the minimum recorded value due to precision. * * - Returns: The lowest recorded non-zero value level in the histogram. */ @@ -1347,3 +1368,5 @@ extension Histogram: TextOutputStreamable { outputPercentileDistribution(to: &to, outputValueUnitScalingRatio: 1.0) } } + +// swiftlint:enable file_length type_body_length line_length identifier_name diff --git a/Sources/HistogramExample/HistogramExample.swift b/Sources/HistogramExample/HistogramExample.swift index 2d85d13..5890352 100644 --- a/Sources/HistogramExample/HistogramExample.swift +++ b/Sources/HistogramExample/HistogramExample.swift @@ -53,3 +53,5 @@ enum HistogramExample { print("\n", histogram) } } + +// swiftlint:enable identifier_name diff --git a/Tests/HistogramTests/HistogramAutosizingTests.swift b/Tests/HistogramTests/HistogramAutosizingTests.swift index 77621c6..1b39c73 100644 --- a/Tests/HistogramTests/HistogramAutosizingTests.swift +++ b/Tests/HistogramTests/HistogramAutosizingTests.swift @@ -8,7 +8,7 @@ // http://www.apache.org/licenses/LICENSE-2.0 // -// swiftlint:disable identifier_name todo +// swiftlint:disable todo @testable import Histogram import XCTest @@ -86,3 +86,5 @@ final class HistogramAutosizingTests: XCTestCase { } } } + +// swiftlint:enable todo diff --git a/Tests/HistogramTests/HistogramDataAccessTests.swift b/Tests/HistogramTests/HistogramDataAccessTests.swift index f98258b..baeee9d 100644 --- a/Tests/HistogramTests/HistogramDataAccessTests.swift +++ b/Tests/HistogramTests/HistogramDataAccessTests.swift @@ -491,3 +491,5 @@ final class HistogramDataAccessTests: XCTestCase { XCTAssertEqual(4_104, snapshots.count) } } + +// swiftlint:enable file_length identifier_name line_length trailing_comma diff --git a/Tests/HistogramTests/HistogramTests.swift b/Tests/HistogramTests/HistogramTests.swift index cd32462..d33b1a5 100644 --- a/Tests/HistogramTests/HistogramTests.swift +++ b/Tests/HistogramTests/HistogramTests.swift @@ -20,6 +20,21 @@ final class HistogramTests: XCTestCase { private static let numberOfSignificantValueDigits = SignificantDigits.three private static let value: UInt64 = 4 + // This is a check for valueAtPercentiles that would return 999936 + // instead of 1000_001 for p0 (which would return lowestEquivalentForValue instead of min) + // and it would return 1000447 for p100, instead of max + func testPercentilesMinMax() throws { + var histogram = Histogram(lowestDiscernibleValue: 1, highestTrackableValue: 3_600_000_000, numberOfSignificantValueDigits: .three) + for _ in 0 ..< 90 { + histogram.record(1_000_001) + } + XCTAssertEqual(histogram.minNonZeroValue, 1_000_001) + XCTAssertEqual(histogram.min, 1_000_001) + XCTAssertEqual(histogram.min, histogram.valueAtPercentile(0.0)) + XCTAssertEqual(histogram.maxRecorded, 1_000_001) + XCTAssertEqual(histogram.maxRecorded, histogram.valueAtPercentile(100.0)) + } + func testCreate() throws { let h = Histogram(lowestDiscernibleValue: 1, highestTrackableValue: 3_600_000_000, numberOfSignificantValueDigits: .three) XCTAssertEqual(h.counts.count, 23_552) @@ -504,3 +519,5 @@ final class HistogramTests: XCTestCase { XCTAssertEqual(computedMaxValue, h.maxValue) } } + +// swiftlint:enable file_length identifier_name line_length