From ab69bf3250cf83f322cd95e768b6391d3966f590 Mon Sep 17 00:00:00 2001
From: Ben Sinclair <ben@typius.com>
Date: Tue, 12 Mar 2024 16:34:40 +1100
Subject: [PATCH] Fix percentile calculation, there was an issue with sorting

The last item of the durations array was being left out of the sort, and
sometimes I was seeing larger p75 values than p995, which is nonsensical. I've
fixed the issue and added some tests and asserts that check for sanity.

The average and standard deviation functions looked like they could use some
tweaks too.
---
 build.zig  |  1 +
 tests.zig  | 19 +++++++++++++++++++
 zbench.zig | 50 ++++++++++++++++----------------------------------
 3 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/build.zig b/build.zig
index a1b078f..79123ba 100644
--- a/build.zig
+++ b/build.zig
@@ -38,6 +38,7 @@ fn setupTesting(b: *std.Build, target: std.zig.CrossTarget, optimize: std.builti
     const test_files = [_]struct { name: []const u8, path: []const u8 }{
         .{ .name = "tests", .path = "tests.zig" },
         .{ .name = "format_test", .path = "util/format_test.zig" },
+        .{ .name = "quicksort", .path = "util/quicksort.zig" },
     };
 
     const test_step = b.step("test", "Run library tests");
diff --git a/tests.zig b/tests.zig
index 36bd877..fb945a1 100644
--- a/tests.zig
+++ b/tests.zig
@@ -2,6 +2,7 @@ const std = @import("std");
 const test_alloc = std.testing.allocator;
 const print = std.debug.print;
 const expectEq = std.testing.expectEqual;
+const expectEqDeep = std.testing.expectEqualDeep;
 
 const Benchmark = @import("./zbench.zig").Benchmark;
 
@@ -30,3 +31,21 @@ test "Benchmark.calculateStd and Benchmark.calculateAverage" {
     try expectEq(@as(u64, 1), bench.calculateAverage());
     try expectEq(@as(u64, 0), bench.calculateStd());
 }
+
+test "Benchmark.calculatePercentiles" {
+    const P = Benchmark.Percentiles;
+    var bench = try Benchmark.init("test_bench", std.testing.allocator, .{});
+    defer bench.durations.deinit();
+
+    for (0..100) |i| try bench.durations.append(i);
+    try expectEq(
+        P{ .p75 = 75, .p99 = 99, .p995 = 99 },
+        bench.calculatePercentiles(),
+    );
+
+    std.mem.reverse(u64, bench.durations.items);
+    try expectEq(
+        P{ .p75 = 75, .p99 = 99, .p995 = 99 },
+        bench.calculatePercentiles(),
+    );
+}
diff --git a/zbench.zig b/zbench.zig
index f34085d..1078152 100644
--- a/zbench.zig
+++ b/zbench.zig
@@ -144,44 +144,31 @@ pub const Benchmark = struct {
     /// which 75%, 99% and 99.5% of the other measurments would lie (respectively) when timings are
     /// sorted in increasing order.
     pub fn calculatePercentiles(self: Benchmark) Percentiles {
-        if (self.durations.items.len < 2) {
+        // quickSort might fail with an empty input slice, so safety checks first
+        if (self.durations.items.len <= 1) {
             std.log.warn("Insufficient data for percentile calculation.", .{});
             return Percentiles{ .p75 = 0, .p99 = 0, .p995 = 0 };
         }
 
-        // quickSort might fail with an empty input slice, so safety checks first
         const len = self.durations.items.len;
-        var lastIndex: usize = 0;
-        if (len > 1) {
-            lastIndex = len - 1;
-        }
-        quicksort.sort(u64, self.durations.items, 0, lastIndex - 1);
+        quicksort.sort(u64, self.durations.items, 0, len - 1);
 
-        const p75Index: usize = len * 75 / 100;
-        const p99Index: usize = len * 99 / 100;
-        const p995Index: usize = len * 995 / 1000;
-
-        const p75 = self.durations.items[p75Index];
-        const p99 = self.durations.items[p99Index];
-        const p995 = self.durations.items[p995Index];
+        const p75 = self.durations.items[len * 75 / 100];
+        const p99 = self.durations.items[len * 99 / 100];
+        const p995 = self.durations.items[len * 995 / 1000];
 
+        std.debug.assert(p75 <= p99);
+        std.debug.assert(p99 <= p995);
         return Percentiles{ .p75 = p75, .p99 = p99, .p995 = p995 };
     }
 
     /// Calculate the average (more precisely arithmetic mean) of the durations
     pub fn calculateAverage(self: Benchmark) u64 {
-        // prevent division by zero
-        const len = self.durations.items.len;
-        if (len == 0) return 0;
-
-        var sum: u64 = 0;
-        for (self.durations.items) |duration| {
-            sum += duration;
-        }
-
-        const avg = sum / len;
-
-        return avg;
+        return if (self.durations.items.len == 0) 0 else blk: {
+            var sum: u64 = 0;
+            for (self.durations.items) |d| sum += d;
+            break :blk sum / self.durations.items.len;
+        };
     }
 
     /// Calculate the standard deviation of the durations. An estimate for the average *deviation*
@@ -189,18 +176,13 @@ pub const Benchmark = struct {
     pub fn calculateStd(self: Benchmark) u64 {
         if (self.durations.items.len <= 1) return 0;
 
+        // We are using the non-biased estimator for the variance; sum(Xi - μ)^2 / (n - 1)
         const avg = self.calculateAverage();
         var nvar: u64 = 0;
         for (self.durations.items) |dur| {
-            // NOTE: With realistic real-life samples this will never overflow,
-            // however a solution without bitcasts would still be cleaner
-            const d: i64 = @bitCast(dur);
-            const a: i64 = @bitCast(avg);
-
-            nvar += @bitCast((d - a) * (d - a));
+            const sd = @max(dur, avg) - @min(dur, avg);
+            nvar += sd * sd;
         }
-
-        // We are using the non-biased estimator for the variance; sum(Xi - μ)^2 / (n - 1)
         return std.math.sqrt(nvar / (self.durations.items.len - 1));
     }
 };