Add JMH performance regression reporting for pull requests - #16071
Add JMH performance regression reporting for pull requests#16071jamesfredley wants to merge 5 commits into
Conversation
Grails has no per-PR performance signal today, so a regression in a hot path ships and only surfaces later as a general "upgrading made us slower" report, with no way to attribute it to a change. This adds an advisory JMH benchmark suite and an opt-in workflow that measures the pull request against its base commit and posts the comparison as a sticky comment. The comparison is paired on a single runner rather than against a stored historical baseline: both revisions are built first, then measured back-to-back, in a two shard matrix where the shards run the two revisions in opposite orders. Only complete same-shard base/head pairs are pooled, and pooling spans the shards' confidence intervals rather than narrowing them, so shards that disagree widen uncertainty. A benchmark is only reported as regressed or improved when the effect is at least 10 percent and the JMH confidence intervals are disjoint; everything else is reported as no clear change. Bootstrap resampling and significance testing were deliberately not used, because JMH's iteration samples share a JVM and its compilation state and are not independent observations. Pure JDK "ruler" benchmarks detect a runner that was unstable between the two halves of a run. Each ruler is evaluated per shard pair, since pooling first lets opposite movements cancel and hide the instability they exist to surface. The check never fails a build and runs only on pull requests labelled performance. grails-benchmarks is build-time only and is not published. JMH is GPLv2 with Classpath Exception, which is ASF Category X, so the module deliberately omits the publish, sbom, vulnerability-scan, jacoco and dependency-validator conventions and is absent from the BOM. Assisted-by: claude-code:claude-opus-5
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in (label-driven) PR-level JMH benchmark comparison system to the Grails framework repo, including a new grails-benchmarks module (JMH suite + fixtures) and a GitHub Actions workflow that runs paired base/head benchmarks and posts an advisory comparison comment on the PR.
Changes:
- Introduces a new
grails-benchmarksGradle subproject containing JMH benchmarks (Java) and Grails/Groovy fixtures. - Adds a new GitHub Actions workflow to run paired, same-runner A/B benchmark shards and publish results (summary + PR comment for same-repo PRs).
- Adds a Python comparison/reporting script (plus unit tests) to render conservative per-benchmark verdicts and runner-health signals.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle | Includes the new grails-benchmarks subproject in the multi-project build. |
| grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/views/ViewTemplateFixture.groovy | Groovy fixtures for creating JSON/markup view templates used by JMH. |
| grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/urlmappings/UrlMappingsFixture.groovy | Groovy fixture that builds URL mappings for benchmarking URI matching and reverse routing. |
| grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/interceptors/InterceptorFixture.groovy | Minimal interceptor fixture used by interceptor matching benchmarks. |
| grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/views/ViewTemplateRenderingBenchmark.java | JMH benchmark for rendering pre-created JSON/markup view templates. |
| grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/urlmappings/UrlMappingsBenchmark.java | JMH benchmarks for URL mapping match (warm/cold) and reverse URL creation, with fixture guards. |
| grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/ruler/MemoryRulerBenchmark.java | “Ruler” benchmark for runner stability detection (memory-related). |
| grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/ruler/CpuRulerBenchmark.java | “Ruler” benchmark for runner stability detection (CPU-related). |
| grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/interceptors/UrlMappingMatcherBenchmark.java | JMH benchmarks for interceptor URI matcher accept/reject decisions. |
| grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/gsp/GroovyPageParserBenchmark.java | JMH benchmark for GSP parsing into generated Groovy source. |
| grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/databinding/SimpleDataBinderBenchmark.java | JMH benchmarks for databinding with and without type conversion. |
| grails-benchmarks/README.adoc | Module documentation (local run instructions, properties, rationale, known gaps). |
| grails-benchmarks/build.gradle | Benchmark module build (JMH plugin config, deps, metadata merge for classpath services, lifecycle wiring). |
| .gitignore | Ignores Python bytecode caches introduced by the new script/tests. |
| .github/workflows/benchmark.yml | Adds the opt-in paired JMH workflow (run shards, pool results, render and comment on PR). |
| .github/scripts/test_jmh_compare.py | Adds unit tests for the JMH comparison/reporting logic. |
| .github/scripts/jmh_compare.py | Adds Python implementation for parsing/pooling/comparing JMH results and rendering PR-friendly reports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback on the JMH benchmark reporting workflow. The classpath metadata merge read line-oriented entries with getInputStream(entry).getText(...) and never closed the stream, while the two neighbouring Properties reads already used withCloseable. That branch handles every META-INF/services and META-INF/groovy entry across the whole benchmark runtime classpath, so it was the most frequently executed of the three and could leak file handles across a large dependency set. It now closes the stream like the others; the merged output is unchanged. The comparison script's unit tests were not executed anywhere, leaving the reporting logic unguarded against regressions. They now run in the benchmark job immediately after checkout, before the JDK and Gradle setup and before the paired build and measurement steps, so a broken reporter fails within seconds rather than after an hour of benchmarking. Running them in the shard job also covers pull requests from forks, which never reach the reporting job. That step is deliberately permitted to fail the job. The workflow's advisory-only rule applies to performance regressions; a comparison script that does not pass its own tests invalidates every number the run produces, so it should stop the run rather than publish results. Assisted-by: claude-code:claude-opus-5
|
Just FYI, we find the graphs Groovy has super useful, the per PR/commit regression stats not so much because of the wide variance of GitHub runner speed. It does some "ruler adjusting" to try to account for runner speed but even then it isn't super accurate - we just use it as a guide to then check things manually on a dedicated machine. |
b06d517 to
ed2db8a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16071 +/- ##
==================================================
+ Coverage 51.8811% 52.3343% +0.4532%
- Complexity 18118 18287 +169
==================================================
Files 2046 2036 -10
Lines 96274 96346 +72
Branches 16727 16831 +104
==================================================
+ Hits 49948 50422 +474
+ Misses 38954 38499 -455
- Partials 7372 7425 +53 🚀 New features to boost your workflow:
|
|
Why are we using python in this PR? |
Port jmh_compare.py into a Gradle-wired Groovy report source set so the benchmark harness stays on the JDK/Groovy toolchain already required by this repository, instead of a free-standing Python script. The comparison, shard pooling, markdown rendering, and sticky PR comment posting live under grails-benchmarks/src/report and are exercised by Spock specs plus byte-identical golden fixtures captured from the previous Python output. The performance workflow and README now invoke :grails-benchmarks:jmhCompare. Assisted-by: Sisyphus:xai/grok-4.5
|
The implementation was based on https://github.com/apache/groovy/blob/master/subprojects/performance/dashboard/jmh-summary.py, didn't notice the Python, but will make it Groovy instead. |
b8b5da6 to
e04c902
Compare
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Initial AI Review:
Reviewed the workflow, the report source set, the benchmarks and fixtures, and the golden tests. I checked the branch out and ran :grails-benchmarks:test (green, 43 tests, and the module is picked up by the aggregate test report), and I wrote a throwaway probe against sourceSets.jmh.runtimeClasspath to confirm every fixture actually does work today — doesMatch returns true/false as intended, both binder targets come back fully populated, and both templates render non-empty output. So nothing here is currently measuring a no-op.
The design holds up. Same-runner paired A/B removes the cross-hardware correction that weakens Groovy's version, per-ruler judgement rather than a geomean is the right call and the reasoning for it is correct, and declining to put p-values on ~10 JMH iterations that share a JVM is the honest choice. Verdict logic, shard pairing, the union-of-intervals pooling, and the sticky-comment path all read correctly, and the action pins and Develocity wiring match the other workflows.
Comments below are mostly cost, robustness, and one dead CLI flag. Two I'd want resolved before merge: the labeled re-trigger (a full two-shard run fires on any label change once performance is present) and the sticky comment being overwritten with an empty report when a run fails early. The rest are smaller.
Narrow the performance-label re-trigger so other labels do not re-run the two-shard suite, stop Gradle daemons before measurement, and leave the sticky PR comment intact when a run produces no HEAD results. Drop the unused --fail-on-regression CLI flag, add @setup fixture guards to the remaining silent-no-op-prone benchmarks, enable code-style and vulnerability-scan for grails-benchmarks, document why jmhClasses is on check, and rename leftover Python golden references. Assisted-by: Sisyphus:grok-4.5
✅ All tests passed ✅🏷️ Commit: 202d206 Learn more about TestLens at testlens.app. |
Why
Today a performance regression in Grails just ships. There is no gate, no signal, no attribution. It surfaces months later as "our app got slower after upgrading", by which point nobody can point at the commit responsible.
This PR adds opt-in PR-level JMH comparison for Grails, inspired by Apache Groovy's approach, with a paired same-runner design that avoids cross-hardware calibration.
What it does
Label a PR
performance(or run viaworkflow_dispatch) and a sticky advisory comment appears with per-benchmark verdicts.A benchmark is only REGRESSED or IMPROVED when both hold:
Everything else is "no clear change". Allocation (
gc.alloc.rate.norm) is advisory. Pure-JDK ruler benchmarks are a runner-stability check only - each ruler is judged individually, never geomean-averaged - and are excluded from verdicts.Design choices
reportsource set undergrails-benchmarks(:grails-benchmarks:jmhCompare). No Python.What is benchmarked
13 benchmarks over per-request / per-object paths, without a Spring context:
Benchmarks are Java; Groovy is confined to fixtures.
@Setupguards fail the run rather than publish silent no-ops when a fixture stops matching or binding/rendering correctly.Cost and triggers
Expensive, so opt-in only:
performancelabel, or viaworkflow_dispatchlabeledevents, only when the label just added isperformance(other labels on an already-tagged PR do not re-run)Security
Uses
pull_request, neverpull_request_target. Fork PRs get job summaries and artifacts but no comment. Only same-repo PRs reach the reporting job withpull-requests: write.Module layout
grails-benchmarks- JMH suite, fixtures, Groovy report source set, Spock tests (including golden fixtures).github/workflows/benchmark.yml- paired shard workflow + sticky comment postingKnown gap
Validateable.validate()is not included: measured variance was too high for intervals to separate. Documented inREADME.adoc.Verification
:grails-benchmarks:testgreen (golden fixtures + comparator/CLI/posting coverage)Out of scope
Long-term trend history, dashboard, gating on regressions, restoring validation benchmarks.