jamesfredley commented on code in PR #16071: URL: https://github.com/apache/grails-core/pull/16071#discussion_r3685933051
########## 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: Good catch, fixed in b06d51784c. This was a genuine inconsistency rather than a theoretical one: of the three places `mergeJmhClasspathMetadata` reads a zip entry, the two `Properties.load` branches already wrapped the stream in `withCloseable`, and only the line-metadata branch called `.getText('UTF-8')` on an unclosed stream. That branch is the one that runs most often, since it handles every `META-INF/services/**` and `META-INF/groovy/**` entry across the whole jmh runtime classpath. It now matches the other two: ```groovy zipFile.getInputStream(entry).withCloseable { input -> input.getText('UTF-8').readLines().each { String line -> if (line) { lines.add(line) } } } ``` Verified the fix changes nothing about the merge output: a full `--rerun-tasks` rebuild of `:grails-benchmarks:jmhJar` still produces the merged `META-INF/services/org.codehaus.groovy.runtime.ExtensionModule` with all 9 extension classes, and `META-INF/spring.factories` with 24 merged keys. All 13 benchmarks still execute cleanly from the rebuilt jar. ########## .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: Agreed, and fixed in b06d51784c - this was a real gap. The 39 tests were the only thing standing behind the comparison logic, and nothing in CI ran them. I've added a `๐งช Verify JMH comparison script` step that runs `python3 .github/scripts/test_jmh_compare.py`, with one deliberate change from your suggestion: rather than placing it immediately before the report is generated, it runs right after checkout and **before** the JDK/Gradle setup and the build-and-measure steps. A broken reporter should fail in seconds rather than after roughly an hour of building and benchmarking two revisions, and the shard job is the right home because it runs for fork PRs too, which never reach the reporting job. The step is also deliberately allowed to **fail** the job. That is an intentional exception to this workflow's "never fails a build" rule, and the YAML carries a comment saying so, because the rule governs performance *regressions* (which stay advisory) whereas a broken comparison script invalidates every number the run produces. Without the note, the obvious later "fix" would be to add `continue-on-error: true` and silently defeat the guard. Worth noting these tests are not decorative - the same suite already caught several defects during development, including shard results being silently overwritten instead of pooled, and a benchmark receiving a verdict after its JMH mode changed. -- 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]
