jdaugherty commented on code in PR #16071: URL: https://github.com/apache/grails-core/pull/16071#discussion_r3721632692
########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/ReportRenderer.groovy: ########## @@ -0,0 +1,148 @@ +/* + * 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 + +@CompileStatic +class ReportRenderer { + + static final String MARKER = '<!-- grails-jmh-benchmark -->' + static final String DASH = 'โ' + static final String BENCHMARK_PACKAGE = 'org.apache.grails.benchmarks.' + + static String safe(String value) { + value.replaceAll('[`|<>\\[\\]\\(\\)!\\r\\n]', '') + } + + static String number(Double value) { + if (value == null || !Double.isFinite(value)) { + return DASH + } + String rendered = String.format(Locale.ROOT, '%.3g', value) + int exponent = rendered.indexOf('e') + String mantissa = exponent >= 0 ? rendered.substring(0, exponent) : rendered + String suffix = exponent >= 0 ? rendered.substring(exponent) : '' + if (mantissa.contains('.')) { + mantissa = mantissa.replaceFirst('0+$', '').replaceFirst('\\.$', '') + } + return mantissa + suffix + } + + static String speedup(Double value) { + value != null && Double.isFinite(value) + ? String.format(Locale.ROOT, '%.2fx', value) + : DASH + } + + static String identity(String value) { + safe(removePrefix(value, BENCHMARK_PACKAGE)) + } + + static String score(Benchmark value) { + String rendered = number(value.score) + rendered == DASH ? DASH : rendered + ' ' + safe(value.unit) + } + + static String allocation(ComparisonRow row) { + if (row.allocationDelta == null || row.allocationPercent == null) { + return DASH + } + if (Math.abs(row.allocationDelta) < 1D) { + return '~0 B/op' + } + return String.format(Locale.ROOT, '%+,.0f B/op (%+.1f%%)%s', + row.allocationDelta, + row.allocationPercent * 100D, + row.allocationCandidate ? ' **candidate**' : '') + } + + static String render(Comparison comparison) { + int regressions = comparison.rows.count { ComparisonRow row -> row.verdict == 'REGRESSED' }.intValue() + int improvements = comparison.rows.count { ComparisonRow row -> row.verdict == 'IMPROVED' }.intValue() + List<RulerDeviation> deviations = comparison.rulerDeviations ?: comparison.rulerSpeedups.collect { List<Object> item -> new RulerDeviation('', (String) item[0], (Double) item[1]) } + RulerDeviation worst = deviations ? deviations.max { RulerDeviation item -> deviation(item.speedup) } : null + String health = comparison.rulerIncomplete + ? 'INCOMPLETE/unreliable (missing or non-finite ruler measurements)' + : worst == null + ? 'not measured' + : "worst ruler deviation: ${String.format(Locale.ROOT, '%.1f%%', deviation(worst.speedup) * 100D)}${worst.shard ? ' in ' + safe(worst.shard) : ''} (${safe(removePrefix(worst.identity, BenchmarkComparator.RULER_PACKAGE))})" + List<String> lines = ['### JMH Benchmark Report', '', "**Regressions:** ${regressions}".toString(), "**Improvements:** ${improvements}".toString(), "**Runner health:** ${health}".toString(), 'Ruler benchmarks are excluded from verdicts and group summaries; runner health is a stability check, not a calibration factor.'] + if (comparison.missingShards) lines.addAll(['', '> **Warning:** No usable base/head pair was produced by ' + comparison.missingShards.collect { String item -> safe(item) }.join(', ') + ". This comparison rests on ${comparison.pairedShards.size()} shard pair(s) instead of the expected ${comparison.pairedShards.size() + comparison.missingShards.size()}, so the alternating measurement order did not fully cancel and the result is weaker than a normal run."]) + if (deviations) { + lines.add('**Ruler movements:** ' + deviations.collect { RulerDeviation item -> Review Comment: Line 85 emits a `>` blockquote and this line follows it with no blank line, so GFM lazy continuation pulls "Ruler movements" *inside* the warning blockquote. Confirmed against GitHub's own renderer (`POST /markdown`, `mode: gfm`): ```html <blockquote> <p><strong>Warning:</strong> No usable base/head pair was produced by shard-c.json.<br> <strong>Ruler movements:</strong> shard-a.json: CpuRuler.run: 0.89x, ...</p> </blockquote> ``` This only fires when `missingShards` is non-empty โ which is exactly the degraded run the warning exists to call out, and the one where the ruler numbers matter most. Prefixing the `lines.add` with `''` fixes it. The two warnings below (lines 93 and 94) already do this correctly, so it's just this one. Worth noting the golden fixtures can't catch this class of bug: they lock the Markdown *source* bytes (`expected-shards.md` lines 8-9 encode the current, broken layout), not the rendered output. ########## .github/workflows/benchmark.yml: ########## @@ -0,0 +1,396 @@ +# 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') && + (github.event.action != 'labeled' || github.event.label.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: '.*' + 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 Review Comment: This gate is placed first, and the comment above it explains why: a broken comparison tool should fail in seconds rather than after an hour of building and benchmarking two revisions. But `:grails-benchmarks:test` isn't seconds โ it builds 33 framework projects before it can run a single spec. The `test` source set inherits `main`'s output, and `main` is where the Groovy fixtures live, so the six `implementation project(...)` dependencies come along. Measured on this branch: | gate | tasks | projects | | --- | ---: | ---: | | `:grails-benchmarks:test` (current) | 222 | 33 | | isolated `reportTest` source set | 9 | 1 | I probed the isolated version โ a `reportTest` source set whose classpath is `sourceSets.report.output` plus Spock and `groovy-json`, with its own `Test` task โ and every spec passes unchanged in 19s including compilation. No spec touches the fixtures, so nothing is lost by cutting `main` out of the classpath. `README.adoc` already states this as the design intent: "The `report` source set depends on Groovy alone and on no Grails project." The *test* source set is what breaks that isolation. One thing to watch if you split it: `grails-test-report` collects test tasks with `tasks.withType(Test).matching { it.name == phase }`, so a task named anything other than `test` drops out of the aggregate report and would need adding there. ########## grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/gsp/GroovyPageParserBenchmark.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.gsp; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.TimeUnit; + +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.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +import org.grails.gsp.compiler.GroovyPageParser; + +/** + * Measures GSP source parsing into generated Groovy source, the work performed when GSP templates + * are prepared. Parser regressions slow application startup and template reloads. + */ +@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 GroovyPageParserBenchmark { Review Comment: This is the one benchmark left without a fixture guard โ it has no `@Setup` at all. `UrlMappingsBenchmark`, `UrlMappingMatcherBenchmark`, `SimpleDataBinderBenchmark` and `ViewTemplateRenderingBenchmark` all now fail the run rather than publish a silent no-op. The same failure mode applies here, and arguably more directly than anywhere else: the measured work *is* the volume of generated source. If `parse()` ever returned an empty or near-empty stream, both methods would keep producing plausible, self-consistent, meaningless numbers with no error anywhere. A `@Setup` that parses both templates once and asserts the generated source is non-trivial (and, for `TAGGED_TEMPLATE`, that it actually contains the tag output) would close the last gap in the fail-loud story. ########## .github/workflows/benchmark.yml: ########## @@ -0,0 +1,396 @@ +# 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') && + (github.event.action != 'labeled' || github.event.label.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: '.*' + 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" + - name: "๐ Stop Gradle daemons before measurement" + if: always() + run: | + ./gradlew --stop || true + if [ -n "${BASE_DIR:-}" ] && [ -d "$BASE_DIR" ]; then (cd "$BASE_DIR" && ./gradlew --stop) || true; fi + if [ -n "${HEAD_DIR:-}" ] && [ -d "$HEAD_DIR" ]; then (cd "$HEAD_DIR" && ./gradlew --stop) || true; fi + # 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" Review Comment: Minor follow-on to the daemon fix: the `--stop` above clears the daemons left by the build phase, but not the one the *first* measurement starts. Whichever revision runs second in a shard is measured on a daemon already warmed and loaded by the first (Gradle reuses a compatible idle daemon across project directories, so it's the same JVM rather than a second one). The alternating shard order cancels this on average, which is why it's a nit rather than a finding. But `--no-daemon` on the two `run_benchmark` invocations would make both halves start from identical daemon state and remove a source of noise the rulers would otherwise have to absorb. ########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/CommentPoster.groovy: ########## @@ -0,0 +1,111 @@ +/* + * 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.json.JsonOutput +import groovy.json.JsonSlurper +import groovy.transform.CompileStatic + +import java.net.http.HttpClient +import java.net.http.HttpRequest +import java.net.http.HttpResponse +import java.time.Duration + +@CompileStatic +interface CommentPoster { + + void post(String report, String repo, String prNumber, String token) +} + +@CompileStatic +class GitHubComments implements CommentPoster { + + static final int MAX_BODY = 65000 + static final String TRUNCATION = '\n\n_Report truncated. The full report is available in the workflow artifacts._\n\n<!-- grails-jmh-benchmark -->' + private final HttpClient client = HttpClient.newBuilder().connectTimeout(Duration.ofSeconds(15)).build() + + @Override + void post(String report, String repo, String prNumber, String token) { Review Comment: `post` itself is untested. The specs cover `commentBody` and `latestMatchingCommentId`, and everything through `JmhCompare` uses `Mock(CommentPoster)`, so the paginated GET loop and the create-versus-update decision on line 63 never execute in a test. That's the sticky-comment mechanism โ the thing whose failure mode is a duplicate report on every push rather than an updated one, which is the specific behaviour the marker exists to prevent. A `com.sun.net.httpserver.HttpServer` stub would cover the page-boundary walk (the `comments.size() < 100` break) and assert PATCH-versus-POST against a canned comment list. Worth flagging that coverage tooling won't catch this for you: the module deliberately omits `grails-jacoco`, so the "all modified and coverable lines are covered by tests" report on this PR says nothing about `grails-benchmarks`. Unrelated, while you're in the specs: `JmhCompareSpec`'s first two features ("speedup respects throughput and latency direction", "verdict requires threshold and disjoint intervals") duplicate five features in `BenchmarkComparatorSpec` exactly. ########## grails-benchmarks/build.gradle: ########## @@ -0,0 +1,270 @@ +/* + * 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' + id 'org.apache.grails.buildsrc.vulnerability-scan' + id 'org.apache.grails.gradle.grails-code-style' +} + +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.gradle.grails-jacoco, and org.apache.grails.buildsrc.dependency-validator. +// Its JMH dependency is GPLv2 with the Classpath Exception (Category X) and must not be published. + +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 Review Comment: `the Validateable trait` is stale โ there is no Validateable fixture in the module. `README.adoc` line 93 carries the same claim in its Layout section, and is then contradicted by its own "Known gap: validation" section 28 lines later, which explains that the validation benchmark was written and withdrawn. Same category as the Python leftovers from the last round: a reader looking for the Groovy fixture that needs the trait won't find one. The remaining two examples (the URL mappings DSL and view templates) carry the argument on their own. ########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/JmhResults.groovy: ########## @@ -0,0 +1,139 @@ +/* + * 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.json.JsonSlurper +import groovy.transform.CompileStatic + +import java.nio.file.Files +import java.nio.file.Path + +@CompileStatic +class JmhResults { + + static final String ALLOCATION_METRIC = 'gc.alloc.rate.norm' + + static Double finiteNumber(Object value) { + if (value == null) { + return null + } + try { + Double number = Double.valueOf(value.toString()) + return Double.isFinite(number) ? number : null + } catch (NumberFormatException ignored) { + return null + } + } + + static List<Double> confidenceInterval(Map<String, Object> metric) { + Object value = metric.get('scoreConfidence') + if (value instanceof List && ((List<?>) value).size() == 2) { + Double lower = finiteNumber(((List<?>) value).get(0)) + Double upper = finiteNumber(((List<?>) value).get(1)) + if (lower != null && upper != null) { + return [Math.min(lower, upper), Math.max(lower, upper)] + } + } + Double score = finiteNumber(metric.get('score')) + Double error = finiteNumber(metric.get('scoreError')) + return score != null && error != null && error >= 0D ? [score - error, score + error] : null + } + + static String identity(String name, Map<String, Object> params) { Review Comment: `identity` keys on benchmark name plus params but not on `mode`, and `parseEntries` does a plain `benchmarks.put(benchmarkId, ...)`. A benchmark declaring more than one mode โ `@BenchmarkMode({Mode.AverageTime, Mode.Throughput})` โ emits one JSON entry per mode under the same name, so all but the last are silently dropped. Nothing in the suite does this today, and `comparable()` plus `poolBenchmarks` correctly refuse to compare across modes, so it can't produce a *wrong* verdict. But it can silently shrink the comparison set, surfacing only as an anonymous "Malformed comparisons skipped: N" count โ the same silent-shrink failure that `jmh.failOnError = true` was set to prevent on the measurement side. Adding mode to the identity key would make it a visible `Only in head` / `Only in base` entry instead. ########## .github/workflows/benchmark.yml: ########## @@ -0,0 +1,396 @@ +# 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') && + (github.event.action != 'labeled' || github.event.label.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: '.*' + 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" + - name: "๐ Stop Gradle daemons before measurement" + if: always() + run: | + ./gradlew --stop || true + if [ -n "${BASE_DIR:-}" ] && [ -d "$BASE_DIR" ]; then (cd "$BASE_DIR" && ./gradlew --stop) || true; fi + if [ -n "${HEAD_DIR:-}" ] && [ -d "$HEAD_DIR" ]; then (cd "$HEAD_DIR" && ./gradlew --stop) || true; fi + # 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' && Review Comment: `github.event_name == 'pull_request'` here means a `workflow_dispatch` run can never reach the reporting job, so it produces artifacts and per-shard job summaries but no sticky comment. The PR description says a comment appears for `workflow_dispatch` too. There's a second consequence: on dispatch there's no `pull_request` context, so `BASE_SHA` is empty and base resolution in "Prepare paired worktrees" falls through the merge-base branch to `git rev-parse "$HEAD_SHA^"`. A manual run therefore benchmarks HEAD against its own parent commit, not against a release branch. Both behaviours are defensible, but neither is what the description promises. Either document what dispatch actually does, or drop the trigger. ########## grails-benchmarks/src/report/groovy/org/apache/grails/benchmarks/report/CommentPoster.groovy: ########## @@ -0,0 +1,111 @@ +/* + * 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.json.JsonOutput +import groovy.json.JsonSlurper +import groovy.transform.CompileStatic + +import java.net.http.HttpClient +import java.net.http.HttpRequest +import java.net.http.HttpResponse +import java.time.Duration + +@CompileStatic +interface CommentPoster { + + void post(String report, String repo, String prNumber, String token) +} + +@CompileStatic +class GitHubComments implements CommentPoster { + + static final int MAX_BODY = 65000 + static final String TRUNCATION = '\n\n_Report truncated. The full report is available in the workflow artifacts._\n\n<!-- grails-jmh-benchmark -->' + private final HttpClient client = HttpClient.newBuilder().connectTimeout(Duration.ofSeconds(15)).build() + + @Override + void post(String report, String repo, String prNumber, String token) { + String body = commentBody(report) + Long commentId = null + for (int page = 1; page <= 100; page++) { + Object response = request( + "https://api.github.com/repos/${repo}/issues/${prNumber}/comments?per_page=100&page=${page}";, + token, + 'GET', + null) + if (!(response instanceof List)) { + break + } + List<?> comments = (List<?>) response + Long pageMatch = latestMatchingCommentId(comments) + if (pageMatch != null) { + commentId = pageMatch + } + if (comments.size() < 100) { + break + } + } + String endpoint = commentId == null + ? "https://api.github.com/repos/${repo}/issues/${prNumber}/comments"; + : "https://api.github.com/repos/${repo}/issues/comments/${commentId}"; + request(endpoint, token, commentId == null ? 'POST' : 'PATCH', JsonOutput.toJson([body: body])) + } + + /** + * Keep the last matching comment on a page so duplicate marker comments update the + * newest bot report (GitHub returns comments oldest-first). + */ + static Long latestMatchingCommentId(List<?> comments) { + Long commentId = null + for (Object comment : comments) { + if (!(comment instanceof Map) || !markerComment((Map<String, Object>) comment)) { + continue + } + Object id = ((Map<String, Object>) comment).get('id') + if (id instanceof Number) { + commentId = ((Number) id).longValue() + } + } + return commentId + } + + static String commentBody(String report) { + String body = report.contains(ReportRenderer.MARKER) + ? report + : report + '\n\n' + ReportRenderer.MARKER + if (body.length() > MAX_BODY) { + body = body.substring(0, MAX_BODY - TRUNCATION.length()) + TRUNCATION + } + return body + } + + private static boolean markerComment(Map<String, Object> comment) { + Object body = comment.get('body') + Object user = comment.get('user') + Object login = user instanceof Map ? ((Map<String, Object>) user).get('login') : null + return body instanceof String && ((String) body).contains(ReportRenderer.MARKER) && (!(login instanceof String) || login == 'github-actions[bot]') Review Comment: The author check defaults the wrong way: `!(login instanceof String) || login == 'github-actions[bot]'` treats a comment whose `user` is absent or non-object as ours. If GitHub returns a comment without a usable `user` (deleted accounts are the usual way this happens) and that body contains the marker, it becomes a PATCH target. The reporting job holds `pull-requests: write`, which is enough to edit any comment in the repository, so the failure mode is overwriting someone else's comment rather than a harmless 403. Requiring the bot instead of assuming it โ `login == 'github-actions[bot]'` โ keeps the intended behaviour and makes the unknown-author case fall through to posting a fresh comment, which is the safe direction. -- 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]
