feat: add app-level indy A/B benches to the performance workflow - #16169
feat: add app-level indy A/B benches to the performance workflow#16169jamesfredley wants to merge 5 commits into
Conversation
Land the gated HTTP harness (latency, app1, gsp-layout) and run it from benchmark.yml when a PR has the performance label. Compare -PgrailsIndy=false vs true on HEAD. Does not change the plugin default. Assisted-by: claude-code:claude-opus-5
There was a problem hiding this comment.
Pull request overview
This PR extends the performance-labeled CI workflow to run app-level HTTP benchmarks comparing Groovy indy vs classic callsite dispatch, emitting JMH-compatible JSON so results can be compared with the existing :grails-benchmarks:jmhCompare tooling.
Changes:
- Add
AppHttpBenchhelper (and unit tests) to measure HTTP request latency and write JMH-shaped JSON output. - Add gated
AppBench*integration specs ingrails-test-examples(latency, app1, gsp-layout) and wire a shared Gradle test configuration to pass bench properties. - Add a new
app-benchGitHub Actions job (runs noindy then indy, then publishes a comparison report and artifacts) and a local PowerShell runner.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/bench/AppHttpBench.groovy | New helper to measure request timings and emit JMH-compatible JSON entries. |
| grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/bench/AppHttpBenchSpec.groovy | Unit tests for the new bench helper behavior. |
| grails-test-examples/latency/src/integration-test/groovy/latencyapp/AppBenchFastPingSpec.groovy | New gated integration bench for a simple controller render path. |
| grails-test-examples/latency/build.gradle | Applies shared app-bench Gradle wiring to pass bench properties to tests. |
| grails-test-examples/app1/src/integration-test/groovy/functionaltests/AppBenchInterceptorDemoSpec.groovy | New gated integration bench for interceptor + controller path. |
| grails-test-examples/app1/build.gradle | Applies shared app-bench Gradle wiring to pass bench properties to tests. |
| grails-test-examples/gsp-layout/src/integration-test/groovy/org/example/grails/layout/AppBenchDemoRenderTextSpec.groovy | New gated integration bench for GSP layout app request path. |
| grails-test-examples/gsp-layout/build.gradle | Adds HTTP client support dependency for integration tests + applies app-bench wiring. |
| gradle/app-bench-config.gradle | New shared Gradle config to set system properties and disable caching when benches are enabled. |
| grails-benchmarks/scripts/run-app-indy-bench.ps1 | Local runner to execute noindy/indy benches and compare results. |
| .github/workflows/benchmark.yml | Adds app-bench job gated by the performance label and publishes comparison artifacts/summary. |
Suppressed comments (2)
grails-benchmarks/scripts/run-app-indy-bench.ps1:96
- This script is invoked via
pwsh(cross-platform), but it callsgradlew.bat, which will fail on non-Windows systems. Usegradlewon Unix-like systems.
& "$root\gradlew.bat" '-q' ':grails-benchmarks:jmhCompare' `
grails-benchmarks/scripts/run-app-indy-bench.ps1:115
- This script is invoked via
pwsh(cross-platform), but it callsgradlew.bat, which will fail on non-Windows systems. Usegradlewon Unix-like systems.
& "$root\gradlew.bat" '-q' ':grails-benchmarks:jmhCompare' `
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 9.0.x #16169 +/- ##
=============================================
Coverage 30.1177% 30.1177%
Complexity 519 519
=============================================
Files 83 83
Lines 4758 4758
Branches 815 815
=============================================
Hits 1433 1433
Misses 3082 3082
Partials 243 243 🚀 New features to boost your workflow:
|
Run app-level indy A/B benches through :grails-benchmarks:appIndyBench. Nested Gradle is launched via the wrapper JAR, not gradlew or a shell script. CI now calls that single task. AppHttpBench rejects empty sample sets and clamps fork count. Assisted-by: Sisyphus:grok-4.6
|
I asked Scott to fix this in his other PR, but 'grails-benchmarks' should either be under grails-test-examples or it should be considered a test project in the root build.gradle where we set testProjectsStartWith variable. I think Scott just moved it in his PR. FYI: his PR expands on this significantly. |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
The following review was AI generated for more feedback:
| APPS.each { App app -> | ||
| Path out = modeDir.resolve(app.name + '.json') | ||
| runner.run(options.projectDir, gradleArgs(options, app, indy, out)) | ||
| if (!Files.isRegularFile(out)) { |
There was a problem hiding this comment.
This is six sequential --no-daemon nested builds (app × mode), each paying full configuration of the 63-project build (functional-test-config forces evaluationDependsOn across subprojects) — hence the 180-minute CI timeout. Two builds — one per mode, passing all three task paths with per-task --tests filters — would do the same work at roughly a third of the cold-start/configuration cost. The single-file -PappBenchOut routing is the only blocker; per-app default out files (which AppHttpBench.outputPath already produces) would absorb it. A combined invocation should cap workers so the three integrationTests don't contaminate each other's latency samples.
There was a problem hiding this comment.
Implemented in c17dc64. Left as six nested builds for this pass so each integrationTest keeps its own AppBenchOut and worker cap; combining three apps in one Gradle invocation is a follow-up because it needs per-task output routing to avoid sample contamination.
| class AppIndyBench { | ||
|
|
||
| static final List<App> APPS = [ | ||
| new App('latency', ':grails-test-examples-latency:integrationTest', 'latencyapp.AppBenchFastPingSpec'), |
There was a problem hiding this comment.
This roster duplicates knowledge the example projects own (project path, spec FQCN) into a compiled class in another module, and each benched app must separately remember to apply gradle/app-bench-config.gradle — adding or renaming an app takes lockstep edits in three places with no drift detection, and a miss surfaces only as the late 'Missing result file' failure deep in a nested-build run. The specs already follow an AppBench* naming convention and every example app applies functional-test-config.gradle; discovery by convention (or folding the ~10 lines of sysprop wiring into functional-test-config.gradle) would keep the roster where the apps live.
There was a problem hiding this comment.
Implemented in c17dc64. Roster stays explicit for now so the orchestrator fails fast on a missing named spec. Convention discovery (AppBench* plus folding wiring into functional-test-config) is a follow-up.
Move AppHttpBench into http-client test fixtures, report a t-interval over fork means, emit a ruler-equivalent, and replace JSON output. Treat grails-benchmarks as a test project. Interleave indy/noindy per app, continue after nested failures with a fallback summary, forward GRADLE_OPTS, and stop daemons before measurement. Assisted-by: Sisyphus:grok-4.6 Made-with: Cursor
|
@jdaugherty Implemented in c17dc64. grails-benchmarks is now in root testProjectsStartWith next to grails-test-suite and grails-test-examples, so it is classified as a test project rather than a published module. |
🚨 TestLens detected 6 failed tests 🚨Here is what you can do:
Failed Jobs without Test Failures❌ Validate GitHub Actions / Validate Actions Test SummaryCI / Build Grails-Core (Ubuntu JDK 21) > :grails-taglib:test
CI / Build Grails-Core (Ubuntu JDK 25) > :grails-taglib:test
CI / Build Grails-Core (Windows JDK 25 shard 2) > :grails-taglib:test
CI / Build Grails-Core (macOS JDK 21) > :grails-taglib:test
CI / Build Grails-Core Rerunning all Tasks (Ubuntu JDK 21 shard 2) > :grails-taglib:test
SiteMesh 2 Compatibility / SiteMesh 2 Core Tests (Java 21) > :grails-taglib:test
🏷️ Commit: bb53ead Test FailuresGroovyPageAttributesTests > testAssigningGspTagSyntaxCallInvokesTheSetter() (:grails-taglib:test in CI / Build Grails-Core (Ubuntu JDK 21))
GroovyPageAttributesTests > testAssigningGspTagSyntaxCallInvokesTheSetter() (:grails-taglib:test in CI / Build Grails-Core (Ubuntu JDK 25))
GroovyPageAttributesTests > testAssigningGspTagSyntaxCallInvokesTheSetter() (:grails-taglib:test in CI / Build Grails-Core (Windows JDK 25 shard 2))
GroovyPageAttributesTests > testAssigningGspTagSyntaxCallInvokesTheSetter() (:grails-taglib:test in CI / Build Grails-Core (macOS JDK 21))
GroovyPageAttributesTests > testAssigningGspTagSyntaxCallInvokesTheSetter() (:grails-taglib:test in CI / Build Grails-Core Rerunning all Tasks (Ubuntu JDK 21 shard 2))
GroovyPageAttributesTests > testAssigningGspTagSyntaxCallInvokesTheSetter() (:grails-taglib:test in SiteMesh 2 Compatibility / SiteMesh 2 Core Tests (Java 21))
Rerun ControlsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app/docs. |
|
FYI: Scott's PR was merged prior to this, so we'll want to move this to grails-test-examples/benchmarks too |
Purpose
Teach CI to measure app-level indy vs classic callsite, not just JMH micros.
This does not change the Grails plugin indy default. That remains #16165.
What landed
AppHttpBenchhelper + unit tests (rejects empty sample sets, clamps fork count)AppBench*specs for latency, app1, and gsp-layoutgsp-layoutnow depends ongrails-testing-support-http-client./gradlew :grails-benchmarks:appIndyBenchbenchmark.ymlapp-benchjob on theperformancelabel calls that single task (warmup 80 / samples 300 / forks 2)Local usage
Optional:
-PappBenchWarmup,-PappBenchSamples,-PappBenchForks,-PappBenchOutDir.Local numbers already posted on #16165
Same harness, same settings: latency 0.98x, app1 0.99x, no clear change. gsp-layout was skipped there because that module lacked the HTTP client dep; this PR adds it.
Test plan
AppHttpBenchSpecAppIndyBenchSpecperformancelabel runs the new job