jamesfredley opened a new pull request, #16071: URL: https://github.com/apache/grails-core/pull/16071
## 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. Every framework change is an unmeasured bet. Apache Groovy solved this for themselves and it works well: every PR gets an automated JMH report. This brings the same capability to Grails, and improves on the parts of their design that are known to be weakest. Worth saying plainly, because it is easy to assume we are catching up: **Micronaut, Quarkus and OpenJDK have no PR-level performance check at all.** Groovy is the outlier that does. This puts Grails in that second group. ## What a reviewer actually sees Label a PR `performance`, and a comment like this appears. This is a **real report from a real run** - a 2x slowdown was deliberately injected into `matchWarmCache` to produce it: > **Regressions:** 1 **Improvements:** 0 > **Runner health:** worst ruler deviation: 11.3% > **Ruler movements:** `CpuRulerBenchmark.integerArithmetic: 0.89x`, `MemoryRulerBenchmark.allocateAndCopyArray: 1.08x` > > ⚠️ **Warning:** The runner was unstable BETWEEN the two halves of the A/B run. Treat results as unreliable. | Benchmark | Base score | Head score | Speedup | Verdict | Allocation (ADVISORY) | | --- | ---: | ---: | ---: | --- | ---: | | urlmappings.UrlMappingsBenchmark.matchWarmCache | 4.07 ns/op | 8.77 ns/op | **0.46x** | **REGRESSED** | ~0 B/op | | databinding.SimpleDataBinderBenchmark.bindFlatMap | 1.72e+04 ns/op | 1.83e+04 ns/op | 0.94x | no clear change | +17 B/op | | gsp.GroovyPageParserBenchmark.parseSmallTemplate | 9.94e+03 ns/op | 9.98e+03 ns/op | 1.00x | no clear change | +10 B/op | | interceptors.UrlMappingMatcherBenchmark.matchUriPattern | 128 ns/op | 128 ns/op | 0.99x | no clear change | +12 B/op | | urlmappings.UrlMappingsBenchmark.matchColdVariedKeys | 818 ns/op | 802 ns/op | 1.02x | no clear change | +14 B/op | | views.ViewTemplateRenderingBenchmark.renderJsonTemplate | 1.82e+04 ns/op | 1.67e+04 ns/op | 1.09x | no clear change | +12 B/op | *(abridged - the real comment lists all 13 in a collapsed `<details>` block, regressions sorted to the top)* **One regression found, zero false positives across the other twelve.** That is the number that decides whether people keep reading these comments or start muting them. A performance check that cries wolf gets ignored within a month, and then it is worse than nothing because it provides false assurance. Note the report says **"no clear change"**, never "unchanged". Failing to detect a change is not proof there wasn't one, and the wording says so. ## How it decides A benchmark is only called REGRESSED or IMPROVED when **both** hold: 1. the effect is at least 10%, **and** 2. the JMH confidence intervals for base and head are disjoint. Everything else is "no clear change". This is deliberately conservative and will miss small real regressions. That is the right trade for a per-PR check whose entire value depends on being trusted. **What it deliberately does not do:** no bootstrap resampling, no p-values, no Mann-Whitney, no Benjamini-Hochberg FDR. JMH gives ~10 iteration samples sharing a JVM and its compilation state, so they are not independent observations. Significance testing over them is pseudoreplication - it manufactures precision the measurement does not contain. Threshold plus disjoint intervals is defensible; a p-value here would not be. ## Where this improves on Groovy's design Groovy's system is the direct inspiration, including their idea of pure-Java "ruler" benchmarks. Credit where due. Three things are done differently. **1. Paired same-runner A/B instead of a historical baseline.** Groovy compares a PR run against a trailing 90-day baseline on `gh-pages`, measured on *different* runner hardware. They correct for that using rulers as a calibration factor and tolerate ±15% drift; beyond that their own report states the speedups are not meaningful. This builds **both** revisions and measures them back-to-back **on the same runner**, so there is no cross-hardware correction to make at all. A 2-shard matrix runs base-then-head and head-then-base so ordering bias cancels, and only **complete same-shard pairs** are pooled into one verdict - a shard that loses either side is dropped rather than cross-paired against a different runner. The pooled interval *spans* the shards rather than narrowing them, so shards that disagree widen uncertainty and make a verdict harder to reach, never easier. **2. Rulers as a stability check, not a calibration factor.** Same idea, different job. If any ruler moves more than 5% between the two halves of a run, the report says the runner was unstable and the numbers should not be trusted. Critically, each ruler is judged **individually**. In the run above the rulers moved 0.89x and 1.08x - which average to ~0.98x and would look perfectly healthy under a geometric mean, while both had in fact moved materially. Averaging them lets opposite movements cancel and silently hides exactly the instability the check exists to catch. (That warning firing above is honest, not a flaw: it was measured on a working laptop, and the tool said so rather than pretending the numbers were pristine.) **3. Per-benchmark verdicts, not just group geomeans.** Groovy reports a geometric mean per group. You learn `core` moved 0.989x; you do not learn which benchmark moved, or whether it was noise. This names the benchmark, gives it a verdict, and marks the group geomean explicitly descriptive rather than a judgement. Also included: allocation (`gc.alloc.rate.norm`, bytes/op) as an advisory column, often a clearer signal than wall time. ## What is benchmarked 13 benchmarks over paths that run per-request or per-object, all constructible without a Spring context or servlet container: - **urlmappings** - URI matching (warm cache, cold varied keys), reverse URL creation - **databinding** - binding a map onto an object, with and without type conversion - **gsp** - GSP parsing, template text to generated Groovy source - **interceptors** - interceptor URI match decisions - **views** - JSON and markup view rendering - **ruler** - two pure-JDK probes used only to detect an unstable runner Benchmarks are Java; only the setup fixtures are Groovy. A Groovy-authored benchmark would measure Groovy's dynamic call-site machinery as much as the Grails API under test, so Groovy is confined to building the closure-based fixtures the URL mappings DSL and view templates require. **Worth knowing what this already caught, before it ever ran in CI.** The URL mappings fixture was initially written with single-quoted strings, so `$category` and `$id` were literal text rather than DSL capture tokens. `match()` returned `null`, and the three most valuable benchmarks were confidently timing *failed lookups* - with no error. Fixing it moved `matchWarmCache` from ~230 ns to **4.6 ns**, against ~780 ns for a cold varied-key match. That ~170x gap is the cache-effectiveness signal this suite exists to defend, and it was completely invisible beforehand. The benchmark now asserts in `@Setup` that the fixture really matches and produces `/catalog/books/42`, and fails the run rather than publishing wrong numbers. ## Cost, and why it is opt-in This is expensive, so it does **not** run on every PR. It runs only when a PR carries the `performance` label, or via manual `workflow_dispatch`. Doc-only changes are filtered out regardless. Given the ASF-wide Actions budget, defaulting this on for every PR is not a reasonable use of shared CI. **It is advisory and never fails a build.** A regression is information for the reviewer, not a gate. If we later want to gate, that should be a separate and deliberate discussion. ## Known gap: validation `Validateable.validate()` belongs here and is **not** included. A benchmark was written, then withdrawn: it measured 80-160% relative error against 15-20% for the pure-JDK rulers on the same idle machine. At that spread the intervals can never separate, so it would report "no clear change" no matter what happened to validation performance, while still burning CI time and contributing a meaningless number to a group geomean. Two causes were ruled out first - construction was moved into `@Setup`, and errors were reset per invocation (`doValidate()` copies existing errors into the new set, so repeatedly validating a failing object grows it without bound). Neither fixed the variance, which points at bimodal behaviour across forks. `README.adoc` records this with suggested next steps. Shipping a benchmark that can never produce a verdict seemed worse than shipping an honest gap. ## Security Uses `pull_request`, **never** `pull_request_target`, so untrusted PR code never runs with a writable token. Fork PRs get the job summary and downloadable artifacts but no comment, because their token is read-only by design. Only the reporting job - which runs solely for same-repository PRs whose authors already have push access - holds `pull-requests: write`. PR-controlled values pass through `env:` rather than into shell, and benchmark names are sanitised before rendering into a comment. ## Verification - 30 stdlib unit tests for the comparison script; `actionlint` clean; `./gradlew rat` passes with 0 unapproved files - All 13 benchmarks executed end-to-end, repeatedly, on real hardware - The full paired 2-shard flow was run against a deliberately injected 2x regression and reported exactly 1 REGRESSED with 0 false positives - The artifact-pairing logic was separately proven against a simulated tree with an incomplete shard, confirming the incomplete shard is dropped rather than cross-paired - The `@Setup` fixture guard was proven by reverting the mappings bug and confirming the run fails loudly instead of publishing wrong numbers - JMH is GPLv2-with-Classpath-Exception (ASF Category X). Acceptable only because the module is build-time-only and never published, which the build enforces and `README.adoc` records for future release managers. ## Follow-ups deliberately out of scope Long-term trend history on `gh-pages`, a dashboard, gating on regressions, and restoring the validation benchmark. This PR is intentionally the smallest thing that produces a signal worth trusting. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
