benchmark-action / benchmark-action/github-action-benchmark
JMH tool: biggerIsBetter is hardcoded false, inverting alert direction for Throughput-mode benchmarks
- Dominant language
- TypeScript
- Stars
- 1.3k
- Forks
- 184
- PR merge metrics
- No merged PRs in 30d
Description
## Description
For `tool: 'jmh'`, `biggerIsBetter()` in `src/write.ts` unconditionally
returns `false`:
```ts
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](https://github.com/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](https://claude.com/claude-code)
Contributor guide
Research direction
Trace the JMH result flow from src/extract.ts through src/write.ts into getRatio() and findAlerts(), focusing on where scoreUnit is discarded. Reproduce the mixed Throughput and AverageTime case with -rf json and comment-on-alert: true; done means alert direction follows ops units versus /op units for each result.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java, typescript
- Domain
- ci-cd, performance
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 64/100