From 33072f9de781c50491c080fe635f3d777aeed4a2 Mon Sep 17 00:00:00 2001 From: Gregor Zeitlinger Date: Wed, 19 Aug 2026 09:06:33 +0000 Subject: [PATCH 1/3] fix: clarify benchmark regression verdicts Signed-off-by: Gregor Zeitlinger --- .mise/tasks/generate_benchmark_summary.py | 142 +++++++++++------- .../tasks/test_generate-benchmark-summary.py | 99 ++++++++++++ 2 files changed, 190 insertions(+), 51 deletions(-) create mode 100644 .mise/tasks/test_generate-benchmark-summary.py diff --git a/.mise/tasks/generate_benchmark_summary.py b/.mise/tasks/generate_benchmark_summary.py index 3a1929b49..c38e7d7fa 100755 --- a/.mise/tasks/generate_benchmark_summary.py +++ b/.mise/tasks/generate_benchmark_summary.py @@ -26,6 +26,8 @@ from datetime import datetime, timezone from pathlib import Path +PRACTICAL_CHANGE_THRESHOLD = 5.0 + def parse_args(): parser = argparse.ArgumentParser( @@ -285,34 +287,53 @@ def lower_is_better(result: dict) -> bool: return mode in {"avgt", "sample", "ss"} or unit.endswith("/op") +def benchmark_metadata(result: dict) -> dict: + """Return metadata that must match for a base/head comparison.""" + primary_metric = result.get("primaryMetric", {}) + return { + "jmhVersion": result.get("jmhVersion"), + "mode": result.get("mode"), + "threads": result.get("threads"), + "forks": result.get("forks"), + "warmupIterations": result.get("warmupIterations"), + "warmupTime": result.get("warmupTime"), + "measurementIterations": result.get("measurementIterations"), + "measurementTime": result.get("measurementTime"), + "jdkVersion": result.get("jdkVersion"), + "scoreUnit": primary_metric.get("scoreUnit"), + "params": result.get("params", {}), + } + + +def comparable_metadata(head: dict, baseline: dict) -> bool: + """Return whether two results describe the same benchmark configuration.""" + return benchmark_metadata(head) == benchmark_metadata(baseline) + + def comparison_status(head: dict, baseline: dict) -> str: - """Classify a benchmark comparison using confidence intervals.""" + """Classify a benchmark comparison using confidence intervals and a threshold.""" head_interval = score_interval(head) baseline_interval = score_interval(baseline) head_score = metric_score(head) baseline_score = metric_score(baseline) - if head_score is None or baseline_score is None: - return "" - - is_lower_better = lower_is_better(head) - if head_interval and baseline_interval: - head_low, head_high = head_interval - baseline_low, baseline_high = baseline_interval - if is_lower_better: - if head_high < baseline_low: - return "faster" - if head_low > baseline_high: - return "slower" - else: - if head_low > baseline_high: - return "faster" - if head_high < baseline_low: - return "slower" + if ( + not comparable_metadata(head, baseline) + or head_score is None + or baseline_score is None + ): + return "inconclusive" + + change = performance_change(head, baseline) + if change is None or head_interval is None or baseline_interval is None: + return "inconclusive" + + head_low, head_high = head_interval + baseline_low, baseline_high = baseline_interval + intervals_overlap = head_low <= baseline_high and baseline_low <= head_high + if intervals_overlap or abs(change) < PRACTICAL_CHANGE_THRESHOLD: return "within noise" - if is_lower_better: - return "faster" if head_score < baseline_score else "slower" - return "faster" if head_score > baseline_score else "slower" + return "meaningful improvement" if change > 0 else "meaningful regression" def performance_change(head: dict, baseline: dict) -> float | None: @@ -373,7 +394,7 @@ def generate_comparison_section( md.append("") return md - md.append("| Benchmark | PR | Base | Change | Result |") + md.append("| Benchmark | PR | Base | Head vs base | Regression verdict |") md.append("|:----------|---:|-----:|-------:|:-------|") for name in common_names: @@ -396,7 +417,9 @@ def generate_comparison_section( md.append("") if missing_in_base: missing = ", ".join(short_benchmark_name(name) for name in missing_in_base) - md.append(f"- Benchmarks only in PR results: {missing}") + md.append( + f"- Benchmarks only in PR results (listed separately below): {missing}" + ) if missing_in_head: missing = ", ".join(short_benchmark_name(name) for name in missing_in_head) md.append(f"- Benchmarks only in base results: {missing}") @@ -474,9 +497,25 @@ def generate_markdown( ) ) + # A benchmark without a base counterpart cannot receive a regression verdict. + # Keep it out of the comparison-oriented head table and list it separately. + baseline_names = { + b.get("benchmark", "") for b in (baseline_results or []) if b.get("benchmark") + } + if baseline_results: + comparable_results = [ + b for b in results if b.get("benchmark", "") in baseline_names + ] + head_only_results = [ + b for b in results if b.get("benchmark", "") not in baseline_names + ] + else: + comparable_results = results + head_only_results = [] + # Group by benchmark class benchmarks_by_class: dict[str, list] = {} - for b in results: + for b in comparable_results: name = b.get("benchmark", "") parts = name.rsplit(".", 1) if len(parts) == 2: @@ -502,16 +541,10 @@ def generate_markdown( reverse=True, ) - md.append("| Benchmark | Score | Error | Units | Within run |") - md.append("|:----------|------:|------:|:------|:-----------|") - - best_score = ( - sorted_benchmarks[0].get("primaryMetric", {}).get("score", 1) - if sorted_benchmarks - else 1 - ) + md.append("| Benchmark | Score | Error | Units |") + md.append("|:----------|------:|------:|:------|") - for i, b in enumerate(sorted_benchmarks): + for b in sorted_benchmarks: name = b.get("benchmark", "").split(".")[-1] score = b.get("primaryMetric", {}).get("score", 0) error = b.get("primaryMetric", {}).get("scoreError", 0) @@ -520,23 +553,28 @@ def generate_markdown( score_fmt = format_score(score) error_fmt = format_error(error) - # Calculate relative performance as multiplier - try: - if i == 0: - relative_fmt = "**fastest**" - else: - multiplier = float(best_score) / float(score) - if multiplier >= 10: - relative_fmt = f"{multiplier:.0f}x slower" - else: - relative_fmt = f"{multiplier:.1f}x slower" - except (ValueError, TypeError, ZeroDivisionError): - relative_fmt = "" + md.append(f"| {name} | {score_fmt} | {error_fmt} | {unit} |") + md.append("") + + if head_only_results: + md.append("## New benchmarks in PR head") + md.append("") + md.append( + "These benchmarks have no base counterpart; scores are descriptive only " + "and have no regression verdict." + ) + md.append("") + md.append("| Benchmark | Score | Error | Units |") + md.append("|:----------|------:|------:|:------|") + for b in sorted(head_only_results, key=lambda x: x.get("benchmark", "")): + name = short_benchmark_name(b.get("benchmark", "")) + score = b.get("primaryMetric", {}).get("score", 0) + error = b.get("primaryMetric", {}).get("scoreError", 0) + unit = b.get("primaryMetric", {}).get("scoreUnit", "ops/s") md.append( - f"| {name} | {score_fmt} | {error_fmt} | {unit} | {relative_fmt} |" + f"| {name} | {format_score(score)} | {format_error(error)} | {unit} |" ) - md.append("") md.append("### Raw Results") @@ -581,12 +619,14 @@ def generate_markdown( md.append("- **Error** = 99.9% confidence interval") if baseline_results: md.append( - "- **Comparison with base** uses JMH confidence intervals when " - 'available; overlapping intervals are marked "within noise".' + "- **Regression verdict** requires comparable benchmark metadata, " + "non-overlapping JMH confidence intervals, and a change of at least " + f"{PRACTICAL_CHANGE_THRESHOLD:.0f}%; otherwise it is marked " + '"within noise" or "inconclusive".' ) md.append( - "- **Within run** compares benchmarks in the same result set, not against " - "the base commit." + "- Scores for different benchmark methods are not ranked against one another; " + "they may measure different workloads." ) md.append("") diff --git a/.mise/tasks/test_generate-benchmark-summary.py b/.mise/tasks/test_generate-benchmark-summary.py new file mode 100644 index 000000000..5ceb7eacc --- /dev/null +++ b/.mise/tasks/test_generate-benchmark-summary.py @@ -0,0 +1,99 @@ +import os +import sys +import unittest + +here = os.path.dirname(__file__) +if here not in sys.path: + sys.path.insert(0, here) + +from generate_benchmark_summary import ( + comparison_status, + generate_markdown, +) + + +def result( + name="io.prometheus.metrics.benchmarks.CounterBenchmark.prometheusInc", + score=100.0, + error=1.0, + threads=4, +): + primary_metric = { + "score": score, + "scoreUnit": "ops/s", + } + if error is not None: + primary_metric.update( + { + "scoreError": error, + "scoreConfidence": [score - error, score + error], + } + ) + + return { + "benchmark": name, + "jmhVersion": "1.37", + "mode": "thrpt", + "threads": threads, + "forks": 3, + "warmupIterations": 3, + "warmupTime": "10 s", + "measurementIterations": 5, + "measurementTime": "10 s", + "jdkVersion": "25.0.3", + "primaryMetric": primary_metric, + } + + +class TestBenchmarkComparison(unittest.TestCase): + def test_meaningful_improvement_requires_threshold_and_separation(self): + self.assertEqual( + comparison_status(result(score=106), result()), "meaningful improvement" + ) + + def test_meaningful_regression_requires_threshold_and_separation(self): + self.assertEqual( + comparison_status(result(score=94), result()), "meaningful regression" + ) + + def test_small_or_uncertain_change_is_within_noise(self): + self.assertEqual( + comparison_status(result(score=102, error=5), result(error=5)), + "within noise", + ) + + def test_mismatched_metadata_is_inconclusive(self): + self.assertEqual(comparison_status(result(threads=1), result()), "inconclusive") + + def test_missing_confidence_interval_is_inconclusive(self): + head = result(score=106, error=None) + self.assertEqual(comparison_status(head, result()), "inconclusive") + + +class TestBenchmarkMarkdown(unittest.TestCase): + def test_head_only_benchmarks_are_separate_and_not_ranked(self): + base = [result()] + head_only = result( + name="io.prometheus.metrics.benchmarks.HistogramBenchmark.newProbe", + score=5.0, + ) + markdown = generate_markdown( + base + [head_only], + "head", + "prometheus/client_java", + baseline_results=base, + baseline_sha="base", + baseline_repo="prometheus/client_java", + ) + + self.assertIn( + "| Benchmark | PR | Base | Head vs base | Regression verdict |", markdown + ) + self.assertIn("## New benchmarks in PR head", markdown) + self.assertIn("no base counterpart", markdown) + self.assertNotIn("Within run", markdown) + self.assertNotIn("x slower", markdown) + + +if __name__ == "__main__": + unittest.main() From 194aac706d21cc41d61adeb520b9ffaf18e17532 Mon Sep 17 00:00:00 2001 From: Gregor Zeitlinger Date: Wed, 19 Aug 2026 09:14:07 +0000 Subject: [PATCH 2/3] fix: validate benchmark execution metadata Signed-off-by: Gregor Zeitlinger --- .mise/tasks/generate_benchmark_summary.py | 55 +++++++++++++++++-- .../tasks/test_generate-benchmark-summary.py | 51 +++++++++++++++++ 2 files changed, 101 insertions(+), 5 deletions(-) diff --git a/.mise/tasks/generate_benchmark_summary.py b/.mise/tasks/generate_benchmark_summary.py index c38e7d7fa..a0166e58b 100755 --- a/.mise/tasks/generate_benchmark_summary.py +++ b/.mise/tasks/generate_benchmark_summary.py @@ -248,7 +248,7 @@ def metric_score(result: dict) -> float | None: """Extract a benchmark score as a finite float.""" try: score = float(result.get("primaryMetric", {}).get("score")) - if not math.isnan(score): + if math.isfinite(score): return score except (ValueError, TypeError): pass @@ -263,7 +263,7 @@ def score_interval(result: dict) -> tuple[float, float] | None: try: low = float(confidence[0]) high = float(confidence[1]) - if not math.isnan(low) and not math.isnan(high): + if math.isfinite(low) and math.isfinite(high): return min(low, high), max(low, high) except (ValueError, TypeError): pass @@ -287,18 +287,34 @@ def lower_is_better(result: dict) -> bool: return mode in {"avgt", "sample", "ss"} or unit.endswith("/op") +def normalize_jvm_args(value) -> list: + """Normalize optional JMH JVM argument fields for metadata comparison.""" + if value is None: + return [] + if isinstance(value, list): + return value + return [value] + + def benchmark_metadata(result: dict) -> dict: """Return metadata that must match for a base/head comparison.""" primary_metric = result.get("primaryMetric", {}) return { "jmhVersion": result.get("jmhVersion"), "mode": result.get("mode"), + "vmName": result.get("vmName"), + "vmVersion": result.get("vmVersion"), + "jvmArgs": normalize_jvm_args(result.get("jvmArgs")), + "jvmArgsPrepend": normalize_jvm_args(result.get("jvmArgsPrepend")), + "jvmArgsAppend": normalize_jvm_args(result.get("jvmArgsAppend")), "threads": result.get("threads"), "forks": result.get("forks"), "warmupIterations": result.get("warmupIterations"), "warmupTime": result.get("warmupTime"), + "warmupBatchSize": result.get("warmupBatchSize"), "measurementIterations": result.get("measurementIterations"), "measurementTime": result.get("measurementTime"), + "measurementBatchSize": result.get("measurementBatchSize"), "jdkVersion": result.get("jdkVersion"), "scoreUnit": primary_metric.get("scoreUnit"), "params": result.get("params", {}), @@ -340,7 +356,12 @@ def performance_change(head: dict, baseline: dict) -> float | None: """Return percent performance change, with positive meaning faster.""" head_score = metric_score(head) baseline_score = metric_score(baseline) - if head_score is None or baseline_score in (None, 0): + if ( + head_score is None + or baseline_score is None + or head_score == 0 + or baseline_score == 0 + ): return None if lower_is_better(head): return (float(baseline_score) / head_score - 1) * 100 @@ -354,6 +375,27 @@ def format_change(change: float | None) -> str: return f"{change:+.1f}%" +def metric_direction_note(results: list) -> str: + """Describe whether scores represent throughput or latency.""" + directions = { + "latency" if lower_is_better(result) else "throughput" for result in results + } + if directions == {"throughput"}: + return ( + "Throughput scores are higher-is-better; positive Head vs base deltas " + "indicate faster performance." + ) + if directions == {"latency"}: + return ( + "Latency scores are lower-is-better; positive Head vs base deltas " + "indicate faster performance." + ) + return ( + "Throughput scores are higher-is-better and latency scores are " + "lower-is-better; positive Head vs base deltas indicate faster performance." + ) + + def generate_comparison_section( results: list, baseline_results: list, @@ -377,7 +419,7 @@ def generate_comparison_section( md.append("") md.append(f"- **Head:** {format_commit_link(commit_sha, repo)}") md.append(f"- **Base:** {format_commit_link(baseline_sha, baseline_repo)}") - md.append("- **Change:** positive means the PR is faster than base.") + md.append(f"- **Metric direction:** {metric_direction_note(results)}") if comparison_note: md.append(f"- **Note:** {comparison_note}") if baseline_system_info: @@ -615,7 +657,10 @@ def generate_markdown( md.append("## Notes") md.append("") - md.append("- **Score** = Throughput in operations per second (higher is better)") + md.append( + "- **Score** = the JMH primary metric; " + "throughput is higher-is-better and latency is lower-is-better." + ) md.append("- **Error** = 99.9% confidence interval") if baseline_results: md.append( diff --git a/.mise/tasks/test_generate-benchmark-summary.py b/.mise/tasks/test_generate-benchmark-summary.py index 5ceb7eacc..bb1317976 100644 --- a/.mise/tasks/test_generate-benchmark-summary.py +++ b/.mise/tasks/test_generate-benchmark-summary.py @@ -65,6 +65,41 @@ def test_small_or_uncertain_change_is_within_noise(self): def test_mismatched_metadata_is_inconclusive(self): self.assertEqual(comparison_status(result(threads=1), result()), "inconclusive") + def test_execution_metadata_mismatch_is_inconclusive(self): + for field, value in ( + ("vmName", "different-vm"), + ("vmVersion", "different-vm-version"), + ("jvmArgs", ["-Xmx1g"]), + ("jvmArgsPrepend", ["-XX:+UseZGC"]), + ("jvmArgsAppend", ["-XX:-UseCompressedOops"]), + ("warmupBatchSize", 2), + ("measurementBatchSize", 2), + ): + head = result() + base = result() + head[field] = value + self.assertEqual(comparison_status(head, base), "inconclusive", field) + + def test_latency_uses_lower_is_better_direction(self): + head = result(score=94, error=1) + base = result(score=100, error=1) + head["mode"] = base["mode"] = "avgt" + head["primaryMetric"]["scoreUnit"] = base["primaryMetric"]["scoreUnit"] = "ms" + self.assertEqual(comparison_status(head, base), "meaningful improvement") + + def test_exact_practical_threshold_is_meaningful(self): + self.assertEqual( + comparison_status(result(score=105, error=0.1), result(error=0.1)), + "meaningful improvement", + ) + + def test_zero_or_non_finite_scores_are_inconclusive(self): + self.assertEqual(comparison_status(result(score=0), result()), "inconclusive") + self.assertEqual(comparison_status(result(), result(score=0)), "inconclusive") + self.assertEqual( + comparison_status(result(score=float("inf")), result()), "inconclusive" + ) + def test_missing_confidence_interval_is_inconclusive(self): head = result(score=106, error=None) self.assertEqual(comparison_status(head, result()), "inconclusive") @@ -91,9 +126,25 @@ def test_head_only_benchmarks_are_separate_and_not_ranked(self): ) self.assertIn("## New benchmarks in PR head", markdown) self.assertIn("no base counterpart", markdown) + self.assertIn("Throughput scores are higher-is-better", markdown) self.assertNotIn("Within run", markdown) self.assertNotIn("x slower", markdown) + def test_latency_note_is_mode_aware(self): + base = result() + head = result() + base["mode"] = head["mode"] = "avgt" + base["primaryMetric"]["scoreUnit"] = head["primaryMetric"]["scoreUnit"] = "ms" + markdown = generate_markdown( + [head], + "head", + "prometheus/client_java", + baseline_results=[base], + baseline_sha="base", + baseline_repo="prometheus/client_java", + ) + self.assertIn("Latency scores are lower-is-better", markdown) + if __name__ == "__main__": unittest.main() From 70e0afa86dd94d4828da997bfd69f0cd3e7be2f5 Mon Sep 17 00:00:00 2001 From: Gregor Zeitlinger Date: Wed, 19 Aug 2026 09:16:25 +0000 Subject: [PATCH 3/3] fix: reject invalid benchmark uncertainty Signed-off-by: Gregor Zeitlinger --- .mise/tasks/generate_benchmark_summary.py | 2 +- .mise/tasks/test_generate-benchmark-summary.py | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.mise/tasks/generate_benchmark_summary.py b/.mise/tasks/generate_benchmark_summary.py index a0166e58b..0da622bcb 100755 --- a/.mise/tasks/generate_benchmark_summary.py +++ b/.mise/tasks/generate_benchmark_summary.py @@ -273,7 +273,7 @@ def score_interval(result: dict) -> tuple[float, float] | None: return None try: error = float(metric.get("scoreError")) - if not math.isnan(error): + if math.isfinite(error) and error >= 0: return score - error, score + error except (ValueError, TypeError): pass diff --git a/.mise/tasks/test_generate-benchmark-summary.py b/.mise/tasks/test_generate-benchmark-summary.py index bb1317976..9918d0e49 100644 --- a/.mise/tasks/test_generate-benchmark-summary.py +++ b/.mise/tasks/test_generate-benchmark-summary.py @@ -100,6 +100,12 @@ def test_zero_or_non_finite_scores_are_inconclusive(self): comparison_status(result(score=float("inf")), result()), "inconclusive" ) + def test_invalid_fallback_uncertainty_is_inconclusive(self): + for error in (float("inf"), float("-inf"), -1.0): + head = result(error=error) + head["primaryMetric"].pop("scoreConfidence") + self.assertEqual(comparison_status(head, result()), "inconclusive", error) + def test_missing_confidence_interval_is_inconclusive(self): head = result(score=106, error=None) self.assertEqual(comparison_status(head, result()), "inconclusive")