Description
For tool: 'jmh', biggerIsBetter() in src/write.ts unconditionally
returns false:
case 'jmh':
return false;
But JMH's scoreUnit depends on @BenchmarkMode: Mode.Throughput
reports units like ops/s where higher is better, while
Mode.AverageTime/SampleTime/SingleShotTime report ms/op/us/op/
ns/op where lower is better. extractJmhResult() in src/extract.ts
already captures scoreUnit per result, but it's discarded before
getRatio()/findAlerts() compute regression ratios.
Impact
Any repo mixing Throughput-mode and Time-mode JMH benchmarks in the same
suite gets inverted alert direction for the Throughput ones: an
improvement (ops/s increases) computes ratio = current/prev > 1 and can
fire a false "Performance Alert," while a genuine regression (ops/s
decreases) computes ratio < 1 and is silently missed.
Reproduction
Run -rf json output containing both a Mode.Throughput benchmark and a
Mode.AverageTime benchmark through the action with tool: jmh,
comment-on-alert: true. The Throughput benchmark's alert direction is
backwards.
Suggested fix
Derive per-result direction from scoreUnit instead of the fixed
per-tool constant, e.g.:
- unit starts with
ops (ops/s, ops/ms, ...) → bigger is better
- unit ends with
/op (ms/op, us/op, ns/op, s/op, B/op) →
smaller is better
Happy to submit a PR for this if the direction looks right — wanted to
confirm first since it'd move biggerIsBetter from a per-tool constant to
a per-BenchmarkResult computation.
Environment
Reproduced via eXist-db/exist's
exist-core-jmh and exist-indexes-jmh benchmark suites, which mix
Mode.Throughput and Mode.AverageTime benchmarks. Checked existing
issues/PRs for this repo — nothing currently covers it, and it isn't
documented as a known limitation in the README.
🤖 Filed with Claude Code
Description
For
tool: 'jmh',biggerIsBetter()insrc/write.tsunconditionallyreturns
false:But JMH's
scoreUnitdepends on@BenchmarkMode:Mode.Throughputreports units like
ops/swhere higher is better, whileMode.AverageTime/SampleTime/SingleShotTimereportms/op/us/op/ns/opwhere lower is better.extractJmhResult()insrc/extract.tsalready captures
scoreUnitper result, but it's discarded beforegetRatio()/findAlerts()compute regression ratios.Impact
Any repo mixing Throughput-mode and Time-mode JMH benchmarks in the same
suite gets inverted alert direction for the Throughput ones: an
improvement (ops/s increases) computes
ratio = current/prev > 1and canfire a false "Performance Alert," while a genuine regression (ops/s
decreases) computes
ratio < 1and is silently missed.Reproduction
Run
-rf jsonoutput containing both aMode.Throughputbenchmark and aMode.AverageTimebenchmark through the action withtool: jmh,comment-on-alert: true. The Throughput benchmark's alert direction isbackwards.
Suggested fix
Derive per-result direction from
scoreUnitinstead of the fixedper-tool constant, e.g.:
ops(ops/s,ops/ms, ...) → bigger is better/op(ms/op,us/op,ns/op,s/op,B/op) →smaller is better
Happy to submit a PR for this if the direction looks right — wanted to
confirm first since it'd move
biggerIsBetterfrom a per-tool constant toa per-
BenchmarkResultcomputation.Environment
Reproduced via eXist-db/exist's
exist-core-jmhandexist-indexes-jmhbenchmark suites, which mixMode.ThroughputandMode.AverageTimebenchmarks. Checked existingissues/PRs for this repo — nothing currently covers it, and it isn't
documented as a known limitation in the README.
🤖 Filed with Claude Code