Copilot commented on code in PR #16071: URL: https://github.com/apache/grails-core/pull/16071#discussion_r3685314754
########## grails-benchmarks/build.gradle: ########## @@ -0,0 +1,238 @@ +/* + * 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. + */ + +import org.gradle.api.file.DuplicatesStrategy +import org.gradle.api.tasks.PathSensitivity + +import java.util.Properties +import java.util.zip.ZipFile + +plugins { + id 'groovy' + id 'java-library' + id 'me.champeau.jmh' version '0.7.3' + id 'org.apache.grails.buildsrc.properties' + id 'org.apache.grails.buildsrc.compile' +} + +version = projectVersion +group = 'org.apache.grails' + +// This build-time-only benchmark harness is never published, so it deliberately omits +// org.apache.grails.buildsrc.publish, org.apache.grails.buildsrc.sbom, +// org.apache.grails.buildsrc.vulnerability-scan, org.apache.grails.gradle.grails-jacoco, +// org.apache.grails.buildsrc.dependency-validator, and org.apache.grails.gradle.grails-code-style. +// The latter remains omitted until existing JMH source import-order and fixture-formatting +// violations can be remediated together. + +// The Groovy fixtures under src/main/groovy build the framework objects whose APIs are +// closure-based (the URL mappings DSL, the Validateable trait, view templates). The JMH +// benchmarks themselves live in src/jmh/java and only *call* those fixtures, so that the +// measured code path is plain Java and does not include Groovy's dynamic dispatch. +// me.champeau.jmh puts the main source set's output on the jmh compile classpath, which is +// why the fixtures are in main rather than src/jmh/groovy: keeping the benchmarks in a pure +// Java source set preserves normal JMH annotation processing. +dependencies { + implementation platform(project(':grails-bom')) + + implementation project(':grails-web-url-mappings') + implementation project(':grails-databinding-core') + implementation project(':grails-gsp-core') + implementation project(':grails-interceptors') + implementation project(':grails-views-gson') + implementation project(':grails-views-markup') + implementation project(':grails-core') + implementation 'org.apache.groovy:groovy' + + // The framework modules declare these as compileOnly, so they are absent from the runtime + // classpath a benchmark actually executes on. The modules' own test suites add them back + // the same way. Without them, URL mapping, interceptor and view benchmarks fail at @Setup + // with NoClassDefFoundError: jakarta/servlet/ServletContext. + implementation 'jakarta.servlet:jakarta.servlet-api' + implementation 'org.springframework:spring-test' +} + +// The jmh source set compiles and runs against everything main depends on. +configurations.jmh.extendsFrom(configurations.implementation) + +// Every JMH knob the CI workflow needs to override is exposed as a Gradle property so that +// .github/workflows/benchmark.yml can run a fast PR profile and a slower scheduled profile +// from the same build. Defaults here are the quick local-development profile. +jmh { + jmhVersion = '1.37' + includes = providers.gradleProperty('jmh.include') + .map { it.split(',') as List<String> } + .orElse(['.*']) + .get() + fork = providers.gradleProperty('jmh.forks').map { it as Integer }.orElse(1).get() + warmupIterations = providers.gradleProperty('jmh.warmupIterations').map { it as Integer }.orElse(1).get() + iterations = providers.gradleProperty('jmh.iterations').map { it as Integer }.orElse(1).get() + warmup = providers.gradleProperty('jmh.warmupTime').orElse('1s').get() + timeOnIteration = providers.gradleProperty('jmh.iterationTime').orElse('1s').get() + resultFormat = providers.gradleProperty('jmh.resultFormat').orElse('JSON').get() + // A benchmark that throws during @Setup is otherwise dropped from the results file while + // the build still reports success, which would silently shrink the comparison set. + failOnError = providers.gradleProperty('jmh.failOnError').map { it.toBoolean() }.orElse(true).get() + resultsFile = file(providers.gradleProperty('jmh.resultFile') + .orElse(layout.buildDirectory.file('results/jmh/results.json').get().asFile.absolutePath) + .get()) + // Comma-separated JMH profilers, e.g. -Pjmh.profilers=gc for allocation reporting. + profilers = providers.gradleProperty('jmh.profilers') + .map { it.split(',') as List<String> } + .orElse([]) + .get() +} + +def mergeJmhClasspathMetadata = tasks.register('mergeJmhClasspathMetadata') { + def mergedMetadataDirectory = layout.buildDirectory.dir('generated/jmh-classpath-metadata') + def lineMetadataPrefixes = ['META-INF/services/', 'META-INF/groovy/'] + def extensionModuleSuffix = 'org.codehaus.groovy.runtime.ExtensionModule' + def propertiesMetadataPaths = [ + 'META-INF/spring.factories', + 'META-INF/spring.handlers', + 'META-INF/spring.schemas' + ] + + inputs.files(configurations.jmhRuntimeClasspath) + .withPropertyName('jmhRuntimeClasspath') + .withPathSensitivity(PathSensitivity.RELATIVE) + outputs.dir(mergedMetadataDirectory) + outputs.cacheIf { true } + + doLast { + File outputDirectory = mergedMetadataDirectory.get().asFile + project.delete(outputDirectory) + + Map<String, Set<String>> lineEntries = new TreeMap<>() + Map<String, Map<String, Set<String>>> extensionModuleEntries = new TreeMap<>() + Map<String, Map<String, Set<String>>> propertyEntries = new TreeMap<>() + + configurations.jmhRuntimeClasspath.files + .findAll { File artifact -> artifact.name.endsWith('.jar') } + .sort { File artifact -> artifact.absolutePath } + .each { File artifact -> + new ZipFile(artifact).withCloseable { ZipFile zipFile -> + zipFile.entries().each { entry -> + if (entry.directory) { + return + } + + String path = entry.name + if (path.endsWith(extensionModuleSuffix)) { + Properties properties = new Properties() + zipFile.getInputStream(entry).withCloseable { input -> + properties.load(input) + } + Map<String, Set<String>> entries = extensionModuleEntries.computeIfAbsent(path) { + new TreeMap<>() + } + ['extensionClasses', 'staticExtensionClasses'].each { String key -> + Set<String> values = entries.computeIfAbsent(key) { new TreeSet<>() } + properties.getProperty(key, '').split(',').each { String value -> + if (value) { + values.add(value.trim()) + } + } + } + } else if (lineMetadataPrefixes.any { String prefix -> path.startsWith(prefix) }) { + Set<String> lines = lineEntries.computeIfAbsent(path) { new TreeSet<>() } + zipFile.getInputStream(entry).getText('UTF-8').readLines().each { String line -> + if (line) { + lines.add(line) + } + } Review Comment: `mergeJmhClasspathMetadata` reads ZIP entry streams via `zipFile.getInputStream(entry).getText(...)` without closing the stream. Over many JARs/entries this can leak file handles and slow/flake the build on some systems. Wrap the `InputStream` in `withCloseable` (as is already done for the Properties reads) so each entry stream is reliably closed. ########## .github/workflows/benchmark.yml: ########## @@ -0,0 +1,404 @@ +# 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. + +name: "JMH Benchmark Comparison" + +# SECURITY: This workflow deliberately uses pull_request, never pull_request_target. +# Pull requests run untrusted code, and Apache Infra policy forbids exposing tokens +# to that code through a privileged pull_request_target workflow. +on: + pull_request: + types: [opened, synchronize, reopened, labeled] + paths-ignore: + - '**/*.md' + - '**/*.adoc' + - 'grails-doc/**' + workflow_dispatch: + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +permissions: + contents: read + +jobs: + # Each shard builds both revisions before measuring either one on the same runner. + # Building first avoids CPU, IO, thermal, cache, and frequency state biasing a measurement. + # Shard a measures BASE then HEAD, while shard b measures HEAD then BASE. + # Alternating the order cancels first-versus-second ordering bias without doubling runtime. + # Both shards use the PR merge commit as HEAD, so they measure what would actually land. + # Results are advisory: a detected regression never fails this workflow. + benchmark: + name: "Paired JMH benchmarks (${{ matrix.shard }})" + if: ${{ github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'performance') }} + runs-on: ubuntu-24.04 + strategy: + fail-fast: false + matrix: + shard: [a, b] + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.sha }} + JMH_INCLUDE: '.*' + PR_NUMBER: ${{ github.event.pull_request.number || 0 }} + REPOSITORY: ${{ github.repository }} + RESULT_DIR: ${{ github.workspace }}/jmh-results/${{ matrix.shard }} + REPORT_DIR: ${{ github.workspace }}/jmh-reports/${{ matrix.shard }} + SHARD: ${{ matrix.shard }} + steps: + - name: "π₯ Checkout repository" + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + fetch-depth: 0 + - 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: "π³ Prepare paired worktrees" + run: | + WORKTREE_ROOT="$RUNNER_TEMP/jmh-worktrees/$SHARD" + mkdir -p "$WORKTREE_ROOT" "$RESULT_DIR" "$REPORT_DIR" + if [ -n "$BASE_SHA" ] && git cat-file -e "$BASE_SHA^{commit}" 2>/dev/null; then + RESOLVED_BASE_SHA="$BASE_SHA" + elif RESOLVED_BASE_SHA="$(git merge-base "$HEAD_SHA^1" "$HEAD_SHA^2" 2>/dev/null)"; then + echo "Configured base commit is unreachable; using merge-base $RESOLVED_BASE_SHA." + elif RESOLVED_BASE_SHA="$(git rev-parse "$HEAD_SHA^" 2>/dev/null)"; then + echo "Configured base commit is unreachable; using HEAD parent $RESOLVED_BASE_SHA." + else + RESOLVED_BASE_SHA="" + echo "No base commit could be resolved; comparison will use HEAD-only mode." + fi + git worktree add --detach "$WORKTREE_ROOT/head" "$HEAD_SHA" + { + printf 'HEAD_DIR=%s\n' "$WORKTREE_ROOT/head" + printf 'WORKTREE_ROOT=%s\n' "$WORKTREE_ROOT" + } >> "$GITHUB_ENV" + if [ -n "$RESOLVED_BASE_SHA" ] && git worktree add --detach "$WORKTREE_ROOT/base" "$RESOLVED_BASE_SHA"; then + { + printf 'BASE_DIR=%s\n' "$WORKTREE_ROOT/base" + printf 'RESOLVED_BASE_SHA=%s\n' "$RESOLVED_BASE_SHA" + } >> "$GITHUB_ENV" + echo "Using base commit $RESOLVED_BASE_SHA." + else + echo "BASE_BENCHMARKS_AVAILABLE=false" >> "$GITHUB_ENV" + fi + # Build both JMH jars before measuring either revision. Builds are CPU- and IO-heavy, + # so building and measuring one revision at a time would bias results with different + # thermal, cache, and CPU-frequency state on the shared runner. + - name: "π¨ Build paired JMH jars" + timeout-minutes: 60 + run: | + base_build_ok=false + head_build_ok=false + if [ "${BASE_BENCHMARKS_AVAILABLE:-true}" = "true" ] && [ -f "$BASE_DIR/grails-benchmarks/build.gradle" ]; then + if ( + cd "$BASE_DIR" + ./gradlew :grails-benchmarks:jmhJar --max-workers=4 + ); then + base_build_ok=true + echo "Built base benchmark at $RESOLVED_BASE_SHA." + else + echo "BASE benchmark JAR build failed." + fi + else + echo "BASE does not contain grails-benchmarks; comparison will use HEAD-only mode." + fi + if ( + cd "$HEAD_DIR" + ./gradlew :grails-benchmarks:jmhJar --max-workers=4 + ); then + head_build_ok=true + echo "Built HEAD benchmark at $HEAD_SHA." + else + echo "HEAD benchmark JAR build failed." + fi + { + printf 'BASE_BUILD_OK=%s\n' "$base_build_ok" + printf 'HEAD_BUILD_OK=%s\n' "$head_build_ok" + } >> "$GITHUB_ENV" + # Two forks, three warmup iterations, and five measurement iterations balance PR latency + # against confidence. Reversing shard order cancels first-versus-second runner-state bias. + - name: "π‘οΈ Run paired JMH benchmarks" + timeout-minutes: 60 + run: | + # JMH writes results incrementally, so a run that dies partway can leave a file that + # parses perfectly while describing only some of the benchmarks. The report job decides + # completeness from which files exist, so a partial file would be indistinguishable from + # a good one. Write to a staging path and publish it only on success, so a failed run + # leaves NO file rather than a plausible one. + run_benchmark() { + local revision_dir="$1" + local result_file="$2" + local staging_file="$result_file.partial" + rm -f "$staging_file" "$result_file" + if ( + cd "$revision_dir" && ./gradlew :grails-benchmarks:jmh \ + -Pjmh.include="$JMH_INCLUDE" \ + -Pjmh.forks=2 \ + -Pjmh.warmupIterations=3 \ + -Pjmh.iterations=5 \ + -Pjmh.resultFile="$staging_file" \ + -Pjmh.profilers=gc \ + --max-workers=4 + ) && [ -s "$staging_file" ] && mv "$staging_file" "$result_file"; then + return 0 + fi + rm -f "$staging_file" + return 1 + } + + base_run_failed=false + head_run_failed=false + run_base_benchmark() { + if [ "${BASE_BUILD_OK:-false}" != "true" ]; then + echo "BASE benchmark execution skipped because its JAR was not built." + elif ! run_benchmark "$BASE_DIR" "$RESULT_DIR/base.json"; then + base_run_failed=true + echo "BASE benchmark execution failed." + fi + } + run_head_benchmark() { + if [ "${HEAD_BUILD_OK:-false}" != "true" ]; then + echo "HEAD benchmark execution skipped because its JAR was not built." + elif ! run_benchmark "$HEAD_DIR" "$RESULT_DIR/head.json"; then + head_run_failed=true + echo "HEAD benchmark execution failed." + fi + } + + if [ "$SHARD" = "a" ]; then + run_base_benchmark + run_head_benchmark + else + run_head_benchmark + run_base_benchmark + fi + { + printf 'BASE_RUN_FAILED=%s\n' "$base_run_failed" + printf 'HEAD_RUN_FAILED=%s\n' "$head_run_failed" + } >> "$GITHUB_ENV" + # The comparison is advisory. jmh_compare.py exits successfully for regressions, and this + # step is non-blocking even if results are incomplete because a benchmark execution failed. + # No --pr-number is passed here on purpose: this job renders the report only. The separate + # report job owns comment posting, so a two-shard matrix cannot produce duplicate comments. + - name: "π Compare JMH results" + if: always() + continue-on-error: true + run: | + report_file="$REPORT_DIR/comparison.md" + { + printf '### JMH shard `%s`\n\n' "$SHARD" + if [ "${BASE_RUN_FAILED:-false}" = "true" ]; then + printf 'BASE benchmark execution failed.\n\n' + fi + if [ "${BASE_BUILD_OK:-false}" != "true" ]; then + printf 'BASE benchmark JAR was not built.\n\n' + fi + if [ "${HEAD_RUN_FAILED:-false}" = "true" ]; then + printf 'HEAD benchmark execution failed.\n\n' + fi + if [ "${HEAD_BUILD_OK:-false}" != "true" ]; then + printf 'HEAD benchmark JAR was not built.\n\n' + fi + } > "$report_file" + if [ ! -f "$RESULT_DIR/head.json" ]; then + printf 'HEAD benchmark result was not produced.\n' >> "$report_file" + elif [ "${BASE_BUILD_OK:-false}" = "true" ] && [ -f "$RESULT_DIR/base.json" ]; then + python3 .github/scripts/jmh_compare.py --head "$RESULT_DIR/head.json" --base "$RESULT_DIR/base.json" >> "$report_file" + else + python3 .github/scripts/jmh_compare.py --head "$RESULT_DIR/head.json" >> "$report_file" + fi Review Comment: The new `jmh_compare.py` unit tests arenβt executed anywhere in CI, so regressions in the reporting logic can land unnoticed. Since this step already invokes the script, consider running the stdlib `test_jmh_compare.py` right before generating the shard report. -- 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]
