jdaugherty commented on code in PR #16071: URL: https://github.com/apache/grails-core/pull/16071#discussion_r3699955916
########## .github/workflows/benchmark.yml: ########## @@ -0,0 +1,381 @@ +# 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] Review Comment: `labeled` fires for *any* label, and the job gate at line 46 only checks that `performance` is **present**, not that it was the label just added. So on a PR that already carries `performance`, adding `bug`, `deps`, or anything else kicks off another full two-shard run โ two framework builds and two JMH suites per shard. Given the CI budget argument in the description, worth narrowing: ```yaml if: >- github.event_name == 'workflow_dispatch' || (contains(github.event.pull_request.labels.*.name, 'performance') && (github.event.action != 'labeled' || github.event.label.name == 'performance')) ``` ########## grails-benchmarks/build.gradle: ########## @@ -0,0 +1,268 @@ +/* + * 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. Review Comment: Two of these omissions read differently from the rest. `publish`/`sbom`/`jacoco` follow straightforwardly from "never published". `grails-code-style` doesn't. The trailing note says it stays off "until existing JMH source import-order and fixture-formatting violations can be remediated together", which leaves a new module permanently invisible to `./gradlew codeStyle` and to the four aggregate violation reports the contributor guide asks for before every commit. It's about ten files โ fixing them now avoids establishing the precedent that a new module can opt out. `vulnerability-scan` is worth a second look too: this module pulls JMH and its transitives (jopt-simple, commons-math3) into the default build graph, and nothing else in the repo scans them. ########## .github/workflows/benchmark.yml: ########## @@ -0,0 +1,381 @@ +# 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 }} + # Deliberately allowed to fail the job, and placed before the expensive work: a broken + # comparison tool makes every number produced here untrustworthy. That is a tooling + # failure rather than a performance finding, so regressions themselves stay advisory. + - name: "๐งช Verify JMH comparison tool" + run: ./gradlew :grails-benchmarks:test --max-workers=4 + - 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. The comparison tool 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" + comparison_file="$RUNNER_TEMP/jmh-comparison-$SHARD.md" + 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 + ./gradlew -q --console=plain :grails-benchmarks:jmhCompare --args="--head $RESULT_DIR/head.json --base $RESULT_DIR/base.json --output $comparison_file" + cat "$comparison_file" >> "$report_file" + else + ./gradlew -q --console=plain :grails-benchmarks:jmhCompare --args="--head $RESULT_DIR/head.json --output $comparison_file" + cat "$comparison_file" >> "$report_file" + fi + - name: "๐ Publish JMH report in job summary" + if: always() + run: | + if [ -f "$REPORT_DIR/comparison.md" ]; then + cat "$REPORT_DIR/comparison.md" >> "$GITHUB_STEP_SUMMARY" + else + printf '## JMH benchmark comparison\n\nNo comparison report was produced.\n' >> "$GITHUB_STEP_SUMMARY" + fi + - name: "๐ค Upload JMH artifacts" + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: jmh-results-${{ matrix.shard }} + path: | + jmh-results/${{ matrix.shard }}/ + jmh-reports/${{ matrix.shard }}/ + if-no-files-found: warn + - name: "๐งน Remove paired worktrees" + if: always() + run: | + WORKTREE_ROOT="$RUNNER_TEMP/jmh-worktrees/$SHARD" + git worktree remove --force "$WORKTREE_ROOT/base" || true + git worktree remove --force "$WORKTREE_ROOT/head" || true + + report: + name: "Publish JMH benchmark comparison" + needs: benchmark + if: ${{ always() && github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'performance') && github.event.pull_request.head.repo.full_name == github.repository }} Review Comment: `always()` also runs this job when the `benchmark` jobs failed outright. Concrete case: the "๐งช Verify JMH comparison tool" gate fails, every later step in the shard job is skipped, so no results are uploaded. The pooling step then falls through to the final `else` and posts *"No HEAD benchmark results were produced"* โ which, because the comment is sticky, **replaces the previous good report on that PR with an empty one**. A tooling failure silently destroys the last known-good numbers. Worth either skipping the post step when there are zero results (leaving the prior comment intact), or making the message say the run failed and the earlier report still stands. ########## .github/workflows/benchmark.yml: ########## @@ -0,0 +1,381 @@ +# 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 }} Review Comment: `PR_NUMBER` and `REPOSITORY` are never referenced in this job โ the compare step deliberately doesn't post, and the `report` job declares its own copies at lines 271-272. Dead as written; dropping them keeps it clear that this job has no comment-posting path. ########## grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/interceptors/UrlMappingMatcherBenchmark.java: ########## @@ -0,0 +1,144 @@ +/* + * 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.interceptors; + +import java.util.Collections; +import java.util.Map; +import java.util.concurrent.TimeUnit; + +import grails.web.mapping.UrlMappingData; +import grails.web.mapping.UrlMappingInfo; +import org.grails.plugins.web.interceptors.UrlMappingMatcher; +import org.grails.web.servlet.mvc.GrailsWebRequest; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +/** + * Measures URI matcher decisions that select Grails interceptors during request dispatch. Both + * matching and rejected paths matter because every interceptor evaluates incoming requests. + */ +@State(Scope.Benchmark) +@Threads(1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) +@Fork(value = 2, jvmArgsAppend = {"-Xms1g", "-Xmx1g", "-XX:+UseG1GC"}) +public class UrlMappingMatcherBenchmark { + + private UrlMappingMatcher matcher; + private UrlMappingInfo mappingInfo; + + @Setup + public void setup() { Review Comment: `UrlMappingsBenchmark` guards its fixture in `@Setup` and fails the run rather than publish wrong numbers, and the description makes a strong case for why. This benchmark has the same failure mode with no guard: if `doesMatch` ever stops returning `true` for `/orders/42`, `matchUriPattern` silently becomes a second copy of `rejectNonMatchingUriPattern` and the report shows two plausible, nearly identical numbers with no error anywhere. I ran a probe on this branch against `sourceSets.jmh.runtimeClasspath` and confirmed the fixtures are all fine today โ `doesMatch` returns `true` then `false`, both binder targets come back fully populated, both templates render non-empty โ so this is not a live defect. But the same one-line assertion applies here, in `SimpleDataBinderBenchmark` (`SimpleDataBinder.bind` skips properties it can't bind, silently), and in `ViewTemplateRenderingBenchmark`. Given the silent-no-op story is the lesson the description leads with, the guard is worth applying uniformly rather than only where the bug happened to be found. ########## grails-benchmarks/src/test/groovy/org/apache/grails/benchmarks/report/GoldenReportSpec.groovy: ########## @@ -0,0 +1,71 @@ +/* + * 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 spock.lang.Unroll + +import java.nio.file.Files +import java.nio.file.Path + +class GoldenReportSpec extends Specification { + @TempDir + Path temporaryDirectory + + @Unroll + def "#name report exactly matches the Python golden file"() { Review Comment: Leftover from the Python implementation this replaced โ "the Python golden file" here, and "matches Python percent g rendering" in `JmhCompareSpec` line 81. There's no Python anywhere in the PR now, so these read as references to something that doesn't exist. The fixtures are still worth keeping as byte-exact locks; they're just locking the Groovy renderer's output. ########## .github/workflows/benchmark.yml: ########## @@ -0,0 +1,381 @@ +# 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 }} + # Deliberately allowed to fail the job, and placed before the expensive work: a broken + # comparison tool makes every number produced here untrustworthy. That is a tooling + # failure rather than a performance finding, so regressions themselves stay advisory. + - name: "๐งช Verify JMH comparison tool" + run: ./gradlew :grails-benchmarks:test --max-workers=4 + - 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 \ Review Comment: Both revisions were built in the preceding step, so by the time measurement starts there are up to two Gradle daemons resident (3h default expiry) alongside this build's own, all on a 4-core runner, for the whole measurement window. Idle daemons still hold heap and run background GC. The rulers exist to *detect* exactly this class of noise โ cheaper to remove a known source of it. Either `--no-daemon` on the two measurement invocations, or a `./gradlew --stop` between the build step and this one, would leave the JMH fork as the only significant JVM competing for the runner. ########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/JmhCompare.groovy: ########## @@ -0,0 +1,116 @@ +/* + * 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 + +@CompileStatic +class JmhCompare { + static void main(String[] args) { + int exit = run(args, new GitHubComments()) + if (exit != 0) { + System.exit(exit) + } + } + + static int run(String[] args, CommentPoster poster) { + return run(args, poster, System.getenv()) + } + + static int run(String[] args, CommentPoster poster, Map<String, String> environment) { + try { + Map<String, String> options = parse(args) + if (options.containsKey('post-file')) { + return postFile(options, poster, environment) + } + double threshold = options.containsKey('threshold') ? Double.parseDouble(options.get('threshold')) : .10D + if (!Double.isFinite(threshold) || threshold <= 0D) { + throw new IllegalArgumentException('--threshold must be greater than zero') + } + String headPath = options.get('head') + if (!headPath) { + throw new IllegalArgumentException('--head is required') + } + Map<String, Map<String, Benchmark>> head = JmhResults.readShards(headPath) + String basePath = options.get('base') + String report = basePath && Files.exists(Path.of(basePath)) + ? ReportRenderer.render(BenchmarkComparator.compareShards(head, JmhResults.readShards(basePath), threshold, + options.getOrDefault('expected-shards', '').split(',').findAll { String value -> !value.trim().isEmpty() })) + : ReportRenderer.headOnly(JmhResults.poolShards(head)) + System.out.println(report) + if (options.containsKey('output')) { + Files.writeString(Path.of(options.get('output')), report + '\n', StandardCharsets.UTF_8) + } + postWhenConfigured(report, options, poster, environment) + return 0 + } catch (Exception error) { + System.err.println("error: ${error.message}") + return 2 + } + } + + private static int postFile(Map<String, String> options, CommentPoster poster, Map<String, String> environment) { + if (options.containsKey('head')) { + throw new IllegalArgumentException('--post-file cannot be used with --head') + } + String report = Files.readString(Path.of(options.get('post-file')), StandardCharsets.UTF_8) + postWhenConfigured(report, options, poster, environment) + return 0 + } + + private static void postWhenConfigured(String report, Map<String, String> options, CommentPoster poster, Map<String, String> environment) { + String pr = options.getOrDefault('pr-number', '').trim() + String repo = options.get('repo') + String token = environment.get('GITHUB_TOKEN') + if (!pr || pr == 'null' || !repo || !token) { + if (options.containsKey('post-file')) { + System.err.println('warning: --post-file requires --repo, --pr-number, and GITHUB_TOKEN; skipping comment post') + } + return + } + try { + poster.post(report, repo, pr, token) + } catch (Exception error) { + System.err.println("warning: unable to post JMH report: ${error.message}") + } + } + + private static Map<String, String> parse(String[] args) { + Set<String> values = ['head', 'base', 'threshold', 'repo', 'pr-number', 'expected-shards', 'output', 'post-file'] 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 (key == 'fail-on-regression') { Review Comment: `--fail-on-regression` is parsed and stored but never read โ `run()` returns 0 regardless, and `JmhCompareSpec` line 113 ("fail on regression remains successful") locks that in. Since gating is explicitly out of scope for this PR, I'd drop the flag and that test rather than ship a CLI option whose name promises behaviour it doesn't have. As it stands someone reaches for it later, sees it accepted without error, and assumes the build is gated. ########## grails-benchmarks/build.gradle: ########## @@ -0,0 +1,268 @@ +/* + * 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. + +sourceSets { + report { + groovy.srcDirs = ['src/report/groovy'] + } +} + +// 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' + + reportImplementation platform(project(':grails-bom')) + reportImplementation 'org.apache.groovy:groovy' + reportImplementation 'org.apache.groovy:groovy-json' + + // Pull the report source set's full runtime classpath (groovy-json etc.), not only its + // compiled classes - the Spock suite exercises JsonSlurper and HttpClient paths. + testImplementation sourceSets.report.runtimeClasspath + testImplementation 'org.apache.groovy:groovy-test-junit5' + testImplementation 'org.junit.jupiter:junit-jupiter-api' + testImplementation 'org.junit.platform:junit-platform-suite' + testImplementation 'org.spockframework:spock-core' + testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine' +} + +apply from: rootProject.layout.projectDirectory.file('gradle/test-config.gradle') + +tasks.register('jmhCompare', JavaExec) { + group = 'verification' + description = 'Compares JMH JSON results and renders a pull request report.' + classpath = sourceSets.report.runtimeClasspath + mainClass = 'org.apache.grails.benchmarks.report.JmhCompare' +} + +// 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).withCloseable { input -> + input.getText('UTF-8').readLines().each { String line -> + if (line) { + lines.add(line) + } + } + } + } else if (propertiesMetadataPaths.contains(path)) { + Properties properties = new Properties() + zipFile.getInputStream(entry).withCloseable { input -> + properties.load(input) + } + Map<String, Set<String>> entries = propertyEntries.computeIfAbsent(path) { + new TreeMap<>() + } + properties.stringPropertyNames().each { String key -> + Set<String> values = entries.computeIfAbsent(key) { new TreeSet<>() } + properties.getProperty(key).split(',').each { String value -> + if (value) { + values.add(value.trim()) + } + } + } + } + } + } + } + + lineEntries.each { String path, Set<String> lines -> + File outputFile = new File(outputDirectory, path) + outputFile.parentFile.mkdirs() + outputFile.setText("${lines.join('\n')}\n", 'UTF-8') + } + extensionModuleEntries.each { String path, Map<String, Set<String>> entries -> + File outputFile = new File(outputDirectory, path) + outputFile.parentFile.mkdirs() + String moduleName = path.startsWith('META-INF/groovy/') + ? 'grails-benchmark-groovy-extension-modules' + : 'grails-benchmark-service-extension-modules' + outputFile.withWriter('UTF-8') { writer -> + writer.write("moduleName=${moduleName}\n") + writer.write('moduleVersion=1.0\n') + entries.each { String key, Set<String> values -> + writer.write("${key}=${values.join(',')}\n") + } + } + } + propertyEntries.each { String path, Map<String, Set<String>> entries -> + File outputFile = new File(outputDirectory, path) + outputFile.parentFile.mkdirs() + outputFile.withWriter('UTF-8') { writer -> + entries.each { String key, Set<String> values -> + String escapedKey = key.replace('\\', '\\\\').replace('=', '\\=').replace(':', '\\:') + writer.write("${escapedKey}=${values.join(',')}\n") + } + } + } + } +} + +tasks.named('jmhJar') { + dependsOn mergeJmhClasspathMetadata + def mergedMetadataDirectory = layout.buildDirectory.dir('generated/jmh-classpath-metadata') + def mergedMetadataPath = mergedMetadataDirectory.get().asFile.absolutePath + File.separator + def lineMetadataPrefixes = ['META-INF/services/', 'META-INF/groovy/'] + def propertiesMetadataPaths = [ + 'META-INF/spring.factories', + 'META-INF/spring.handlers', + 'META-INF/spring.schemas' + ] + + from(mergedMetadataDirectory) + eachFile { details -> + boolean mergeable = lineMetadataPrefixes.any { String prefix -> details.path.startsWith(prefix) } || + propertiesMetadataPaths.contains(details.path) + if (mergeable && !details.file.absolutePath.startsWith(mergedMetadataPath)) { + details.exclude() + } + } + duplicatesStrategy = DuplicatesStrategy.EXCLUDE +} + +tasks.named('check') { + // Benchmarks are not otherwise compiled by the standard lifecycle tasks. + dependsOn tasks.named('jmhClasses') Review Comment: Wiring `jmhClasses` into `check` means every root `./gradlew build`/`check` now compiles the JMH source set and resolves JMH, plus the six `implementation project(...)` dependencies, for a module only exercised by an opt-in workflow. That's a cost on every developer and CI build, which sits a little oddly beside the budget discipline argued for the workflow itself. The alternative โ letting the workflow's `jmhJar` be the only compile gate โ risks the benchmarks rotting silently between labelled runs, so I don't think the current choice is wrong. But it deserves to be a recorded decision rather than a bare `dependsOn`; could the comment say why compile-on-every-build was preferred? -- 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]
