benchmark-action / benchmark-action/github-action-benchmark

JMH tool: biggerIsBetter is hardcoded false, inverting alert direction for Throughput-mode benchmarks

Open
#365 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.