jdaugherty commented on code in PR #16169: URL: https://github.com/apache/grails-core/pull/16169#discussion_r3920279193
########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/AppIndyBench.groovy: ########## @@ -0,0 +1,244 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.grails.benchmarks.report + +import groovy.transform.CompileStatic + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.StandardOpenOption + +@CompileStatic +class AppIndyBench { + + static final List<App> APPS = [ + new App('latency', ':grails-test-examples-latency:integrationTest', 'latencyapp.AppBenchFastPingSpec'), + new App('app1', ':grails-test-examples-app1:integrationTest', 'functionaltests.AppBenchInterceptorDemoSpec'), + new App('gsp-layout', ':grails-test-examples-gsp-layout:integrationTest', 'org.example.grails.layout.AppBenchDemoRenderTextSpec') + ].asImmutable() + + static void main(String[] args) { + int exit = run(args, new WrapperGradleRunner()) + if (exit != 0) { + System.exit(exit) + } + } + + static int run(String[] args, GradleRunner runner) { + return run(args, runner, new GitHubComments(), System.getenv()) + } + + static int run(String[] args, GradleRunner runner, CommentPoster poster, Map<String, String> environment) { + try { + Options options = parse(args) + Path noindyDir = recreateDirectory(options.outputDir.resolve('noindy')) + Path indyDir = recreateDirectory(options.outputDir.resolve('indy')) + + ['false', 'true'].each { String indy -> + Path modeDir = indy == 'true' ? indyDir : noindyDir Review Comment: The A/B always runs all `noindy` builds first and all `indy` builds second on the same runner, so first-vs-second runner-state drift (thermal throttle, page/dependency caches, CPU frequency) lands systematically on the indy side. The JMH job in this same workflow alternates shard order specifically to cancel this bias (see the header comments in benchmark.yml). Over the ~hour between the paired measurements, monotonic drift shows up in the report as an indy regression/improvement with no cancellation mechanism. Consider alternating mode order per app or interleaving repeats so ordering bias cancels. ########## grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/bench/AppHttpBench.groovy: ########## @@ -0,0 +1,223 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.testing.http.client.bench + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.Paths + +import groovy.json.JsonOutput +import groovy.transform.CompileStatic + +/** + * App-level HTTP microbench helper that emits JMH-compatible JSON so results can be compared + * with {@code :grails-benchmarks:jmhCompare} using the same methodology as the framework JMH suite. + * + * <p>Measurement model (deliberately simple and reproducible on one machine): + * <ul> + * <li>warm up the full Spring Boot stack with {@code warmup} requests (discarded)</li> + * <li>collect {@code samples} timed requests as one raw series</li> + * <li>split the series into {@code forks} equal chunks to mimic JMH multi-fork rawData shape</li> + * <li>report mean ns/op with a simple standard-error-based scoreError</li> + * </ul> + * + * <p>Enable gated specs with {@code -PappBench=true}. Optional properties: + * {@code appBenchWarmup}, {@code appBenchSamples}, {@code appBenchForks}, {@code appBenchOut}. + */ +@CompileStatic +final class AppHttpBench { + + private AppHttpBench() { + } + + static boolean enabled() { + Boolean.getBoolean('app.bench') || Boolean.parseBoolean(System.getProperty('appBench', 'false')) + } + + static int warmupCount() { + Integer.getInteger('app.bench.warmup', Integer.getInteger('appBenchWarmup', 200)) + } + + static int sampleCount() { + Integer.getInteger('app.bench.samples', Integer.getInteger('appBenchSamples', 1000)) + } + + static int forkCount() { + Integer.getInteger('app.bench.forks', Integer.getInteger('appBenchForks', 2)) + } + + static Path outputPath(String defaultFileName) { + String configured = System.getProperty('app.bench.out', System.getProperty('appBenchOut', '')) + if (configured) { + return Paths.get(configured) + } + Path dir = Paths.get('build', 'app-bench') + Files.createDirectories(dir) + return dir.resolve(defaultFileName) + } + + /** + * Time a single request body. The closure must perform the HTTP call and assert success. + * + * @return elapsed nanoseconds + */ + static long timeNanos(Closure<?> request) { + long start = System.nanoTime() + request.call() + return System.nanoTime() - start + } + + /** + * Warm up, sample, and append one JMH-shaped benchmark entry to {@code out}. + * + * @param benchmark fully-qualified-style name, e.g. {@code appbench.latency.FastPing.httpGet} + * @param request closure that performs one successful request + */ + static void measureAndWrite(String benchmark, Path out, Closure<?> request) { + int warmup = Math.max(0, warmupCount()) + int samples = sampleCount() + if (samples < 1) { + throw new IllegalArgumentException("app.bench.samples must be >= 1, was ${samples}") + } + + for (int i = 0; i < warmup; i++) { + request.call() + } + + double[] values = new double[samples] + for (int i = 0; i < samples; i++) { + values[i] = (double) timeNanos(request) + } + + Map<String, Object> entry = toJmhEntry(benchmark, values, forkCount()) + appendEntry(out, entry) + } + + static Map<String, Object> toJmhEntry(String benchmark, double[] values, int forks) { + if (values == null || values.length == 0) { + throw new IllegalArgumentException('values must contain at least one sample') + } + double mean = mean(values) + double stdev = stdev(values, mean) + double scoreError = stdev * 1.96d / Math.sqrt((double) values.length) + + int forkCount = Math.min(Math.max(1, forks), values.length) + int perFork = Math.max(1, values.length.intdiv(forkCount)) + List<List<Double>> rawData = new ArrayList<>(forkCount) + int offset = 0 + for (int f = 0; f < forkCount; f++) { + int end = (f == forkCount - 1) ? values.length : Math.min(values.length, offset + perFork) + List<Double> chunk = new ArrayList<>(Math.max(0, end - offset)) + for (int i = offset; i < end; i++) { + chunk.add(values[i]) + } + rawData.add(chunk) + offset = end + } + + Map<String, Object> percentiles = new LinkedHashMap<>() + double[] sorted = Arrays.copyOf(values, values.length) + Arrays.sort(sorted) + percentiles.put('0.0', sorted[0]) + percentiles.put('50.0', percentile(sorted, 0.50d)) + percentiles.put('90.0', percentile(sorted, 0.90d)) + percentiles.put('95.0', percentile(sorted, 0.95d)) + percentiles.put('99.0', percentile(sorted, 0.99d)) + percentiles.put('100.0', sorted[sorted.length - 1]) + + Map<String, Object> primary = new LinkedHashMap<>() + primary.put('score', mean) + primary.put('scoreError', scoreError) + primary.put('scoreConfidence', [mean - scoreError, mean + scoreError]) + primary.put('scorePercentiles', percentiles) + primary.put('scoreUnit', 'ns/op') + primary.put('rawData', rawData) + + Map<String, Object> entry = new LinkedHashMap<>() + entry.put('jmhVersion', 'app-bench-1.0') + entry.put('benchmark', benchmark) + entry.put('mode', 'avgt') + entry.put('threads', 1) + entry.put('forks', forkCount) + entry.put('jdkVersion', System.getProperty('java.version', 'unknown')) + entry.put('vmName', System.getProperty('java.vm.name', 'unknown')) + entry.put('vmVersion', System.getProperty('java.vm.version', 'unknown')) + entry.put('warmupIterations', 1) + entry.put('warmupTime', "${warmupCount()} reqs") + entry.put('measurementIterations', values.length) + entry.put('measurementTime', '1 req') + entry.put('primaryMetric', primary) + entry.put('secondaryMetrics', Collections.emptyMap()) + return entry + } + + static void appendEntry(Path out, Map<String, Object> entry) { + List<Object> entries = new ArrayList<>() + if (Files.exists(out)) { + String existing = Files.readString(out, StandardCharsets.UTF_8).trim() + if (existing.startsWith('[')) { + Object parsed = new groovy.json.JsonSlurper().parseText(existing) Review Comment: Three issues in the existing-file path: content that doesn't start with `[` is silently discarded and overwritten; a truncated array (JVM killed mid-write) throws an uncaught JsonException; and in the documented manual flow (`-PappBench=true` with the default out file, `upToDateWhen { false }` forcing reruns) each rerun appends another entry with the same benchmark name — `JmhResults.parseEntries` keys by identity, so only the last entry wins and earlier runs are silently ignored while the file looks pooled. Only the orchestrator's `recreateDirectory` avoids this. Suggest replacing the file rather than appending, or failing loudly on unexpected existing content. ########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/AppIndyBench.groovy: ########## @@ -0,0 +1,244 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.grails.benchmarks.report + +import groovy.transform.CompileStatic + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.StandardOpenOption + +@CompileStatic +class AppIndyBench { + + static final List<App> APPS = [ + new App('latency', ':grails-test-examples-latency:integrationTest', 'latencyapp.AppBenchFastPingSpec'), + new App('app1', ':grails-test-examples-app1:integrationTest', 'functionaltests.AppBenchInterceptorDemoSpec'), + new App('gsp-layout', ':grails-test-examples-gsp-layout:integrationTest', 'org.example.grails.layout.AppBenchDemoRenderTextSpec') + ].asImmutable() + + static void main(String[] args) { + int exit = run(args, new WrapperGradleRunner()) + if (exit != 0) { + System.exit(exit) + } + } + + static int run(String[] args, GradleRunner runner) { + return run(args, runner, new GitHubComments(), System.getenv()) + } + + static int run(String[] args, GradleRunner runner, CommentPoster poster, Map<String, String> environment) { + try { + Options options = parse(args) + Path noindyDir = recreateDirectory(options.outputDir.resolve('noindy')) + Path indyDir = recreateDirectory(options.outputDir.resolve('indy')) + + ['false', 'true'].each { String indy -> + Path modeDir = indy == 'true' ? indyDir : noindyDir + APPS.each { App app -> + Path out = modeDir.resolve(app.name + '.json') + runner.run(options.projectDir, gradleArgs(options, app, indy, out)) + if (!Files.isRegularFile(out)) { + throw new IllegalStateException("Missing result file: ${out}") + } + } + } + + Path report = options.outputDir.resolve('indy-vs-noindy.md') + int compareExit = JmhCompare.run( + ['--base', noindyDir.toString(), '--head', indyDir.toString(), '--output', report.toString()] as String[], + poster, + environment + ) + if (compareExit != 0) { + return compareExit + } + appendStepSummary(report, environment) + return 0 + } catch (Exception error) { + error.printStackTrace(System.err) + return 2 + } + } + + static List<String> gradleArgs(Options options, App app, String indy, Path out) { + return [ + '--no-daemon', + "--max-workers=${options.maxWorkers}".toString(), + app.task, + '--tests', + app.tests, + "-PgrailsIndy=${indy}".toString(), + '-PappBench=true', + "-PappBenchWarmup=${options.warmup}".toString(), + "-PappBenchSamples=${options.samples}".toString(), + "-PappBenchForks=${options.forks}".toString(), + "-PappBenchOut=${out.toAbsolutePath()}".toString() + ] + } + + static Options parse(String[] args) { + Set<String> values = ['project-dir', 'output-dir', 'warmup', 'samples', 'forks', 'max-workers'] as Set<String> + Map<String, String> options = new LinkedHashMap<>() + for (int index = 0; index < args.length; index++) { + String option = args[index] + if (!option.startsWith('--')) { + throw new IllegalArgumentException("unknown option: ${option}") + } + String key = option.substring(2) + if (!values.contains(key)) { + throw new IllegalArgumentException("unknown option: --${key}") + } + if (index + 1 >= args.length) { + throw new IllegalArgumentException("missing value for --${key}") + } + options.put(key, args[++index]) + } + String projectDirValue = options.get('project-dir') + if (!projectDirValue) { + throw new IllegalArgumentException('--project-dir is required') + } + Path projectDir = Path.of(projectDirValue).toAbsolutePath().normalize() + Path outputDir = options.containsKey('output-dir') + ? Path.of(options.get('output-dir')).toAbsolutePath().normalize() + : projectDir.resolve('build').resolve('app-bench') + return new Options( + projectDir, + outputDir, + parsePositiveInt(options.getOrDefault('warmup', '200'), 'warmup', true), + parsePositiveInt(options.getOrDefault('samples', '1000'), 'samples', false), + parsePositiveInt(options.getOrDefault('forks', '2'), 'forks', false), + parsePositiveInt(options.getOrDefault('max-workers', '4'), 'max-workers', false) + ) + } + + private static int parsePositiveInt(String raw, String name, boolean allowZero) { + int value + try { + value = Integer.parseInt(raw) + } catch (NumberFormatException ignored) { + throw new IllegalArgumentException("--${name} must be an integer") + } + if (value < 0 || (!allowZero && value < 1)) { + throw new IllegalArgumentException("--${name} must be ${allowZero ? '>= 0' : '>= 1'}") + } + return value + } + + private static void appendStepSummary(Path report, Map<String, String> environment) { + String summary = environment.get('GITHUB_STEP_SUMMARY') + if (!summary || !Files.isRegularFile(report)) { + return + } + Files.writeString( + Path.of(summary), + Files.readString(report, StandardCharsets.UTF_8), + StandardCharsets.UTF_8, + StandardOpenOption.CREATE, + StandardOpenOption.APPEND + ) + } + + static Path recreateDirectory(Path directory) { + if (Files.exists(directory)) { + Files.walk(directory).withCloseable { stream -> + stream.sorted(Comparator.reverseOrder()).forEach { Path path -> Files.deleteIfExists(path) } + } + } + return Files.createDirectories(directory) + } + + @CompileStatic + static final class App { + final String name + final String task + final String tests + + App(String name, String task, String tests) { + this.name = name + this.task = task + this.tests = tests + } + } + + @CompileStatic + static final class Options { + final Path projectDir + final Path outputDir + final int warmup + final int samples + final int forks + final int maxWorkers + + Options(Path projectDir, Path outputDir, int warmup, int samples, int forks, int maxWorkers) { + this.projectDir = projectDir + this.outputDir = outputDir + this.warmup = warmup + this.samples = samples + this.forks = forks + this.maxWorkers = maxWorkers + } + } + + @CompileStatic + interface GradleRunner { + void run(Path projectDir, List<String> args) + } + + @CompileStatic + static final class WrapperGradleRunner implements GradleRunner { + @Override + void run(Path projectDir, List<String> args) { + Path javaHome = Path.of(System.getProperty('java.home')) + List<String> command = commandLine(javaHome, projectDir, args) + ProcessBuilder processBuilder = new ProcessBuilder(command) + processBuilder.directory(projectDir.toFile()) + processBuilder.inheritIO() + processBuilder.environment().put('JAVA_HOME', javaHome.toString()) + Process process = processBuilder.start() + int exit = process.waitFor() + if (exit != 0) { Review Comment: `waitFor()` has no timeout, and the nested build runs in the same checkout and GRADLE_USER_HOME as the still-executing outer build. A wedged nested build (cache-lock contention with the live outer daemon, or a hung test JVM) hangs `appIndyBench` forever locally — CI is only saved by `timeout-minutes: 180`. Suggest `waitFor(timeout, unit)` + `destroyForcibly()`, and consider a separate `--project-cache-dir` for the nested builds so they don't contend for the outer build's execution-history/file-hash locks (the JMH job sidesteps this entirely by measuring in separate worktrees). ########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/AppIndyBench.groovy: ########## @@ -0,0 +1,244 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.grails.benchmarks.report + +import groovy.transform.CompileStatic + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.StandardOpenOption + +@CompileStatic +class AppIndyBench { + + static final List<App> APPS = [ + new App('latency', ':grails-test-examples-latency:integrationTest', 'latencyapp.AppBenchFastPingSpec'), + new App('app1', ':grails-test-examples-app1:integrationTest', 'functionaltests.AppBenchInterceptorDemoSpec'), + new App('gsp-layout', ':grails-test-examples-gsp-layout:integrationTest', 'org.example.grails.layout.AppBenchDemoRenderTextSpec') + ].asImmutable() + + static void main(String[] args) { + int exit = run(args, new WrapperGradleRunner()) + if (exit != 0) { + System.exit(exit) + } + } + + static int run(String[] args, GradleRunner runner) { + return run(args, runner, new GitHubComments(), System.getenv()) + } + + static int run(String[] args, GradleRunner runner, CommentPoster poster, Map<String, String> environment) { + try { + Options options = parse(args) + Path noindyDir = recreateDirectory(options.outputDir.resolve('noindy')) + Path indyDir = recreateDirectory(options.outputDir.resolve('indy')) + + ['false', 'true'].each { String indy -> + Path modeDir = indy == 'true' ? indyDir : noindyDir + APPS.each { App app -> + Path out = modeDir.resolve(app.name + '.json') + runner.run(options.projectDir, gradleArgs(options, app, indy, out)) + if (!Files.isRegularFile(out)) { + throw new IllegalStateException("Missing result file: ${out}") + } Review Comment: When the gated spec is silently skipped, the nested build exits 0 and this bare 'Missing result file' points nowhere near the cause. That happens with any inherited `skipTests`/`skipFunctionalTests`/`onlyCoreTests` property — the functional-test-config `onlyIf` is `hasProperty`-based, so even `-PskipTests=false` or a value in `~/.gradle/gradle.properties` disables the Test task — or if the `-PappBench=true` → `app.bench` sysprop chain ever breaks the `@IgnoreIf` gate. Suggest naming these likely causes in the exception message, or detecting that zero tests ran. ########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/AppIndyBench.groovy: ########## @@ -0,0 +1,244 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.grails.benchmarks.report + +import groovy.transform.CompileStatic + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.StandardOpenOption + +@CompileStatic +class AppIndyBench { + + static final List<App> APPS = [ + new App('latency', ':grails-test-examples-latency:integrationTest', 'latencyapp.AppBenchFastPingSpec'), Review 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. ########## grails-benchmarks/src/reportTest/groovy/org/apache/grails/benchmarks/report/AppIndyBenchSpec.groovy: ########## @@ -0,0 +1,225 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.grails.benchmarks.report + +import spock.lang.Specification +import spock.lang.TempDir + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path + +class AppIndyBenchSpec extends Specification { + + @TempDir + Path temporaryDirectory + + void 'parse requires project-dir and applies defaults'() { + when: + AppIndyBench.Options options = AppIndyBench.parse(['--project-dir', temporaryDirectory.toString()] as String[]) + + then: + options.projectDir == temporaryDirectory.toAbsolutePath().normalize() + options.outputDir == options.projectDir.resolve('build').resolve('app-bench') + options.warmup == 200 + options.samples == 1000 + options.forks == 2 + options.maxWorkers == 4 + } + + void 'parse rejects a missing project-dir'() { + when: + AppIndyBench.parse(['--warmup', '10'] as String[]) + + then: + IllegalArgumentException error = thrown() + error.message.contains('--project-dir is required') + } + + void 'run invokes six nested builds then compares directory results'() { + given: + Path outputDir = temporaryDirectory.resolve('out') + Path summary = temporaryDirectory.resolve('summary.md') + List<List<String>> invocations = [] + AppIndyBench.GradleRunner runner = { Path projectDir, List<String> args -> + invocations.add(args) + writeDummyResult(args) + } as AppIndyBench.GradleRunner + + when: + int exit = AppIndyBench.run( + [ + '--project-dir', temporaryDirectory.toString(), + '--output-dir', outputDir.toString(), + '--warmup', '80', + '--samples', '300', + '--forks', '2', + '--max-workers', '3' + ] as String[], + runner, + new GitHubComments(), + [GITHUB_STEP_SUMMARY: summary.toString()] + ) + + then: + exit == 0 + invocations.size() == 6 + invocations[0].contains(':grails-test-examples-latency:integrationTest') + invocations[0].contains('latencyapp.AppBenchFastPingSpec') + invocations[0].contains('-PgrailsIndy=false') + invocations[0].contains('-PappBench=true') Review Comment: The suite never correlates each invocation's `-PgrailsIndy` value with the mode directory in its `-PappBenchOut` path, and both modes get identical dummy scores — so an inverted `modeDir` ternary (or swapped `--base`/`--head`) would pass every test while flipping the sign of every production verdict. Suggest asserting that invocations carrying `-PgrailsIndy=false` write under `/noindy/` (and `true` under `/indy/`), or giving the two modes distinguishable scores and asserting the verdict direction. ########## grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/bench/AppHttpBench.groovy: ########## @@ -0,0 +1,223 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.testing.http.client.bench + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.Paths + +import groovy.json.JsonOutput +import groovy.transform.CompileStatic + +/** + * App-level HTTP microbench helper that emits JMH-compatible JSON so results can be compared + * with {@code :grails-benchmarks:jmhCompare} using the same methodology as the framework JMH suite. + * + * <p>Measurement model (deliberately simple and reproducible on one machine): + * <ul> + * <li>warm up the full Spring Boot stack with {@code warmup} requests (discarded)</li> + * <li>collect {@code samples} timed requests as one raw series</li> + * <li>split the series into {@code forks} equal chunks to mimic JMH multi-fork rawData shape</li> + * <li>report mean ns/op with a simple standard-error-based scoreError</li> + * </ul> + * + * <p>Enable gated specs with {@code -PappBench=true}. Optional properties: + * {@code appBenchWarmup}, {@code appBenchSamples}, {@code appBenchForks}, {@code appBenchOut}. + */ +@CompileStatic +final class AppHttpBench { + + private AppHttpBench() { + } + + static boolean enabled() { + Boolean.getBoolean('app.bench') || Boolean.parseBoolean(System.getProperty('appBench', 'false')) + } + + static int warmupCount() { + Integer.getInteger('app.bench.warmup', Integer.getInteger('appBenchWarmup', 200)) + } + + static int sampleCount() { + Integer.getInteger('app.bench.samples', Integer.getInteger('appBenchSamples', 1000)) + } + + static int forkCount() { + Integer.getInteger('app.bench.forks', Integer.getInteger('appBenchForks', 2)) + } + + static Path outputPath(String defaultFileName) { + String configured = System.getProperty('app.bench.out', System.getProperty('appBenchOut', '')) + if (configured) { + return Paths.get(configured) + } + Path dir = Paths.get('build', 'app-bench') + Files.createDirectories(dir) + return dir.resolve(defaultFileName) + } + + /** + * Time a single request body. The closure must perform the HTTP call and assert success. + * + * @return elapsed nanoseconds + */ + static long timeNanos(Closure<?> request) { + long start = System.nanoTime() + request.call() + return System.nanoTime() - start + } + + /** + * Warm up, sample, and append one JMH-shaped benchmark entry to {@code out}. + * + * @param benchmark fully-qualified-style name, e.g. {@code appbench.latency.FastPing.httpGet} + * @param request closure that performs one successful request + */ + static void measureAndWrite(String benchmark, Path out, Closure<?> request) { + int warmup = Math.max(0, warmupCount()) + int samples = sampleCount() + if (samples < 1) { + throw new IllegalArgumentException("app.bench.samples must be >= 1, was ${samples}") + } + + for (int i = 0; i < warmup; i++) { + request.call() + } + + double[] values = new double[samples] + for (int i = 0; i < samples; i++) { + values[i] = (double) timeNanos(request) + } + + Map<String, Object> entry = toJmhEntry(benchmark, values, forkCount()) + appendEntry(out, entry) + } + + static Map<String, Object> toJmhEntry(String benchmark, double[] values, int forks) { + if (values == null || values.length == 0) { + throw new IllegalArgumentException('values must contain at least one sample') + } + double mean = mean(values) + double stdev = stdev(values, mean) + double scoreError = stdev * 1.96d / Math.sqrt((double) values.length) + Review Comment: `stdev * 1.96 / sqrt(n)` is a 95% standard-error band over N=300–1000 sequential request timings from one JVM — samples that are strongly autocorrelated (GC pauses, JIT phases, connection reuse). That is far tighter than the JMH iteration-mean confidence intervals `BenchmarkComparator` was designed around: with scoreError around 0.1% of the mean, the CI-disjointness test in the comparator is effectively always satisfied, so any ≥10% drift (realistic given the fixed noindy→indy run order) produces a confident REGRESSED/IMPROVED verdict. The app path also emits no ruler benchmarks (`RULER_PACKAGE` never matches `appbench.*`), so the report prints 'Runner health: not measured' and none of the instability gates can downgrade a verdict. Consider computing the error across batch/fork means (t-interval) the way JMH does, and emitting a ruler-equivalent so the health gates apply. ########## grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/bench/AppHttpBench.groovy: ########## @@ -0,0 +1,223 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.testing.http.client.bench + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.Paths + +import groovy.json.JsonOutput +import groovy.transform.CompileStatic + +/** + * App-level HTTP microbench helper that emits JMH-compatible JSON so results can be compared + * with {@code :grails-benchmarks:jmhCompare} using the same methodology as the framework JMH suite. + * + * <p>Measurement model (deliberately simple and reproducible on one machine): + * <ul> + * <li>warm up the full Spring Boot stack with {@code warmup} requests (discarded)</li> + * <li>collect {@code samples} timed requests as one raw series</li> + * <li>split the series into {@code forks} equal chunks to mimic JMH multi-fork rawData shape</li> + * <li>report mean ns/op with a simple standard-error-based scoreError</li> + * </ul> + * + * <p>Enable gated specs with {@code -PappBench=true}. Optional properties: + * {@code appBenchWarmup}, {@code appBenchSamples}, {@code appBenchForks}, {@code appBenchOut}. + */ +@CompileStatic +final class AppHttpBench { + Review Comment: This module is published, so `AppHttpBench` — including the `app.bench.*` system-property contract and the JMH-shaped JSON emitter — ships as public API to every consumer of grails-testing-support-http-client; renaming properties or fixing the statistics later becomes a breaking change. grails-benchmarks deliberately never publishes its bench tooling, and grails-geb uses testFixtures for shared test machinery. Suggest moving this to a testFixtures source set or the unpublished benchmarks module — or, if it's intentionally public, it needs grails-doc coverage. ########## gradle/app-bench-config.gradle: ########## @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Experimental app-level indy A/B harness wiring. +// Enable with -PappBench=true (and usually --tests '*AppBench*'). +// Optional: -PappBenchWarmup=200 -PappBenchSamples=1000 -PappBenchForks=2 -PappBenchOut=<json> + +def appBenchEnabled = providers.gradleProperty('appBench').map { Boolean.parseBoolean(it) }.orElse(false) +def appBenchWarmup = providers.gradleProperty('appBenchWarmup').orElse('200') +def appBenchSamples = providers.gradleProperty('appBenchSamples').orElse('1000') +def appBenchForks = providers.gradleProperty('appBenchForks').orElse('2') +def appBenchOut = providers.gradleProperty('appBenchOut').orElse('') + +tasks.withType(Test).configureEach { Test test -> + test.systemProperty('app.bench', String.valueOf(appBenchEnabled.get())) + test.systemProperty('app.bench.warmup', appBenchWarmup.get()) + test.systemProperty('app.bench.samples', appBenchSamples.get()) + test.systemProperty('app.bench.forks', appBenchForks.get()) + if (appBenchOut.get()) { + test.systemProperty('app.bench.out', appBenchOut.get()) + test.outputs.file(appBenchOut.get()) + } Review Comment: This registers the single `-PappBenchOut` file as a declared output of every `Test` task (unit `test` and `integrationTest`) in all three projects. With `org.gradle.parallel=true` (gradle.properties), the header's documented manual invocation from the root runs the three apps' tasks concurrently: `appendEntry`'s unlocked read-parse-rewrite drops entries (last writer wins) and Gradle flags multiple tasks sharing one declared output (overlapping-outputs warnings, disabled caching). The `systemProperty` wiring above also runs unconditionally, so `app.bench.*` values become inputs of ordinary test runs and any `-PappBench*` change invalidates unrelated tests' up-to-date state. Suggest guarding the whole block behind `appBenchEnabled` and wiring only `integrationTest`. ########## .github/workflows/benchmark.yml: ########## @@ -264,6 +264,40 @@ jobs: git worktree remove --force "$WORKTREE_ROOT/base" || true git worktree remove --force "$WORKTREE_ROOT/head" || true + app-bench: + name: "App indy benchmarks" + if: >- + contains(github.event.pull_request.labels.*.name, 'performance') && + (github.event.action != 'labeled' || github.event.label.name == 'performance') + runs-on: ubuntu-24.04 + env: + RESULT_DIR: ${{ github.workspace }}/app-bench-results + steps: + - name: "Checkout repository" + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: "Setup JDK" + uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5.2.0 + with: + distribution: liberica + java-version: 21 + - name: "Setup Gradle" + uses: gradle/actions/setup-gradle@50e97c2cd7a37755bbfafc9c5b7cafaece252f6e # v6.1.0 + with: + cache-provider: basic # 'basic' uses the MIT-licensed, open-source cache provider; the default 'enhanced' provider (v6+) is proprietary (Gradle commercial Terms of Use) + develocity-access-key: ${{ secrets.DEVELOCITY_ACCESS_KEY }} + - name: "Verify JMH comparison tool" + run: ./gradlew :grails-benchmarks:test --max-workers=4 + - name: "Run app-level indy A/B benches" + timeout-minutes: 180 + run: ./gradlew --no-daemon :grails-benchmarks:appIndyBench -PappBenchWarmup=80 -PappBenchSamples=300 -PappBenchForks=2 -PappBenchOutDir="$RESULT_DIR" --max-workers=4 Review Comment: The workflow header says results are advisory and a detected regression never fails the workflow, and the JMH compare step runs with `if: always()` + `continue-on-error: true` plus a fallback summary. Here, any one of the six nested builds failing makes `AppIndyBench` exit 2 before any comparison or summary is produced — a flaky nested integrationTest turns the job red with zero diagnostics in the step summary (only the artifact upload is `if: always()`). Consider `continue-on-error` on this step, and/or having AppIndyBench compare whatever results exist and emit a fallback summary. ########## .github/workflows/benchmark.yml: ########## @@ -264,6 +264,40 @@ jobs: git worktree remove --force "$WORKTREE_ROOT/base" || true git worktree remove --force "$WORKTREE_ROOT/head" || true + app-bench: + name: "App indy benchmarks" + if: >- + contains(github.event.pull_request.labels.*.name, 'performance') && + (github.event.action != 'labeled' || github.event.label.name == 'performance') + runs-on: ubuntu-24.04 + env: + RESULT_DIR: ${{ github.workspace }}/app-bench-results + steps: + - name: "Checkout repository" + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: "Setup JDK" + uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5.2.0 + with: + distribution: liberica + java-version: 21 + - name: "Setup Gradle" + uses: gradle/actions/setup-gradle@50e97c2cd7a37755bbfafc9c5b7cafaece252f6e # v6.1.0 + with: + cache-provider: basic # 'basic' uses the MIT-licensed, open-source cache provider; the default 'enhanced' provider (v6+) is proprietary (Gradle commercial Terms of Use) + develocity-access-key: ${{ secrets.DEVELOCITY_ACCESS_KEY }} + - name: "Verify JMH comparison tool" + run: ./gradlew :grails-benchmarks:test --max-workers=4 Review Comment: This runs without `--no-daemon`, so a persistent daemon (`org.gradle.jvmargs=-Xmx5G`, 3h idle timeout) stays resident on the runner throughout the measurement step. The `benchmark` job has an explicit 'Stop Gradle daemons before measurement' step for exactly this reason; during sampling this runner additionally hosts the outer single-use daemon, the nested build's daemon, and the forked Spring Boot test JVM — memory pressure and daemon GC noise skew the latency deltas the job exists to measure. Suggest adding the same daemon-stop step before the bench run. ########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/AppIndyBench.groovy: ########## @@ -0,0 +1,244 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.grails.benchmarks.report + +import groovy.transform.CompileStatic + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.StandardOpenOption + +@CompileStatic +class AppIndyBench { + + static final List<App> APPS = [ + new App('latency', ':grails-test-examples-latency:integrationTest', 'latencyapp.AppBenchFastPingSpec'), + new App('app1', ':grails-test-examples-app1:integrationTest', 'functionaltests.AppBenchInterceptorDemoSpec'), + new App('gsp-layout', ':grails-test-examples-gsp-layout:integrationTest', 'org.example.grails.layout.AppBenchDemoRenderTextSpec') + ].asImmutable() + + static void main(String[] args) { + int exit = run(args, new WrapperGradleRunner()) + if (exit != 0) { + System.exit(exit) + } + } + + static int run(String[] args, GradleRunner runner) { + return run(args, runner, new GitHubComments(), System.getenv()) + } + + static int run(String[] args, GradleRunner runner, CommentPoster poster, Map<String, String> environment) { + try { + Options options = parse(args) + Path noindyDir = recreateDirectory(options.outputDir.resolve('noindy')) + Path indyDir = recreateDirectory(options.outputDir.resolve('indy')) + + ['false', 'true'].each { String indy -> + Path modeDir = indy == 'true' ? indyDir : noindyDir + APPS.each { App app -> + Path out = modeDir.resolve(app.name + '.json') + runner.run(options.projectDir, gradleArgs(options, app, indy, out)) + if (!Files.isRegularFile(out)) { + throw new IllegalStateException("Missing result file: ${out}") + } + } + } + + Path report = options.outputDir.resolve('indy-vs-noindy.md') + int compareExit = JmhCompare.run( + ['--base', noindyDir.toString(), '--head', indyDir.toString(), '--output', report.toString()] as String[], + poster, + environment + ) + if (compareExit != 0) { + return compareExit + } + appendStepSummary(report, environment) + return 0 + } catch (Exception error) { + error.printStackTrace(System.err) + return 2 + } + } + + static List<String> gradleArgs(Options options, App app, String indy, Path out) { + return [ + '--no-daemon', + "--max-workers=${options.maxWorkers}".toString(), + app.task, + '--tests', + app.tests, + "-PgrailsIndy=${indy}".toString(), + '-PappBench=true', + "-PappBenchWarmup=${options.warmup}".toString(), + "-PappBenchSamples=${options.samples}".toString(), + "-PappBenchForks=${options.forks}".toString(), + "-PappBenchOut=${out.toAbsolutePath()}".toString() + ] + } + + static Options parse(String[] args) { + Set<String> values = ['project-dir', 'output-dir', 'warmup', 'samples', 'forks', 'max-workers'] as Set<String> + Map<String, String> options = new LinkedHashMap<>() + for (int index = 0; index < args.length; index++) { + String option = args[index] + if (!option.startsWith('--')) { + throw new IllegalArgumentException("unknown option: ${option}") + } + String key = option.substring(2) + if (!values.contains(key)) { + throw new IllegalArgumentException("unknown option: --${key}") + } + if (index + 1 >= args.length) { + throw new IllegalArgumentException("missing value for --${key}") + } + options.put(key, args[++index]) + } + String projectDirValue = options.get('project-dir') + if (!projectDirValue) { + throw new IllegalArgumentException('--project-dir is required') + } + Path projectDir = Path.of(projectDirValue).toAbsolutePath().normalize() + Path outputDir = options.containsKey('output-dir') + ? Path.of(options.get('output-dir')).toAbsolutePath().normalize() + : projectDir.resolve('build').resolve('app-bench') + return new Options( + projectDir, + outputDir, + parsePositiveInt(options.getOrDefault('warmup', '200'), 'warmup', true), + parsePositiveInt(options.getOrDefault('samples', '1000'), 'samples', false), + parsePositiveInt(options.getOrDefault('forks', '2'), 'forks', false), + parsePositiveInt(options.getOrDefault('max-workers', '4'), 'max-workers', false) + ) + } + + private static int parsePositiveInt(String raw, String name, boolean allowZero) { + int value + try { + value = Integer.parseInt(raw) + } catch (NumberFormatException ignored) { + throw new IllegalArgumentException("--${name} must be an integer") + } + if (value < 0 || (!allowZero && value < 1)) { + throw new IllegalArgumentException("--${name} must be ${allowZero ? '>= 0' : '>= 1'}") + } + return value + } + + private static void appendStepSummary(Path report, Map<String, String> environment) { + String summary = environment.get('GITHUB_STEP_SUMMARY') + if (!summary || !Files.isRegularFile(report)) { + return + } + Files.writeString( + Path.of(summary), + Files.readString(report, StandardCharsets.UTF_8), + StandardCharsets.UTF_8, + StandardOpenOption.CREATE, + StandardOpenOption.APPEND + ) + } + + static Path recreateDirectory(Path directory) { + if (Files.exists(directory)) { Review Comment: This recursively deletes `<outputDir>/noindy` and `<outputDir>/indy` for whatever directory the user supplies via `--output-dir`/`-PappBenchOutDir`, with no containment or sanity check — pointing it at a directory that happens to contain unrelated `noindy`/`indy` subtrees destroys them before benching starts. Meanwhile a stale `indy-vs-noindy.md` in outputDir itself is never cleaned, so a rerun that fails before JmhCompare leaves yesterday's report sitting next to today's JSON. (Also, GDK `deleteDir()` already implements the walk + reverse-sort deletion — see PublishGuideTask in build-logic.) ########## grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/bench/AppHttpBench.groovy: ########## @@ -0,0 +1,223 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.testing.http.client.bench + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.Paths + +import groovy.json.JsonOutput +import groovy.transform.CompileStatic + +/** + * App-level HTTP microbench helper that emits JMH-compatible JSON so results can be compared + * with {@code :grails-benchmarks:jmhCompare} using the same methodology as the framework JMH suite. + * + * <p>Measurement model (deliberately simple and reproducible on one machine): + * <ul> + * <li>warm up the full Spring Boot stack with {@code warmup} requests (discarded)</li> + * <li>collect {@code samples} timed requests as one raw series</li> + * <li>split the series into {@code forks} equal chunks to mimic JMH multi-fork rawData shape</li> + * <li>report mean ns/op with a simple standard-error-based scoreError</li> + * </ul> + * + * <p>Enable gated specs with {@code -PappBench=true}. Optional properties: + * {@code appBenchWarmup}, {@code appBenchSamples}, {@code appBenchForks}, {@code appBenchOut}. + */ +@CompileStatic +final class AppHttpBench { + + private AppHttpBench() { + } + + static boolean enabled() { + Boolean.getBoolean('app.bench') || Boolean.parseBoolean(System.getProperty('appBench', 'false')) + } + + static int warmupCount() { + Integer.getInteger('app.bench.warmup', Integer.getInteger('appBenchWarmup', 200)) + } Review Comment: `Integer.getInteger` silently returns the default on an unparsable value and decodes leading-zero values as octal, and app-bench-config forwards the raw `-PappBench*` strings without validation. So `-PappBenchSamples=1O00` (letter O) silently benches with the default 1000, and `-PappBenchSamples=010` benches with 8 samples — no warning either way. The orchestrator path validates with `parseInt`; the documented direct-Gradle path should too (either in these getters or in app-bench-config.gradle). ########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/AppIndyBench.groovy: ########## @@ -0,0 +1,244 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.grails.benchmarks.report + +import groovy.transform.CompileStatic + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.StandardOpenOption + +@CompileStatic +class AppIndyBench { + + static final List<App> APPS = [ + new App('latency', ':grails-test-examples-latency:integrationTest', 'latencyapp.AppBenchFastPingSpec'), + new App('app1', ':grails-test-examples-app1:integrationTest', 'functionaltests.AppBenchInterceptorDemoSpec'), + new App('gsp-layout', ':grails-test-examples-gsp-layout:integrationTest', 'org.example.grails.layout.AppBenchDemoRenderTextSpec') + ].asImmutable() + + static void main(String[] args) { + int exit = run(args, new WrapperGradleRunner()) + if (exit != 0) { + System.exit(exit) + } + } + + static int run(String[] args, GradleRunner runner) { + return run(args, runner, new GitHubComments(), System.getenv()) + } + + static int run(String[] args, GradleRunner runner, CommentPoster poster, Map<String, String> environment) { + try { + Options options = parse(args) + Path noindyDir = recreateDirectory(options.outputDir.resolve('noindy')) + Path indyDir = recreateDirectory(options.outputDir.resolve('indy')) + + ['false', 'true'].each { String indy -> + Path modeDir = indy == 'true' ? indyDir : noindyDir + APPS.each { App app -> + Path out = modeDir.resolve(app.name + '.json') + runner.run(options.projectDir, gradleArgs(options, app, indy, out)) + if (!Files.isRegularFile(out)) { Review 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. ########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/AppIndyBench.groovy: ########## @@ -0,0 +1,244 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.grails.benchmarks.report + +import groovy.transform.CompileStatic + +import java.nio.charset.StandardCharsets +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.StandardOpenOption + +@CompileStatic +class AppIndyBench { + + static final List<App> APPS = [ + new App('latency', ':grails-test-examples-latency:integrationTest', 'latencyapp.AppBenchFastPingSpec'), + new App('app1', ':grails-test-examples-app1:integrationTest', 'functionaltests.AppBenchInterceptorDemoSpec'), + new App('gsp-layout', ':grails-test-examples-gsp-layout:integrationTest', 'org.example.grails.layout.AppBenchDemoRenderTextSpec') + ].asImmutable() + + static void main(String[] args) { + int exit = run(args, new WrapperGradleRunner()) + if (exit != 0) { + System.exit(exit) + } + } + + static int run(String[] args, GradleRunner runner) { + return run(args, runner, new GitHubComments(), System.getenv()) + } + + static int run(String[] args, GradleRunner runner, CommentPoster poster, Map<String, String> environment) { + try { + Options options = parse(args) + Path noindyDir = recreateDirectory(options.outputDir.resolve('noindy')) + Path indyDir = recreateDirectory(options.outputDir.resolve('indy')) + + ['false', 'true'].each { String indy -> + Path modeDir = indy == 'true' ? indyDir : noindyDir + APPS.each { App app -> + Path out = modeDir.resolve(app.name + '.json') + runner.run(options.projectDir, gradleArgs(options, app, indy, out)) + if (!Files.isRegularFile(out)) { + throw new IllegalStateException("Missing result file: ${out}") + } + } + } + + Path report = options.outputDir.resolve('indy-vs-noindy.md') + int compareExit = JmhCompare.run( + ['--base', noindyDir.toString(), '--head', indyDir.toString(), '--output', report.toString()] as String[], + poster, + environment + ) + if (compareExit != 0) { + return compareExit + } + appendStepSummary(report, environment) + return 0 + } catch (Exception error) { + error.printStackTrace(System.err) + return 2 + } + } + + static List<String> gradleArgs(Options options, App app, String indy, Path out) { + return [ + '--no-daemon', + "--max-workers=${options.maxWorkers}".toString(), + app.task, + '--tests', + app.tests, + "-PgrailsIndy=${indy}".toString(), + '-PappBench=true', + "-PappBenchWarmup=${options.warmup}".toString(), + "-PappBenchSamples=${options.samples}".toString(), + "-PappBenchForks=${options.forks}".toString(), + "-PappBenchOut=${out.toAbsolutePath()}".toString() + ] + } + + static Options parse(String[] args) { + Set<String> values = ['project-dir', 'output-dir', 'warmup', 'samples', 'forks', 'max-workers'] as Set<String> + Map<String, String> options = new LinkedHashMap<>() + for (int index = 0; index < args.length; index++) { + String option = args[index] + if (!option.startsWith('--')) { + throw new IllegalArgumentException("unknown option: ${option}") + } + String key = option.substring(2) + if (!values.contains(key)) { + throw new IllegalArgumentException("unknown option: --${key}") + } + if (index + 1 >= args.length) { + throw new IllegalArgumentException("missing value for --${key}") + } + options.put(key, args[++index]) + } + String projectDirValue = options.get('project-dir') + if (!projectDirValue) { + throw new IllegalArgumentException('--project-dir is required') + } + Path projectDir = Path.of(projectDirValue).toAbsolutePath().normalize() + Path outputDir = options.containsKey('output-dir') + ? Path.of(options.get('output-dir')).toAbsolutePath().normalize() + : projectDir.resolve('build').resolve('app-bench') + return new Options( + projectDir, + outputDir, + parsePositiveInt(options.getOrDefault('warmup', '200'), 'warmup', true), + parsePositiveInt(options.getOrDefault('samples', '1000'), 'samples', false), + parsePositiveInt(options.getOrDefault('forks', '2'), 'forks', false), + parsePositiveInt(options.getOrDefault('max-workers', '4'), 'max-workers', false) + ) + } + + private static int parsePositiveInt(String raw, String name, boolean allowZero) { + int value + try { + value = Integer.parseInt(raw) + } catch (NumberFormatException ignored) { + throw new IllegalArgumentException("--${name} must be an integer") + } + if (value < 0 || (!allowZero && value < 1)) { + throw new IllegalArgumentException("--${name} must be ${allowZero ? '>= 0' : '>= 1'}") + } + return value + } + + private static void appendStepSummary(Path report, Map<String, String> environment) { + String summary = environment.get('GITHUB_STEP_SUMMARY') + if (!summary || !Files.isRegularFile(report)) { + return + } + Files.writeString( + Path.of(summary), + Files.readString(report, StandardCharsets.UTF_8), + StandardCharsets.UTF_8, + StandardOpenOption.CREATE, + StandardOpenOption.APPEND + ) + } + + static Path recreateDirectory(Path directory) { + if (Files.exists(directory)) { + Files.walk(directory).withCloseable { stream -> + stream.sorted(Comparator.reverseOrder()).forEach { Path path -> Files.deleteIfExists(path) } + } + } + return Files.createDirectories(directory) + } + + @CompileStatic + static final class App { + final String name + final String task + final String tests + + App(String name, String task, String tests) { + this.name = name + this.task = task + this.tests = tests + } + } + + @CompileStatic + static final class Options { + final Path projectDir + final Path outputDir + final int warmup + final int samples + final int forks + final int maxWorkers + + Options(Path projectDir, Path outputDir, int warmup, int samples, int forks, int maxWorkers) { + this.projectDir = projectDir + this.outputDir = outputDir + this.warmup = warmup + this.samples = samples + this.forks = forks + this.maxWorkers = maxWorkers + } + } + + @CompileStatic + interface GradleRunner { + void run(Path projectDir, List<String> args) + } + + @CompileStatic + static final class WrapperGradleRunner implements GradleRunner { + @Override + void run(Path projectDir, List<String> args) { + Path javaHome = Path.of(System.getProperty('java.home')) + List<String> command = commandLine(javaHome, projectDir, args) + ProcessBuilder processBuilder = new ProcessBuilder(command) + processBuilder.directory(projectDir.toFile()) + processBuilder.inheritIO() + processBuilder.environment().put('JAVA_HOME', javaHome.toString()) + Process process = processBuilder.start() + int exit = process.waitFor() + if (exit != 0) { + throw new IllegalStateException("Nested Gradle exited ${exit}: ${command}") + } + } + + static List<String> commandLine(Path javaHome, Path projectDir, List<String> args) { + Path java = javaExecutable(javaHome) + Path wrapperJar = projectDir.resolve('gradle').resolve('wrapper').resolve('gradle-wrapper.jar') + if (!Files.isRegularFile(java)) { + throw new IllegalStateException("Java executable not found: ${java}") + } + if (!Files.isRegularFile(wrapperJar)) { + throw new IllegalStateException("Gradle wrapper jar not found: ${wrapperJar}") + } + List<String> command = new ArrayList<>() + command.add(java.toString()) + command.add('-cp') + command.add(wrapperJar.toString()) + command.add('org.gradle.wrapper.GradleWrapperMain') + command.addAll(args) Review Comment: Invoking `GradleWrapperMain` directly (rather than the gradlew script) means `DEFAULT_JVM_OPTS`/`JAVA_OPTS`/`GRADLE_OPTS` are never spliced into the launcher JVM — the gradlew script is what does that — so environment-based configuration is silently ignored for all six nested builds. Our own OOM guidance is `export GRADLE_OPTS="-Xms2G -Xmx5G"`, and proxy settings are commonly passed the same way; nested builds will behave differently from the documented `./gradlew` invocation with no hint the env var was dropped (build-JVM heap currently survives only because gradle.properties pins `org.gradle.jvmargs`). If avoiding the shell scripts is the goal, consider forwarding those env vars onto the wrapper JVM explicitly. -- 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]
