jamesfredley commented on code in PR #16071: URL: https://github.com/apache/grails-core/pull/16071#discussion_r3706555344
########## .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: Fixed in 202d206a6a. Both the `benchmark` and `report` job gates now require: ```yaml github.event.action != 'labeled' || github.event.label.name == 'performance' ``` So once a PR already carries `performance`, adding `bug`/`deps`/etc. no longer kicks off another two-shard run. ########## .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: Fixed in 202d206a6a. Dropped `PR_NUMBER` and `REPOSITORY` from the shard job env. They remain only on the `report` job, which is the sole comment-posting path. ########## .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: Fixed in 202d206a6a. Added a `Stop Gradle daemons before measurement` step between the paired `jmhJar` builds and the JMH runs. It runs `./gradlew --stop` in the main workspace and both worktrees so idle daemons are not competing with the JMH forks during the measurement window. ########## .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: Fixed in 202d206a6a. The pool/compare step now sets `should_post=true` only when complete pairs or HEAD-only results exist. The comment step is gated on that output. When nothing was produced, the job summary says the run failed and the previous sticky report was left unchanged - no empty overwrite, and the sticky marker is not written in that path. ########## 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: Fixed in 202d206a6a. Removed the `--fail-on-regression` parse branch and the test that locked in the no-op success path. Passing the flag now fails CLI parsing (missing value / unknown option) instead of silently accepting it. ########## 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: Fixed in 202d206a6a. Added `assertFixtureMatches()` guards called from `@Setup` in: - `UrlMappingMatcherBenchmark` - match `/orders/42`, reject `/catalog/42` - `SimpleDataBinderBenchmark` - both flat and type-converted binds populate all fields - `ViewTemplateRenderingBenchmark` - JSON and markup renders contain expected content Same fail-loud pattern as `UrlMappingsBenchmark`. ########## 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: Fixed in 202d206a6a. Enabled `org.apache.grails.gradle.grails-code-style` and `org.apache.grails.buildsrc.vulnerability-scan`. Import-order / formatting issues in the module were cleaned so style checks pass. Publish/sbom/jacoco/dependency-validator stay off because this harness is build-time-only and JMH is Category X. ########## 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: Fixed in 202d206a6a. Documented the decision on `check.dependsOn jmhClasses`: compile-on-every-build is preferred over workflow-only compilation so the suite cannot silently bit-rot between labelled runs; `jmhJar` remains the packaging gate. ########## 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: Fixed in 202d206a6a. Renamed the golden test to `#name report exactly matches the golden fixture` and the number-format test to `matches percent-g style rendering`. No Python references remain under `grails-benchmarks`. Also normalized CRLF in the golden comparison so Windows CI does not false-fail on line endings. -- 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]
