Copilot commented on code in PR #16169: URL: https://github.com/apache/grails-core/pull/16169#discussion_r3808159188
########## grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/bench/AppHttpBench.groovy: ########## @@ -0,0 +1,221 @@ +/* + * 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 = warmupCount() + int samples = sampleCount() + int forks = Math.max(1, forkCount()) + Review Comment: `measureAndWrite` allows `app.bench.samples`/`appBenchSamples` to be set to 0 (or negative), which will produce an empty `values` array and then trigger divide-by-zero / index errors later in `toJmhEntry`. Consider validating samples >= 1 and clamping forks to samples early so the bench fails fast with a clear message. ########## 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').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', 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()) + } + if (appBenchEnabled.get() == 'true') { + test.outputs.upToDateWhen { false } + test.outputs.cacheIf { false } + } +} Review Comment: `appBenchEnabled` is treated case-insensitively by `Boolean.getBoolean('app.bench')`, but the Gradle wiring disables caching only when the property is exactly `'true'`. If someone passes `-PappBench=TRUE`, benches will run but outputs may remain cached/up-to-date. Parse the property as a boolean once and use that consistently. ########## grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/bench/AppHttpBench.groovy: ########## @@ -0,0 +1,221 @@ +/* + * 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 = warmupCount() + int samples = sampleCount() + int forks = Math.max(1, forkCount()) + + 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, forks) + appendEntry(out, entry) + } + + static Map<String, Object> toJmhEntry(String benchmark, double[] values, int forks) { + double mean = mean(values) + double stdev = stdev(values, mean) + double scoreError = stdev * 1.96d / Math.sqrt((double) values.length) + + int forkCount = Math.max(1, forks) + 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]) + } + if (chunk.isEmpty() && !rawData.isEmpty()) { + chunk.addAll(rawData.get(rawData.size() - 1)) + } + rawData.add(chunk) + offset = end + } + Review Comment: `toJmhEntry` assumes `values` is non-empty (e.g., `sorted[0]`) and also duplicates the last fork’s rawData when `forks > values.length`, which can skew the JMH-shaped output. It’s safer to reject empty input and clamp forkCount to `values.length` so rawData never needs to be synthesized. ########## grails-benchmarks/scripts/run-app-indy-bench.ps1: ########## @@ -0,0 +1,118 @@ +# 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 vs non-indy HTTP benches against grails-test-examples. +# Run from repo root. Rebuilds framework+app with -PgrailsIndy, runs gated AppBench* specs, +# then compares JMH-shaped JSON with :grails-benchmarks:jmhCompare. +# +# Usage: +# pwsh grails-benchmarks/scripts/run-app-indy-bench.ps1 +# pwsh grails-benchmarks/scripts/run-app-indy-bench.ps1 -Samples 500 -Warmup 100 + +param( + [int]$Warmup = 200, + [int]$Samples = 1000, + [int]$Forks = 2 +) + +$ErrorActionPreference = 'Stop' +$root = (Resolve-Path (Join-Path $PSScriptRoot '..\..')).Path +Set-Location -LiteralPath $root + +$outDir = Join-Path $root 'build\app-bench' +New-Item -ItemType Directory -Force -Path $outDir | Out-Null + +$apps = @( + @{ + Name = 'latency' + Project = ':grails-test-examples-latency' + Tests = 'latencyapp.AppBenchFastPingSpec' + OutName = 'latency' + }, + @{ + Name = 'app1' + Project = ':grails-test-examples-app1' + Tests = 'functionaltests.AppBenchInterceptorDemoSpec' + OutName = 'app1' + }, + @{ + Name = 'gsp-layout' + Project = ':grails-test-examples-gsp-layout' + Tests = 'org.example.grails.layout.AppBenchDemoRenderTextSpec' + OutName = 'gsp-layout' + } +) + +function Invoke-AppBenchMode { + param( + [string]$Indy, + [hashtable]$App + ) + $label = if ($Indy -eq 'true') { 'indy' } else { 'noindy' } + $outFile = Join-Path $outDir "$($App.OutName)-$label.json" + if (Test-Path -LiteralPath $outFile) { + Remove-Item -LiteralPath $outFile -Force + } + + Write-Host "=== $($App.Name) grailsIndy=$Indy -> $outFile ===" + & "$root\gradlew.bat" 'clean' "$($App.Project):integrationTest" ` Review Comment: This script is invoked via `pwsh` (cross-platform), but it calls `gradlew.bat`, which will fail on non-Windows systems. Use `gradlew` on Unix-like systems. This issue also appears in the following locations of the same file: - line 96 - line 115 -- 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]
