diff --git a/.github/PERFORMANCE.md b/.github/PERFORMANCE.md index 53a6182e3..84ec6fceb 100644 --- a/.github/PERFORMANCE.md +++ b/.github/PERFORMANCE.md @@ -1,95 +1,69 @@ # Release performance CI -`Nitro Performance` builds `apps/benchmark` in its ordinary Release/Hermes configuration, -then runs base → head → head → base on one booted target. A noisy comparison can -run one additional head/base pair. Each suite measures 40 cases with five warmup -batches and twenty samples, calibrated toward 150 ms (roughly 100–200 ms) of -timed work per sample. Iteration counts are rounded to two significant digits -and frozen after warmup; measured outliers are retained. Allocation-heavy samples -sum bounded timed chunks with explicit GC and native-cleanup yields between chunks excluded from timing, -so memory-safety limits do not shorten the sample. See `iterations` and -`chunkIterations` in raw results; sample milliseconds are `ns/op * iterations / 1e6`. -Calibration changes and version-2 benchmark definitions require a new baseline. -Each binary is installed once per suite run. Each case uses a fresh app process -so the runtime-scoped JSI reference cache does not accumulate millions of weak -reference records across unrelated cases. Startup is outside measured work; -the host validates and combines the per-case results, preserving their raw samples. -The host computes confidence intervals by resampling whole matched base/head -process runs, then batches within each selected run. Bencher median bounds use -the same run-aware approach. Treating all batches from multiple processes as -independent gave a false-positive control result in the initial A/A validation; -process-level variability must be preserved in the uncertainty estimate. -Robust CV remains visible and causes a neutral result when uncertainty overlaps -zero. It is not an unconditional veto: an effect beyond the budget whose full -run-aware confidence interval excludes zero is still reported in its direction. +`Nitro Performance` builds the dedicated `apps/benchmark` Release/Hermes app. +Each platform runs base → head → head → base on the same machine, reversing case +order for the second pair. Each case gets a fresh app process; installation, +startup, transport and process restarts are outside timing. There is no automatic +third pair. Manual reruns are retained as identifiable workflow attempts. -The benchmark app is separate from `apps/example`, which keeps the demos and -Harness correctness tests. It shares the real Nitro test packages, but has no -Harness, navigation, screens, or safe-area dependencies. The TurboModule control -and all benchmark cases belong to the benchmark app. Android permits -cleartext only to loopback; the example app does not contain the CI entrypoint. -See [`apps/benchmark/README.md`](../apps/benchmark/README.md) for local commands. +Each case records twenty ordered batch averages after five warmup batches. +Calibration targets 150 ms of timed work; this target does not establish steady +state or erase drift. Allocation-heavy cases sum bounded timed chunks with +explicit cleanup outside timing. Raw `iterations`, `chunkIterations` and ordered +`samplesNsPerOp` describe the work; timed sample milliseconds are +`ns/op * iterations / 1e6`. Slow samples are retained. -## Android host requirements +## Reading results + +The main score is the median (p50) of batch averages in ns/op, not individual-call +tail latency. The report shows every observed change of at least 5%, including +Promise cases. This is a presentation threshold, not a calibrated regression +budget. Expand the report for all metrics, individual process medians, matched +pair changes, and sample MAD relative to p50. Matching pooled medians do not prove +equal performance. Two process pairs do not justify confidence intervals. -The API 36 x86_64 emulator **must use KVM CPU acceleration**. The workflow grants -the ephemeral runner access to `/dev/kvm`, verifies acceleration before boot, and -launches with `-accel on`. Missing acceleration fails the job rather than silently -falling back to software CPU emulation. Software GPU rendering is separate and is -still used on the headless runner. +Performance is currently report-only. Build, execution and malformed-result +failures still fail CI. Turning observed differences into a regression gate needs +empirical validation on unchanged commits and intentional slowdowns on each +unchanged suite/testbed. No Promise case is permanently exempt. Scheduled/manual +runs with the same base and head SHA measure baseline variation explicitly. +Changed benchmark definitions require a new baseline and are not compared. -The initial bootstrap run accidentally used `-accel off`: boot alone took 8½ -minutes and six suites took another 32½ minutes. Those Android timings are not a -performance baseline. Accelerated results use a distinct `...-kvm` Bencher testbed. +## Artifacts and publishing -Host-side log lines show each suite's start, completion, measurement duration, -total wall time (including app installation/launch), and why a repeat pair ran. -These logs are outside the app's timed regions. Raw JSON, BMF, and comparison -Markdown are retained in the workflow artifacts for 30 days. -Both targets have a five-minute boot limit; individual device installation and -launch commands have a two-minute limit. Android app exits also retain logcat and -process-exit diagnostics, so native crashes do not disappear at emulator teardown. +The canonical artifact is `performance-report-`: raw JSON for every +base/head process plus `performance-report.json` with repository, revisions, +workflow run and attempt provenance. Artifacts remain available for 30 days. +The PR comment links its exact immutable artifact ID; downloads require GitHub +access. An agent can inspect the JSON instead of scraping the rendered table. -Same-repository PRs publish from a separate clean job after both device jobs -finish. This works before merge: it checks out an immutable, reviewed reporting -commit, downloads only result data, validates it against GitHub API metadata, -and rebuilds the table/BMF without installing or executing either app checkout. -Fork PRs never enter this privileged job; their `workflow_run` reporter becomes -active once its definition reaches the default branch. That reporter skips -same-repository PRs to avoid duplicate uploads. Both paths post the rebuilt -paired-comparison table as one updatable PR comment, independently of Bencher's -historical comparison. Stale results do -not overwrite a newer PR revision, and user-authored comments are never edited. -Revoke the previously exposed credential and replace the -repository's `BENCHER_KEY` secret before enabling publishing. PR jobs never receive -that secret in device/build jobs or any fork job. After rotation, set the repository variable `NITRO_BENCHER_ENABLED` -to `true` to enable Bencher uploads. The paired PR comment does not need that key. -Verdicts remain advisory during noise calibration. +One default-branch `Publish Nitro Performance` workflow handles internal PRs, +forks and main runs. It downloads the exact artifact from the triggering attempt, +validates bounded JSON against GitHub's run and current PR metadata, and computes +the comparison, Markdown and Bencher values from raw samples. It never installs +or executes PR code or app artifacts. Docs-only and cancelled runs skip +publication. Relevant failures remain failures. Stale PR results are skipped. -The pre-merge publisher requires a real base benchmark app. The infrastructure -PR's bootstrap A/A runs remain diagnostic artifacts, not Bencher baselines. -For paired PR reports, first upload the measured base to `baseline-` -on each testbed, then upload `pr-` with that exact baseline as its start -point. This works with an empty Bencher project and with stacked PRs without -pretending their base is `main`. Pushes/scheduled main runs record main history. -Both platform baselines are uploaded before either head. The publisher does not -reset the PR branch for each platform, preserving the other testbed's reports; -a changed base SHA naturally selects a different start point. -The Bencher action and downloaded CLI version are both pinned. Both publishers -verify the reviewed Linux CLI SHA-256 before the step receiving the API key. +The trusted publisher uses `BENCHER_KEY` as an Actions secret. Its CLI version and +binary digest are pinned. Bencher receives median latency values without invented +bounds; its JSON adapter requires only `value`. PR publications seed both measured +platform baselines at `baseline-` before recording the head at +`pr-`. Main runs record main history. User comments are never edited. -## Promoting performance verdicts to a gate +The raw-manifest publisher and producer land together in the first cleanup PR. +Until that PR reaches the default branch, the previous trusted reporter cannot +consume its new manifest. Merge that PR before running later stack revisions; +there is no permanent old-schema reporting path. Existing raw app results may +contain extra summary fields, which the raw parser ignores. + +## Android host requirements -This initial workflow always passes `--mode advisory`; merging it does **not** -enable performance enforcement. Collect at least 30 successful main/no-change -runs on each unchanged suite and testbed before a separate reviewed promotion. -Use only same-commit comparisons to estimate noise; ordinary base/head deltas -may include real code changes and must not inflate the noise allowance. +The API 36 x86_64 emulator requires KVM. CI checks `/dev/kvm`, verifies acceleration +before boot and uses `-accel on`; it must not silently use software CPU emulation. +Software GPU rendering is separate. Base/head measurements stay on that machine. +Android process failures retain logcat and process-exit diagnostics. Both platforms +bound boot time to five minutes and individual install/launch commands to two. -For each synchronous case, the promotion must set a per-case budget of -`max(5%, 1.5 × p95(abs(no-change delta)))`. Cases requiring over 10%, Promise -metrics, and inconclusive comparisons remain advisory. The current 5% table -threshold is provisional, not a calibrated gate. Do not simply change `--mode` -to `enforce`: the initial comparator uses that single provisional threshold, and -the trusted reporter deliberately rejects PR attempts to enable enforcement. -Promotion needs the reviewed per-case policy and matching reporter support. +The benchmark app shares real Nitro test packages without Harness/navigation UI. +Correctness remains in `apps/example`. See the [benchmark app README](../apps/benchmark/README.md) +for local build and run commands. diff --git a/.github/workflows/performance-report.yml b/.github/workflows/performance-report.yml index d03d6950e..0632084a1 100644 --- a/.github/workflows/performance-report.yml +++ b/.github/workflows/performance-report.yml @@ -13,11 +13,6 @@ permissions: jobs: publish: - # Same-repository PRs publish in a separate job of Nitro Performance, which - # also works before this workflow reaches the default branch. - if: >- - github.event.workflow_run.event != 'pull_request' || - github.event.workflow_run.head_repository.full_name != github.repository runs-on: ubuntu-24.04 steps: - name: Checkout trusted reporting code @@ -26,17 +21,24 @@ jobs: with: bun-version: 1.3.14 + - name: Select exact report artifact + id: select + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: bun scripts/performance/select-report.ts + - name: Download performance report id: download - continue-on-error: true + if: steps.select.outputs.artifact_id != '' uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7 with: - name: performance-report + artifact-ids: ${{ steps.select.outputs.artifact_id }} path: untrusted-artifact github-token: ${{ secrets.GITHUB_TOKEN }} run-id: ${{ github.event.workflow_run.id }} - name: Validate untrusted report + id: validate if: steps.download.outcome == 'success' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -57,27 +59,28 @@ jobs: --output-directory validated-report \ --expected-repository "$GITHUB_REPOSITORY" \ --trusted-workflow-event "$GITHUB_EVENT_PATH" \ + --artifact-id "${{ steps.select.outputs.artifact_id }}" \ "${TRUSTED_PR_ARGUMENTS[@]}" - name: Post paired comparison to the PR - if: steps.download.outcome == 'success' + if: steps.download.outcome == 'success' && steps.validate.outputs.stale != 'true' env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: bun scripts/performance/github-report.ts --directory validated-report - name: Install Bencher CLI - if: steps.download.outcome == 'success' && vars.NITRO_BENCHER_ENABLED == 'true' + if: steps.download.outcome == 'success' && steps.validate.outputs.stale != 'true' uses: bencherdev/bencher@8d75325c3bc59403a2186a056b472c4f49d42838 # v0.6.12 with: version: '0.6.12' - name: Verify pinned Bencher binary - if: steps.download.outcome == 'success' && vars.NITRO_BENCHER_ENABLED == 'true' + if: steps.download.outcome == 'success' && steps.validate.outputs.stale != 'true' run: | printf '%s %s\n' 'c2d3a6a7fae654246134e5ced1408bdb9ba4e198b0ac3b903af17a06574a7e08' "$(command -v bencher)" | sha256sum --check - - name: Publish to Bencher and GitHub - if: steps.download.outcome == 'success' && vars.NITRO_BENCHER_ENABLED == 'true' + if: steps.download.outcome == 'success' && steps.validate.outputs.stale != 'true' env: BENCHER_API_KEY: ${{ secrets.BENCHER_KEY }} BENCHER_PROJECT: nitro diff --git a/.github/workflows/performance.yml b/.github/workflows/performance.yml index 876a92eee..9f08aaa38 100644 --- a/.github/workflows/performance.yml +++ b/.github/workflows/performance.yml @@ -24,7 +24,6 @@ jobs: head_sha: ${{ steps.metadata.outputs.head_sha }} pr_number: ${{ steps.metadata.outputs.pr_number }} relevant: ${{ steps.metadata.outputs.relevant }} - base_benchmark_available: ${{ steps.metadata.outputs.base_benchmark_available }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: @@ -69,14 +68,14 @@ jobs: RELEVANT=false fi - echo "base_sha=$BASE_SHA" >> "$GITHUB_OUTPUT" - echo "head_sha=$HEAD_SHA" >> "$GITHUB_OUTPUT" - echo "pr_number=$NUMBER" >> "$GITHUB_OUTPUT" - echo "relevant=$RELEVANT" >> "$GITHUB_OUTPUT" - if git cat-file -e "$BASE_SHA:apps/benchmark/package.json" 2>/dev/null; then - echo 'base_benchmark_available=true' >> "$GITHUB_OUTPUT" - else - echo 'base_benchmark_available=false' >> "$GITHUB_OUTPUT" + { + echo "base_sha=$BASE_SHA" + echo "head_sha=$HEAD_SHA" + echo "pr_number=$NUMBER" + echo "relevant=$RELEVANT" + } >> "$GITHUB_OUTPUT" + if [[ "$RELEVANT" == 'false' ]]; then + echo 'No performance-sensitive files changed.' >> "$GITHUB_STEP_SUMMARY" fi - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 @@ -132,14 +131,8 @@ jobs: stat --format='%A %U:%G %n' /dev/kvm test -r /dev/kvm && test -w /dev/kvm - - name: Select trusted controller - run: | - if [[ -f base/scripts/performance/run-sequence.ts ]]; then - echo "PERF_CONTROLLER_ROOT=$GITHUB_WORKSPACE/base" >> "$GITHUB_ENV" - else - echo "PERF_CONTROLLER_ROOT=$GITHUB_WORKSPACE/head" >> "$GITHUB_ENV" - echo "PERF_BOOTSTRAP=true" >> "$GITHUB_ENV" - fi + - name: Select measurement controller + run: echo "PERF_CONTROLLER_ROOT=$GITHUB_WORKSPACE/head" >> "$GITHUB_ENV" - name: Build head benchmark APK working-directory: head @@ -149,7 +142,6 @@ jobs: ./gradlew :app:assembleRelease --no-daemon --no-build-cache -PreactNativeArchitectures=x86_64 - name: Build base benchmark APK - if: env.PERF_BOOTSTRAP != 'true' working-directory: base run: | bun install --frozen-lockfile @@ -158,13 +150,8 @@ jobs: - name: Select base artifact run: | - if [[ "$PERF_BOOTSTRAP" == "true" ]]; then - echo "BASE_APP=$GITHUB_WORKSPACE/head/apps/benchmark/android/app/build/outputs/apk/release/app-release.apk" >> "$GITHUB_ENV" - echo "BASE_ROOT=$GITHUB_WORKSPACE/head" >> "$GITHUB_ENV" - else - echo "BASE_APP=$GITHUB_WORKSPACE/base/apps/benchmark/android/app/build/outputs/apk/release/app-release.apk" >> "$GITHUB_ENV" - echo "BASE_ROOT=$GITHUB_WORKSPACE/base" >> "$GITHUB_ENV" - fi + echo "BASE_APP=$GITHUB_WORKSPACE/base/apps/benchmark/android/app/build/outputs/apk/release/app-release.apk" >> "$GITHUB_ENV" + echo "BASE_ROOT=$GITHUB_WORKSPACE/base" >> "$GITHUB_ENV" - name: Run paired Android benchmarks uses: reactivecircus/android-emulator-runner@4c44018e59b437e86cdfc41da381398f93ed8808 # v2 @@ -194,7 +181,6 @@ jobs: --os-version "Android 16 / API 36" --architecture x86_64 --toolchain "JDK 17 / NDK 29.0.14206865 / KVM" - --mode advisory - name: Upload Android results if: always() @@ -205,78 +191,6 @@ jobs: if-no-files-found: warn retention-days: 30 - # Same-repository contributors already have access to repository workflows. - # Forks never enter this job; their reports use the default-branch workflow_run. - # Keep reporting on a fresh runner with reviewed code, not either app checkout. - publish-pr: - name: Publish performance to PR and Bencher - needs: [prepare, nitro-performance] - if: >- - github.event_name == 'pull_request' && - github.event.pull_request.head.repo.full_name == github.repository && - needs.prepare.outputs.base_benchmark_available == 'true' && - needs.nitro-performance.result == 'success' - runs-on: ubuntu-24.04 - timeout-minutes: 10 - permissions: - actions: read - contents: read - checks: write - pull-requests: write - steps: - - name: Checkout pinned reporting code - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 - with: - repository: margelo/nitro - ref: f5857e95af6c07a67a6604c44ecbfc6bc365e234 - persist-credentials: false - - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 - with: - bun-version: 1.3.14 - - name: Download completed benchmark data - uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7 - with: - name: performance-report - path: untrusted-artifact - - name: Validate against GitHub metadata and rebuild report - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_NUMBER: ${{ github.event.pull_request.number }} - run: | - set -euo pipefail - gh api "repos/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" \ - --jq '{repository: .repository, workflow_run: .}' > trusted-workflow-event.json - gh api "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER" > trusted-pull-request.json - bun scripts/performance/validate-report.ts \ - --artifact-directory untrusted-artifact \ - --output-directory validated-report \ - --expected-repository "$GITHUB_REPOSITORY" \ - --trusted-workflow-event trusted-workflow-event.json \ - --trusted-pull-request trusted-pull-request.json - - name: Post paired comparison to the PR - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: bun scripts/performance/github-report.ts --directory validated-report - - name: Install pinned Bencher CLI - if: vars.NITRO_BENCHER_ENABLED == 'true' - uses: bencherdev/bencher@8d75325c3bc59403a2186a056b472c4f49d42838 # v0.6.12 - with: - version: '0.6.12' - - name: Verify pinned Bencher binary - if: vars.NITRO_BENCHER_ENABLED == 'true' - run: | - printf '%s %s\n' 'c2d3a6a7fae654246134e5ced1408bdb9ba4e198b0ac3b903af17a06574a7e08' "$(command -v bencher)" | sha256sum --check - - - name: Publish to Bencher and GitHub - if: vars.NITRO_BENCHER_ENABLED == 'true' - env: - BENCHER_API_KEY: ${{ secrets.BENCHER_KEY }} - BENCHER_PROJECT: nitro - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: bun scripts/performance/publish.ts --directory validated-report - - name: Explain disabled Bencher publishing - if: vars.NITRO_BENCHER_ENABLED != 'true' - run: echo 'Bencher uploads are disabled until the exposed key is rotated and NITRO_BENCHER_ENABLED=true. The paired PR comment is still published.' >> "$GITHUB_STEP_SUMMARY" - ios: needs: prepare if: needs.prepare.outputs.relevant == 'true' @@ -308,14 +222,8 @@ jobs: - name: Select Xcode 26.5 run: sudo xcode-select -s /Applications/Xcode_26.5.app/Contents/Developer - - name: Select trusted controller - run: | - if [[ -f base/scripts/performance/run-sequence.ts ]]; then - echo "PERF_CONTROLLER_ROOT=$GITHUB_WORKSPACE/base" >> "$GITHUB_ENV" - else - echo "PERF_CONTROLLER_ROOT=$GITHUB_WORKSPACE/head" >> "$GITHUB_ENV" - echo "PERF_BOOTSTRAP=true" >> "$GITHUB_ENV" - fi + - name: Select measurement controller + run: echo "PERF_CONTROLLER_ROOT=$GITHUB_WORKSPACE/head" >> "$GITHUB_ENV" - name: Build head benchmark app working-directory: head @@ -340,7 +248,6 @@ jobs: build - name: Build base benchmark app - if: env.PERF_BOOTSTRAP != 'true' working-directory: base run: | bun install --frozen-lockfile @@ -364,13 +271,8 @@ jobs: - name: Select base artifact run: | - if [[ "$PERF_BOOTSTRAP" == "true" ]]; then - echo "BASE_APP=$GITHUB_WORKSPACE/head/apps/benchmark/ios/build-benchmark/Build/Products/Release-iphonesimulator/NitroBenchmark.app" >> "$GITHUB_ENV" - echo "BASE_ROOT=$GITHUB_WORKSPACE/head" >> "$GITHUB_ENV" - else - echo "BASE_APP=$GITHUB_WORKSPACE/base/apps/benchmark/ios/build-benchmark/Build/Products/Release-iphonesimulator/NitroBenchmark.app" >> "$GITHUB_ENV" - echo "BASE_ROOT=$GITHUB_WORKSPACE/base" >> "$GITHUB_ENV" - fi + echo "BASE_APP=$GITHUB_WORKSPACE/base/apps/benchmark/ios/build-benchmark/Build/Products/Release-iphonesimulator/NitroBenchmark.app" >> "$GITHUB_ENV" + echo "BASE_ROOT=$GITHUB_WORKSPACE/base" >> "$GITHUB_ENV" - name: Run paired iOS benchmarks run: | @@ -400,8 +302,7 @@ jobs: --device 'iPhone 17 Pro simulator' \ --os-version 'iOS 26.5' \ --architecture arm64 \ - --toolchain 'Xcode 26.5' \ - --mode advisory + --toolchain 'Xcode 26.5' - name: Delete simulator if: always() && env.DEVICE_ID != '' @@ -421,20 +322,13 @@ jobs: nitro-performance: name: nitro-performance needs: [prepare, android, ios] - if: always() + if: always() && !cancelled() && needs.prepare.outputs.relevant == 'true' runs-on: ubuntu-24.04 steps: - name: Require successful preparation if: needs.prepare.result != 'success' run: exit 1 - - name: Report not applicable - if: needs.prepare.outputs.relevant != 'true' - run: | - echo '## Nitro performance' >> "$GITHUB_STEP_SUMMARY" - echo '' >> "$GITHUB_STEP_SUMMARY" - echo 'No performance-sensitive files changed.' >> "$GITHUB_STEP_SUMMARY" - - name: Require successful platform runs if: needs.prepare.outputs.relevant == 'true' env: @@ -477,24 +371,20 @@ jobs: run: | mkdir -p performance-report/raw/android performance-report/raw/ios bun scripts/performance/report.ts \ - --comparison artifacts/android/comparison-android.json \ - --comparison artifacts/ios/comparison-ios.json \ --output performance-report/performance-report.json \ - --markdown-output performance-report/performance-summary.md \ --repository "$REPOSITORY" \ --event-name "${{ github.event_name }}" \ - --pull-request "$PR_NUMBER" - cp artifacts/android/bencher-android.json performance-report/ - cp artifacts/ios/bencher-ios.json performance-report/ + --pull-request "$PR_NUMBER" \ + --base-sha "${{ needs.prepare.outputs.base_sha }}" \ + --head-sha "${{ needs.prepare.outputs.head_sha }}" cp artifacts/android/base-*.json artifacts/android/head-*.json performance-report/raw/android/ cp artifacts/ios/base-*.json artifacts/ios/head-*.json performance-report/raw/ios/ - cat performance-report/performance-summary.md >> "$GITHUB_STEP_SUMMARY" - name: Upload aggregate report if: needs.prepare.outputs.relevant == 'true' uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 with: - name: performance-report + name: performance-report-${{ github.run_attempt }} path: performance-report if-no-files-found: error retention-days: 30 diff --git a/apps/benchmark/README.md b/apps/benchmark/README.md index ca942157a..53b775364 100644 --- a/apps/benchmark/README.md +++ b/apps/benchmark/README.md @@ -59,7 +59,7 @@ For iOS, use `--platform ios`, a simulator UDID for `--device-id`, and the built `NitroBenchmark.app` for `--app`, with matching simulator/toolchain metadata. Local runs do not upload results. -Each of the 40 metrics targets 150 ms of timed work per sample (roughly +Each metric targets 150 ms of timed work per sample (roughly 100–200 ms), using round iteration counts with two significant digits, such as 1,500,000 or 24,000. Calibration can grow or shrink the count and is rechecked after five warmup batches. That count is then frozen for twenty measured samples; @@ -83,11 +83,10 @@ the timed batch. Operation-induced allocations remain inside it. ## CI and reporting -See [performance CI](../../.github/PERFORMANCE.md) for the paired comparison, -noise calibration, artifacts, fork-safe reporting, and Bencher activation. -The initial infrastructure PR uses the head binary for both sides for A/A -validation because its base does not yet contain this app. Subsequent PRs build -base and head independently. Performance verdicts remain advisory. +See [performance CI](../../.github/PERFORMANCE.md) for observed comparisons, +process variability, raw artifacts, trusted reporting, and Bencher publishing. +Base and head are built independently. Same-revision scheduled/manual runs show +baseline variation. Performance remains report-only. The example's former benchmark screen and TurboModule control have moved here. No public Nitro API changes are needed. App dependency versions initially match diff --git a/apps/benchmark/src/benchmarks/runner.ts b/apps/benchmark/src/benchmarks/runner.ts index 169f7ec88..f215963e8 100644 --- a/apps/benchmark/src/benchmarks/runner.ts +++ b/apps/benchmark/src/benchmarks/runner.ts @@ -1,10 +1,4 @@ -import { - bootstrapMedianConfidenceInterval, - median, - medianAbsoluteDeviation, - quantile, - robustCoefficientOfVariationPercent, -} from './statistics' +import { median } from './statistics' import { calibrateIterations, roundIterations } from './calibration' import { benchmarkRuntime, executeBatch, type BenchmarkRuntime } from './batch' import type { @@ -72,28 +66,17 @@ export async function runBenchmarkDefinitions( version: definition.version, family: definition.family, implementation: definition.implementation, - advisory: definition.advisory ?? false, iterations, chunkIterations: Math.min( iterations, definition.maxChunkIterations ?? iterations ), samplesNsPerOp, - medianNsPerOp: median(samplesNsPerOp), - p95NsPerOp: quantile(samplesNsPerOp, 0.95), - medianAbsoluteDeviationNsPerOp: medianAbsoluteDeviation(samplesNsPerOp), - robustCoefficientOfVariationPercent: - robustCoefficientOfVariationPercent(samplesNsPerOp), - medianConfidenceInterval95: bootstrapMedianConfidenceInterval( - samplesNsPerOp, - 2_000, - definition.id - ), checksum, } metrics.push(metric) console.info( - `[NitroBenchmark] ${metric.id}: ${metric.medianNsPerOp.toFixed(2)} ns/op; ${iterations} ops/sample, chunks of ${metric.chunkIterations}, median timed batch ${((metric.medianNsPerOp * iterations) / 1_000_000).toFixed(1)} ms` + `[NitroBenchmark] ${metric.id}: ${median(metric.samplesNsPerOp).toFixed(2)} ns/op; ${iterations} ops/sample, chunks of ${metric.chunkIterations}, median timed batch ${((median(metric.samplesNsPerOp) * iterations) / 1_000_000).toFixed(1)} ms` ) } diff --git a/apps/benchmark/src/benchmarks/statistics.ts b/apps/benchmark/src/benchmarks/statistics.ts index 13b9488ce..7ca6cf3c3 100644 --- a/apps/benchmark/src/benchmarks/statistics.ts +++ b/apps/benchmark/src/benchmarks/statistics.ts @@ -1,102 +1,13 @@ -function sorted(values: readonly number[]): number[] { - return [...values].sort((left, right) => left - right) -} - -export function quantile( - values: readonly number[], - percentile: number -): number { - if (values.length === 0) { - throw new Error('Cannot calculate a quantile for an empty sample.') - } - if (percentile < 0 || percentile > 1) { - throw new Error(`Percentile must be between 0 and 1, got ${percentile}.`) - } - - const ordered = sorted(values) - const position = (ordered.length - 1) * percentile - const lowerIndex = Math.floor(position) - const upperIndex = Math.ceil(position) - const lower = ordered[lowerIndex]! - const upper = ordered[upperIndex]! - return lower + (upper - lower) * (position - lowerIndex) -} - export function median(values: readonly number[]): number { - return quantile(values, 0.5) + if (values.length === 0) throw new Error('Cannot calculate an empty median.') + const ordered = [...values].sort((left, right) => left - right) + const middle = Math.floor(ordered.length / 2) + return ordered.length % 2 === 0 + ? (ordered[middle - 1]! + ordered[middle]!) / 2 + : ordered[middle]! } export function medianAbsoluteDeviation(values: readonly number[]): number { const center = median(values) return median(values.map((value) => Math.abs(value - center))) } - -export function robustCoefficientOfVariationPercent( - values: readonly number[] -): number { - const center = median(values) - if (center === 0) return 0 - return (1.4826 * medianAbsoluteDeviation(values) * 100) / center -} - -function hashSeed(seed: string): number { - let value = 17 - for (let index = 0; index < seed.length; index++) { - value = (value * 31 + seed.charCodeAt(index)) % 2_147_483_647 - } - return value -} - -function createRandom(seed: string): () => number { - let state = hashSeed(seed) || 1 - return () => { - state = (state * 48_271) % 2_147_483_647 - return state / 2_147_483_647 - } -} - -function resampleMedian( - values: readonly number[], - random: () => number -): number { - const sample = new Array(values.length) - for (let index = 0; index < values.length; index++) { - sample[index] = values[Math.floor(random() * values.length)]! - } - return median(sample) -} - -export function bootstrapMedianConfidenceInterval( - values: readonly number[], - iterations: number, - seed: string -): [number, number] { - if (iterations < 1) { - throw new Error('Bootstrap iterations must be positive.') - } - const random = createRandom(seed) - const medians = new Array(iterations) - for (let index = 0; index < iterations; index++) { - medians[index] = resampleMedian(values, random) - } - return [quantile(medians, 0.025), quantile(medians, 0.975)] -} - -export function bootstrapPercentChangeConfidenceInterval( - base: readonly number[], - head: readonly number[], - iterations: number, - seed: string -): [number, number] { - if (base.length === 0 || head.length === 0) { - throw new Error('Both base and head samples are required.') - } - const random = createRandom(seed) - const changes = new Array(iterations) - for (let index = 0; index < iterations; index++) { - const baseMedian = resampleMedian(base, random) - const headMedian = resampleMedian(head, random) - changes[index] = (headMedian / baseMedian - 1) * 100 - } - return [quantile(changes, 0.025), quantile(changes, 0.975)] -} diff --git a/apps/benchmark/src/benchmarks/suite.ts b/apps/benchmark/src/benchmarks/suite.ts index 132b1297e..f9affed77 100644 --- a/apps/benchmark/src/benchmarks/suite.ts +++ b/apps/benchmark/src/benchmarks/suite.ts @@ -324,7 +324,6 @@ function createObjectBenchmarks( family: 'promise', implementation, kind: 'async', - advisory: true, // Release fulfilled Promise chains between chunks, outside the timer. maxChunkIterations: 5_000, collectNativeGarbage: diff --git a/apps/benchmark/src/benchmarks/types.ts b/apps/benchmark/src/benchmarks/types.ts index 281185928..89214e7f3 100644 --- a/apps/benchmark/src/benchmarks/types.ts +++ b/apps/benchmark/src/benchmarks/types.ts @@ -24,7 +24,6 @@ interface BenchmarkDefinitionBase { version: number family: BenchmarkFamily implementation: BenchmarkImplementation - advisory?: boolean initialIterations?: number maxIterations?: number /** Bound live allocations, not the total operations in a measured sample. */ @@ -60,16 +59,10 @@ export interface BenchmarkMetric { version: number family: BenchmarkFamily implementation: BenchmarkImplementation - advisory: boolean iterations: number /** Maximum operations between untimed garbage collections. */ chunkIterations: number samplesNsPerOp: number[] - medianNsPerOp: number - p95NsPerOp: number - medianAbsoluteDeviationNsPerOp: number - robustCoefficientOfVariationPercent: number - medianConfidenceInterval95: [number, number] checksum: number } diff --git a/scripts/performance/compare.ts b/scripts/performance/compare.ts index 9eef94917..580d2278c 100644 --- a/scripts/performance/compare.ts +++ b/scripts/performance/compare.ts @@ -1,9 +1,6 @@ +import { renderPerformanceReportMarkdown } from './report-markdown' import { readFile } from 'node:fs/promises' -import { - compareRuns, - renderPlatformMarkdown, - toBencherMetricFormat, -} from './comparison' +import { compareRuns, toBencherMetricFormat } from './comparison' import { parseArguments, repeatedArgument, requiredArgument } from './args' import { validateBenchmarkRun } from './schema' @@ -13,7 +10,6 @@ const headFiles = repeatedArgument(argumentsMap, 'head') const output = requiredArgument(argumentsMap, 'output') const markdownOutput = requiredArgument(argumentsMap, 'markdown-output') const bencherOutput = requiredArgument(argumentsMap, 'bencher-output') -const advisoryMode = argumentsMap.get('mode')?.[0] !== 'enforce' async function readRun(file: string) { return validateBenchmarkRun(JSON.parse(await readFile(file, 'utf8'))) @@ -21,15 +17,20 @@ async function readRun(file: string) { const baseRuns = await Promise.all(baseFiles.map(readRun)) const headRuns = await Promise.all(headFiles.map(readRun)) -const comparison = compareRuns(baseRuns, headRuns, advisoryMode) +const comparison = compareRuns(baseRuns, headRuns) await Promise.all([ Bun.write(output, `${JSON.stringify(comparison, null, 2)}\n`), - Bun.write(markdownOutput, `${renderPlatformMarkdown(comparison)}\n`), + Bun.write( + markdownOutput, + renderPerformanceReportMarkdown([comparison], { + repository: requiredArgument(argumentsMap, 'repository'), + baseSha: comparison.baseSha, + headSha: comparison.headSha, + }) + ), Bun.write( bencherOutput, `${JSON.stringify(toBencherMetricFormat(headRuns), null, 2)}\n` ), ]) - -if (!advisoryMode && comparison.hasRegression) process.exitCode = 1 diff --git a/scripts/performance/comparison.test.ts b/scripts/performance/comparison.test.ts index a6d144c2e..69b3ea861 100644 --- a/scripts/performance/comparison.test.ts +++ b/scripts/performance/comparison.test.ts @@ -14,6 +14,7 @@ function run( return { schemaVersion: 1, suiteVersion: 1, + benchmarkCount: 1, configuration: { runId: `run-${sha[0]}`, reverse: false, @@ -44,18 +45,9 @@ function run( version: 1, family: 'primitive', implementation: 'nitro-cpp', - advisory: false, iterations: 10_000, chunkIterations: 10_000, samplesNsPerOp: samples, - medianNsPerOp: samples[2]!, - p95NsPerOp: Math.max(...samples), - medianAbsoluteDeviationNsPerOp: 1, - robustCoefficientOfVariationPercent: 1, - medianConfidenceInterval95: [ - Math.min(...samples), - Math.max(...samples), - ], checksum: 42, }, ], @@ -63,141 +55,49 @@ function run( } describe('performance comparison', () => { - test('validates chunk sizes and accepts older single-chunk results', () => { - const result = run(BASE_SHA, [99, 100, 100, 101, 100]) - result.metrics[0]!.chunkIterations = 1_000 - expect(validateBenchmarkRun(result).metrics[0]!.chunkIterations).toBe(1_000) - const legacy = JSON.parse(JSON.stringify(result)) - delete legacy.metrics[0].chunkIterations - expect(validateBenchmarkRun(legacy).metrics[0]!.chunkIterations).toBe( - 10_000 - ) - for (const invalid of [0, -1, 1.5, 10_001]) { - result.metrics[0]!.chunkIterations = invalid - expect(() => validateBenchmarkRun(result)).toThrow() + test('requires explicit bounded chunk counts and Release Hermes', () => { + const result = run(BASE_SHA, [100, 100]) + for (const chunk of [0, -1, 1.5, 10_001, undefined]) { + expect(() => + validateBenchmarkRun({ + ...result, + metrics: [{ ...result.metrics[0], chunkIterations: chunk }], + }) + ).toThrow() } + result.environment.dev = true + expect(() => validateBenchmarkRun(result)).toThrow('production Hermes') }) - test('keeps an A/A run unchanged', () => { - const samples = [99, 100, 100, 101, 100] - const comparison = compareRuns( - [run(BASE_SHA, samples)], - [run(HEAD_SHA, samples)], - true + test('retains process disagreement even when pooled medians match', () => { + const result = compareRuns( + [run(BASE_SHA, [80, 80]), run(BASE_SHA, [120, 120])], + [run(HEAD_SHA, [120, 120]), run(HEAD_SHA, [80, 80])] ) - expect(comparison.comparisons[0]?.verdict).toBe('unchanged') - expect(comparison.hasRegression).toBe(false) + expect(result.comparisons[0]?.deltaPercent).toBe(0) + expect(result.comparisons[0]?.baseProcessMedians).toEqual([80, 120]) + expect(result.comparisons[0]?.pairChangesPercent[0]).toBe(50) + expect(result.comparisons[0]?.pairChangesPercent[1]).toBeCloseTo(-33.3333) }) - test('detects regressions and improvements', () => { - const base = [run(BASE_SHA, [99, 100, 100, 101, 100])] - const regression = compareRuns( - base, - [run(HEAD_SHA, [114, 115, 115, 116, 115])], - true - ) - expect(regression.comparisons[0]?.verdict).toBe('regression') - - const improvement = compareRuns( - base, - [run(HEAD_SHA, [84, 85, 85, 86, 85])], - true + test('reports observed changes without inventing confidence bounds', () => { + const result = compareRuns( + [run(BASE_SHA, [99, 100, 101])], + [run(HEAD_SHA, [119, 120, 121])] ) - expect(improvement.comparisons[0]?.verdict).toBe('improvement') + expect(result.comparisons[0]?.deltaPercent).toBeCloseTo(20) + expect(result.comparisons[0]?.baseMadPercent).toBe(1) + expect(toBencherMetricFormat([run(HEAD_SHA, [119, 120, 121])])).toEqual({ + 'nitro-cpp/primitive/add-numbers': { latency: { value: 120 } }, + }) }) - test('does not mistake process-level A/A drift for independent evidence', () => { - // Rounded run medians from the accelerated Android bootstrap A/A run. - const samples = (value: number) => - Array.from({ length: 20 }, (_, index) => value + (index % 3) - 1) - const comparison = compareRuns( - [540, 527, 523].map((value) => run(BASE_SHA, samples(value))), - [529, 579, 556].map((value) => run(HEAD_SHA, samples(value))), - true - ) - expect(comparison.hasRegression).toBe(false) - expect(comparison.comparisons[0]?.verdict).toBe('inconclusive') + test('does not compare changed benchmark definitions', () => { expect( - comparison.comparisons[0]?.deltaConfidenceInterval95[0] - ).toBeLessThan(0) - expect(comparison.rerunRecommended).toBe(true) - }) - - test('detects a consistent 15% change across independent process pairs', () => { - for (const scale of [0.85, 1.15]) { - const samples = [99, 100, 100, 101, 100] - const comparison = compareRuns( - [1, 2, 3].map(() => run(BASE_SHA, samples)), - [1, 2, 3].map(() => - run( - HEAD_SHA, - samples.map((n) => n * scale) - ) - ), - true - ) - expect(comparison.comparisons[0]?.verdict).toBe( - scale > 1 ? 'regression' : 'improvement' - ) - } - }) - - test('does not let high CV hide a change whose paired interval is decisive', () => { - const centers = [80, 100, 120] - const base = centers.map((center) => - run(BASE_SHA, [center - 1, center, center, center + 1, center]) - ) - const head = centers.map((center) => - run(HEAD_SHA, [ - center * 2 - 2, - center * 2, - center * 2, - center * 2 + 2, - center * 2, - ]) - ) - const comparison = compareRuns(base, head, true) - const metric = comparison.comparisons[0]! - expect(metric.baseRobustCvPercent).toBeGreaterThan(5) - expect(metric.headRobustCvPercent).toBeGreaterThan(5) - expect(metric.deltaConfidenceInterval95[0]).toBeGreaterThan(0) - expect(metric.verdict).toBe('regression') - }) - - test('marks changed suites for rebaseline', () => { - const comparison = compareRuns( - [run(BASE_SHA, [99, 100, 100, 101, 100], 'c'.repeat(64))], - [run(HEAD_SHA, [99, 100, 100, 101, 100], 'd'.repeat(64))], - true - ) - expect(comparison.suiteComparable).toBe(false) - expect(comparison.comparisons).toEqual([]) - }) - - test('validates release metadata and converts BMF', () => { - const result = validateBenchmarkRun(run(HEAD_SHA, [99, 100, 100, 101, 100])) - const bmf = toBencherMetricFormat([result]) - expect(bmf['nitro-cpp/primitive/add-numbers']?.latency.value).toBe(100) - }) - - test('includes between-run variation in Bencher confidence bounds', () => { - const bmf = toBencherMetricFormat( - [100, 200, 300].map((value) => - run( - HEAD_SHA, - Array.from({ length: 20 }, () => value) - ) - ) - ) - const latency = bmf['nitro-cpp/primitive/add-numbers']!.latency - expect(latency.value).toBe(200) - expect(latency.lower_value).toBe(100) - expect(latency.upper_value).toBe(300) - }) - - test('rejects a non-release result', () => { - const invalid = run(HEAD_SHA, [99, 100, 100, 101, 100]) - invalid.environment.dev = true - expect(() => validateBenchmarkRun(invalid)).toThrow('production Hermes') + compareRuns( + [run(BASE_SHA, [100])], + [run(HEAD_SHA, [100], 'd'.repeat(64))] + ).suiteComparable + ).toBe(false) }) }) diff --git a/scripts/performance/comparison.ts b/scripts/performance/comparison.ts index 77ffbdbbe..a01b8de99 100644 --- a/scripts/performance/comparison.ts +++ b/scripts/performance/comparison.ts @@ -1,60 +1,36 @@ import { median, - robustCoefficientOfVariationPercent, + medianAbsoluteDeviation, } from '../../apps/benchmark/src/benchmarks/statistics' import type { BenchmarkMetric, BenchmarkRunResult, } from '../../apps/benchmark/src/benchmarks/types' -import { - bootstrapPairedRunChange, - bootstrapRunMedian, -} from './paired-bootstrap' -export type MetricVerdict = - | 'improvement' - | 'regression' - | 'unchanged' - | 'inconclusive' - | 'advisory' +/** A presentation threshold, not a calibrated regression budget. */ +export const REPORTING_THRESHOLD_PERCENT = 5 export interface MetricComparison { id: string - advisory: boolean baseMedianNsPerOp: number headMedianNsPerOp: number deltaPercent: number - deltaConfidenceInterval95: [number, number] - baseRobustCvPercent: number - headRobustCvPercent: number - budgetPercent: number - verdict: MetricVerdict + baseProcessMedians: number[] + headProcessMedians: number[] + baseMadPercent: number + headMadPercent: number + pairChangesPercent: number[] } export interface PlatformComparison { - schemaVersion: 1 platform: 'android' | 'ios' baseSha: string headSha: string - baseSuiteHash: string - headSuiteHash: string suiteComparable: boolean - advisoryMode: boolean - rerunRecommended: boolean - hasRegression: boolean comparisons: MetricComparison[] } -export type BencherMetricFormat = Record< - string, - { - latency: { - value: number - lower_value: number - upper_value: number - } - } -> +export type BencherMetricFormat = Record function indexMetrics( runs: readonly BenchmarkRunResult[] @@ -68,225 +44,117 @@ function indexMetrics( return index } -function samples(metrics: readonly BenchmarkMetric[]): number[] { - return metrics.flatMap((metric) => metric.samplesNsPerOp) -} - -function verdictFor( - advisory: boolean, - deltaPercent: number, - confidenceInterval: readonly [number, number], - baseCv: number, - headCv: number, - budgetPercent: number -): MetricVerdict { - if (advisory) return 'advisory' - if (deltaPercent >= budgetPercent && confidenceInterval[0] > 0) { - return 'regression' - } - if (deltaPercent <= -budgetPercent && confidenceInterval[1] < 0) { - return 'improvement' - } - if (Math.abs(deltaPercent) >= budgetPercent || baseCv > 5 || headCv > 5) - return 'inconclusive' - return 'unchanged' -} - export function compareRuns( baseRuns: readonly BenchmarkRunResult[], - headRuns: readonly BenchmarkRunResult[], - advisoryMode: boolean, - budgetPercent = 5 + headRuns: readonly BenchmarkRunResult[] ): PlatformComparison { - if (baseRuns.length === 0 || headRuns.length === 0) { - throw new Error('At least one base and one head run are required.') - } - if (baseRuns.length !== headRuns.length) { + if (baseRuns.length === 0 || baseRuns.length !== headRuns.length) { throw new Error('A matching base run is required for every head run.') } - const platform = baseRuns[0]!.configuration.platform - const baseSha = baseRuns[0]!.configuration.commitSha + const { platform, commitSha: baseSha, suiteHash } = baseRuns[0]!.configuration const headSha = headRuns[0]!.configuration.commitSha - for (const run of [...baseRuns, ...headRuns]) { - if (run.configuration.platform !== platform) { - throw new Error('Cannot compare results from different platforms.') - } - } - for (const run of baseRuns) { - if (run.configuration.commitSha !== baseSha) { - throw new Error('Base runs have different commit SHAs.') + for (const [runs, sha] of [ + [baseRuns, baseSha], + [headRuns, headSha], + ] as const) { + if ( + runs.some( + (run) => + run.configuration.platform !== platform || + run.configuration.commitSha !== sha + ) + ) { + throw new Error('Process runs have different platforms or commit SHAs.') } } - for (const run of headRuns) { - if (run.configuration.commitSha !== headSha) { - throw new Error('Head runs have different commit SHAs.') + const first = baseRuns[0]! + for (const run of [...baseRuns, ...headRuns]) { + for (const key of [ + 'device', + 'osVersion', + 'architecture', + 'toolchain', + ] as const) { + if (run.configuration[key] !== first.configuration[key]) + throw new Error(`Process runs have different ${key} settings.`) } - } - - const baseSuiteHash = baseRuns[0]!.configuration.suiteHash - const headSuiteHash = headRuns[0]!.configuration.suiteHash - const suiteComparable = - baseRuns.every((run) => run.configuration.suiteHash === baseSuiteHash) && - headRuns.every((run) => run.configuration.suiteHash === headSuiteHash) && - baseSuiteHash === headSuiteHash - - const comparisons: MetricComparison[] = [] - if (suiteComparable) { - const baseMetrics = indexMetrics(baseRuns) - const headMetrics = indexMetrics(headRuns) if ( - baseMetrics.size !== headMetrics.size || - [...baseMetrics.keys()].some((id) => !headMetrics.has(id)) + JSON.stringify(run.runner) !== JSON.stringify(first.runner) || + JSON.stringify(run.environment) !== JSON.stringify(first.environment) ) { - throw new Error('Base and head expose different benchmark IDs.') - } - - for (const id of [...baseMetrics.keys()].sort()) { - const base = baseMetrics.get(id)! - const head = headMetrics.get(id)! - const version = base[0]!.version - if (base.length !== baseRuns.length || head.length !== headRuns.length) { - throw new Error(`Benchmark ${id} is missing from a process run.`) - } - if ( - base.some((metric) => metric.version !== version) || - head.some((metric) => metric.version !== version) - ) { - throw new Error(`Benchmark version mismatch for ${id}.`) - } - const baseSamples = samples(base) - const headSamples = samples(head) - const baseMedian = median(baseSamples) - const headMedian = median(headSamples) - const deltaPercent = (headMedian / baseMedian - 1) * 100 - const interval = bootstrapPairedRunChange( - base.map((metric) => metric.samplesNsPerOp), - head.map((metric) => metric.samplesNsPerOp), - 10_000, - `${platform}:${id}:${baseSha}:${headSha}` - ) - const baseCv = Math.max( - robustCoefficientOfVariationPercent(baseSamples), - robustCoefficientOfVariationPercent( - base.map((metric) => median(metric.samplesNsPerOp)) - ) - ) - const headCv = Math.max( - robustCoefficientOfVariationPercent(headSamples), - robustCoefficientOfVariationPercent( - head.map((metric) => median(metric.samplesNsPerOp)) - ) - ) - const advisory = base[0]!.advisory || head[0]!.advisory - comparisons.push({ - id, - advisory, - baseMedianNsPerOp: baseMedian, - headMedianNsPerOp: headMedian, - deltaPercent, - deltaConfidenceInterval95: interval, - baseRobustCvPercent: baseCv, - headRobustCvPercent: headCv, - budgetPercent, - verdict: verdictFor( - advisory, - deltaPercent, - interval, - baseCv, - headCv, - budgetPercent - ), - }) + throw new Error('Process runs have different runtime settings.') } } - - const hasRegression = comparisons.some( - (comparison) => comparison.verdict === 'regression' + const suiteComparable = [...baseRuns, ...headRuns].every( + (run) => run.configuration.suiteHash === suiteHash ) - return { - schemaVersion: 1, + const comparison: PlatformComparison = { platform, baseSha, headSha, - baseSuiteHash, - headSuiteHash, suiteComparable, - advisoryMode, - rerunRecommended: comparisons.some( - (comparison) => comparison.verdict === 'inconclusive' - ), - hasRegression, - comparisons, + comparisons: [], + } + if (!suiteComparable) return comparison + + const baseMetrics = indexMetrics(baseRuns) + const headMetrics = indexMetrics(headRuns) + if ( + baseMetrics.size !== headMetrics.size || + [...baseMetrics.keys()].some((id) => !headMetrics.has(id)) + ) { + throw new Error('Base and head expose different benchmark IDs.') } + for (const id of [...baseMetrics.keys()].sort()) { + const base = baseMetrics.get(id)! + const head = headMetrics.get(id)! + if ( + base.length !== baseRuns.length || + head.length !== headRuns.length || + [...base, ...head].some((metric) => metric.version !== base[0]!.version) + ) { + throw new Error(`Benchmark ${id} is missing or has a different version.`) + } + const baseSamples = base.flatMap((metric) => metric.samplesNsPerOp) + const headSamples = head.flatMap((metric) => metric.samplesNsPerOp) + const baseMedian = median(baseSamples) + const headMedian = median(headSamples) + const baseProcessMedians = base.map((metric) => + median(metric.samplesNsPerOp) + ) + const headProcessMedians = head.map((metric) => + median(metric.samplesNsPerOp) + ) + comparison.comparisons.push({ + id, + baseMedianNsPerOp: baseMedian, + headMedianNsPerOp: headMedian, + deltaPercent: (headMedian / baseMedian - 1) * 100, + baseProcessMedians, + headProcessMedians, + baseMadPercent: (medianAbsoluteDeviation(baseSamples) / baseMedian) * 100, + headMadPercent: (medianAbsoluteDeviation(headSamples) / headMedian) * 100, + pairChangesPercent: headProcessMedians.map( + (value, index) => (value / baseProcessMedians[index]! - 1) * 100 + ), + }) + } + return comparison } export function toBencherMetricFormat( - headRuns: readonly BenchmarkRunResult[] + runs: readonly BenchmarkRunResult[] ): BencherMetricFormat { - const metrics = indexMetrics(headRuns) return Object.fromEntries( - [...metrics.entries()] + [...indexMetrics(runs)] .sort(([left], [right]) => left.localeCompare(right)) - .map(([id, entries]) => { - const values = samples(entries) - const interval = bootstrapRunMedian( - entries.map((metric) => metric.samplesNsPerOp), - 10_000, - `bencher:${id}` - ) - return [ - id, - { - latency: { - value: median(values), - lower_value: interval[0], - upper_value: interval[1], - }, + .map(([id, metrics]) => [ + id, + { + latency: { + value: median(metrics.flatMap((metric) => metric.samplesNsPerOp)), }, - ] - }) + }, + ]) ) } - -function formatNumber(value: number): string { - if (value >= 1_000_000) return `${(value / 1_000_000).toFixed(2)} ms` - if (value >= 1_000) return `${(value / 1_000).toFixed(2)} µs` - return `${value.toFixed(1)} ns` -} - -function icon(verdict: MetricVerdict): string { - switch (verdict) { - case 'improvement': - return '🟢 improvement' - case 'regression': - return '🔴 regression' - case 'inconclusive': - return '🟡 noisy' - case 'advisory': - return 'ℹ️ advisory' - default: - return '⚪ unchanged' - } -} - -export function renderPlatformMarkdown(comparison: PlatformComparison): string { - const lines = [`### ${comparison.platform === 'ios' ? 'iOS' : 'Android'}`] - if (!comparison.suiteComparable) { - lines.push( - '', - '> Benchmark definitions changed in this PR. Results require a new baseline and are not compared.' - ) - return lines.join('\n') - } - lines.push( - '', - '| Benchmark | Base | Head | Δ | 95% CI | Status |', - '| --- | ---: | ---: | ---: | ---: | --- |' - ) - for (const metric of comparison.comparisons) { - lines.push( - `| \`${metric.id}\` | ${formatNumber(metric.baseMedianNsPerOp)} | ${formatNumber(metric.headMedianNsPerOp)} | ${metric.deltaPercent.toFixed(2)}% | ${metric.deltaConfidenceInterval95[0].toFixed(2)}%…${metric.deltaConfidenceInterval95[1].toFixed(2)}% | ${icon(metric.verdict)} |` - ) - } - return lines.join('\n') -} diff --git a/scripts/performance/github-report.ts b/scripts/performance/github-report.ts index 4fa4576a7..34de3255e 100644 --- a/scripts/performance/github-report.ts +++ b/scripts/performance/github-report.ts @@ -1,15 +1,15 @@ import { readFile } from 'node:fs/promises' import path from 'node:path' import { parseArguments, requiredArgument } from './args' -import { isSafeSha } from './schema' +import type { ReportMetadata } from './report' const COMMENT_MARKER = '' -interface PullRequestReport { - repository: string +interface PullRequestReport extends Pick< + ReportMetadata, + 'repository' | 'baseSha' | 'headSha' +> { pullRequestNumber: number - baseSha: string - headSha: string markdown: string } @@ -23,16 +23,8 @@ export async function postPerformanceComment( report: PullRequestReport, request: GitHubRequest ): Promise<'created' | 'updated' | 'stale'> { - if ( - !/^[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+$/.test(report.repository) || - !Number.isSafeInteger(report.pullRequestNumber) || - report.pullRequestNumber < 1 || - !isSafeSha(report.baseSha) || - !isSafeSha(report.headSha) || - report.markdown.length > 60_000 - ) { - throw new Error('Invalid validated PR report metadata or size.') - } + if (report.markdown.length > 60_000) + throw new Error('Performance comment exceeds GitHub size limit.') const root = `/repos/${report.repository}` // A PR may have advanced while validation or Bencher publishing was running. @@ -83,10 +75,7 @@ if (import.meta.main) { const directory = requiredArgument(argumentsMap, 'directory') const metadata = JSON.parse( await readFile(path.join(directory, 'metadata.json'), 'utf8') - ) as Omit - if (metadata.repository !== process.env.GITHUB_REPOSITORY) { - throw new Error('Report repository does not match the trusted workflow.') - } + ) as ReportMetadata if (metadata.pullRequestNumber != null) { const token = process.env.GITHUB_TOKEN if (token == null) throw new Error('GITHUB_TOKEN is required.') @@ -95,7 +84,7 @@ if (import.meta.main) { 'utf8' ) const status = await postPerformanceComment( - { ...metadata, markdown }, + { ...metadata, pullRequestNumber: metadata.pullRequestNumber, markdown }, async (endpoint, method = 'GET', body) => { const response = await fetch(`https://api.github.com${endpoint}`, { method, diff --git a/scripts/performance/isolated-cases.test.ts b/scripts/performance/isolated-cases.test.ts index 9ebb1f45d..738925b26 100644 --- a/scripts/performance/isolated-cases.test.ts +++ b/scripts/performance/isolated-cases.test.ts @@ -34,15 +34,9 @@ function result(index: number): BenchmarkRunResult { version: 2, family: 'control', implementation: 'nitro-cpp', - advisory: false, iterations: 1_500_000, chunkIterations: 5_000, samplesNsPerOp: [100 + index, 101 + index], - medianNsPerOp: 100.5 + index, - p95NsPerOp: 101 + index, - medianAbsoluteDeviationNsPerOp: 0.5, - robustCoefficientOfVariationPercent: 0.5, - medianConfidenceInterval95: [100 + index, 101 + index], checksum: 42, }, ], diff --git a/scripts/performance/paired-bootstrap.ts b/scripts/performance/paired-bootstrap.ts deleted file mode 100644 index 0388520e6..000000000 --- a/scripts/performance/paired-bootstrap.ts +++ /dev/null @@ -1,93 +0,0 @@ -import { - median, - quantile, -} from '../../apps/benchmark/src/benchmarks/statistics' - -function createRandom(seed: string): () => number { - let state = 17 - for (let index = 0; index < seed.length; index++) { - state = (state * 31 + seed.charCodeAt(index)) % 2_147_483_647 - } - if (state === 0) state = 1 - return () => { - state = (state * 48_271) % 2_147_483_647 - return state / 2_147_483_647 - } -} - -function validateRuns( - runs: readonly (readonly number[])[], - iterations: number -): void { - if ( - runs.length === 0 || - runs.some( - (samples) => - samples.length === 0 || - samples.some((value) => !Number.isFinite(value) || value <= 0) - ) || - !Number.isInteger(iterations) || - iterations < 1 - ) { - throw new Error( - 'Bootstrap needs non-empty positive samples and iterations.' - ) - } -} - -function resampleRun( - run: readonly number[], - random: () => number, - samples: number[] -): void { - for (let sample = 0; sample < run.length; sample++) { - samples.push(run[Math.floor(random() * run.length)]!) - } -} - -/** Resample matched process runs first, then their batches (not iid batches). */ -export function bootstrapPairedRunChange( - base: readonly (readonly number[])[], - head: readonly (readonly number[])[], - iterations: number, - seed: string -): [number, number] { - validateRuns(base, iterations) - validateRuns(head, iterations) - if (base.length !== head.length) { - throw new Error('Paired bootstrap needs matching non-empty runs.') - } - const random = createRandom(seed) - - const changes = new Array(iterations) - for (let iteration = 0; iteration < iterations; iteration++) { - const baseSamples: number[] = [] - const headSamples: number[] = [] - for (let pair = 0; pair < base.length; pair++) { - // Preserve the matching base/head process pair when sampling blocks. - const runIndex = Math.floor(random() * base.length) - resampleRun(base[runIndex]!, random, baseSamples) - resampleRun(head[runIndex]!, random, headSamples) - } - changes[iteration] = (median(headSamples) / median(baseSamples) - 1) * 100 - } - return [quantile(changes, 0.025), quantile(changes, 0.975)] -} - -export function bootstrapRunMedian( - runs: readonly (readonly number[])[], - iterations: number, - seed: string -): [number, number] { - validateRuns(runs, iterations) - const random = createRandom(seed) - const medians = new Array(iterations) - for (let iteration = 0; iteration < iterations; iteration++) { - const samples: number[] = [] - for (let block = 0; block < runs.length; block++) { - resampleRun(runs[Math.floor(random() * runs.length)]!, random, samples) - } - medians[iteration] = median(samples) - } - return [quantile(medians, 0.025), quantile(medians, 0.975)] -} diff --git a/scripts/performance/publish.test.ts b/scripts/performance/publish.test.ts index 67b5cf295..fa096a0c7 100644 --- a/scripts/performance/publish.test.ts +++ b/scripts/performance/publish.test.ts @@ -1,12 +1,8 @@ +import type { ReportMetadata } from './report' import { describe, expect, test } from 'bun:test' -import { - bencherArguments, - bencherPublications, - validateMetadata, - type Metadata, -} from './publish' +import { bencherArguments, bencherPublications } from './publish' -const metadata: Metadata = { +const metadata: ReportMetadata = { repository: 'margelo/nitro', eventName: 'pull_request', pullRequestNumber: 123, @@ -85,16 +81,4 @@ describe('Bencher publications', () => { bencherArguments(main, 'ios', 'base', '/validated', 'nitro') ).toThrow() }) - - test('rejects missing/duplicate platforms and inconsistent PR metadata', () => { - expect(validateMetadata(metadata)).toEqual(metadata) - for (const invalid of [ - { ...metadata, platforms: ['ios', 'ios'] }, - { ...metadata, platforms: [] }, - { ...metadata, pullRequestNumber: null }, - { ...metadata, eventName: 'push' }, - { ...metadata, headSha: 'not-a-sha' }, - ]) - expect(() => validateMetadata(invalid)).toThrow() - }) }) diff --git a/scripts/performance/publish.ts b/scripts/performance/publish.ts index b7194ef51..69262c335 100644 --- a/scripts/performance/publish.ts +++ b/scripts/performance/publish.ts @@ -1,52 +1,7 @@ import { readFile } from 'node:fs/promises' import path from 'node:path' import { parseArguments, requiredArgument } from './args' -import { isSafeSha } from './schema' - -export interface Metadata { - repository: string - eventName: 'pull_request' | 'push' | 'schedule' | 'workflow_dispatch' - pullRequestNumber: number | null - baseSha: string - headSha: string - platforms: ('android' | 'ios')[] -} - -export function validateMetadata(value: unknown): Metadata { - if (value == null || typeof value !== 'object') { - throw new Error('Invalid metadata.') - } - const metadata = value as Partial - if ( - typeof metadata.repository !== 'string' || - (metadata.eventName !== 'pull_request' && - metadata.eventName !== 'push' && - metadata.eventName !== 'schedule' && - metadata.eventName !== 'workflow_dispatch') || - (metadata.pullRequestNumber !== null && - (!Number.isInteger(metadata.pullRequestNumber) || - (metadata.pullRequestNumber ?? 0) < 1)) || - typeof metadata.baseSha !== 'string' || - typeof metadata.headSha !== 'string' || - !isSafeSha(metadata.baseSha) || - !isSafeSha(metadata.headSha) || - !Array.isArray(metadata.platforms) || - metadata.platforms.length !== 2 || - new Set(metadata.platforms).size !== 2 || - metadata.platforms.some( - (platform) => platform !== 'android' && platform !== 'ios' - ) - ) { - throw new Error('Invalid metadata.') - } - if ( - (metadata.eventName === 'pull_request') !== - (metadata.pullRequestNumber !== null) - ) { - throw new Error('PR event and number must agree.') - } - return metadata as Metadata -} +import type { ReportMetadata } from './report' const testbeds = { android: 'nitro-benchmark-android-release-x86-64-api-36-ubuntu-24-04-kvm', @@ -54,7 +9,7 @@ const testbeds = { } as const export function bencherArguments( - metadata: Metadata, + metadata: ReportMetadata, platform: 'android' | 'ios', revision: 'base' | 'head', directory: string, @@ -97,7 +52,7 @@ export function bencherArguments( } export function bencherPublications( - metadata: Metadata, + metadata: ReportMetadata, directory: string, project: string ) { @@ -133,8 +88,8 @@ if (import.meta.main) { 'BENCHER_PROJECT, BENCHER_API_KEY, and GITHUB_TOKEN are required.' ) } - const metadata = validateMetadata( - JSON.parse(await readFile(path.join(directory, 'metadata.json'), 'utf8')) + const metadata: ReportMetadata = JSON.parse( + await readFile(path.join(directory, 'metadata.json'), 'utf8') ) for (const { platform, revision, command } of bencherPublications( metadata, diff --git a/scripts/performance/report-markdown.test.ts b/scripts/performance/report-markdown.test.ts index 8d21afba6..5655378f9 100644 --- a/scripts/performance/report-markdown.test.ts +++ b/scripts/performance/report-markdown.test.ts @@ -1,136 +1,74 @@ -import { describe, expect, test } from 'bun:test' -import type { - MetricComparison, - MetricVerdict, - PlatformComparison, -} from './comparison' +import { expect, test } from 'bun:test' +import type { MetricComparison, PlatformComparison } from './comparison' import { renderPerformanceReportMarkdown } from './report-markdown' -const BASE_SHA = 'a'.repeat(40) -const HEAD_SHA = 'b'.repeat(40) - +const options = { + repository: 'margelo/nitro', + baseSha: 'a'.repeat(40), + headSha: 'b'.repeat(40), + workflowRunUrl: 'https://github.com/margelo/nitro/actions/runs/123', + artifactId: 987, + runAttempt: 2, +} function metric( id: string, - verdict: MetricVerdict, - deltaPercent: number + delta: number, + pairs = [delta, delta] ): MetricComparison { return { id, - advisory: verdict === 'advisory', baseMedianNsPerOp: 100, - headMedianNsPerOp: 100 * (1 + deltaPercent / 100), - deltaPercent, - deltaConfidenceInterval95: [deltaPercent - 1, deltaPercent + 1], - baseRobustCvPercent: 1, - headRobustCvPercent: 1, - budgetPercent: 5, - verdict, + headMedianNsPerOp: 100 + delta, + deltaPercent: delta, + baseProcessMedians: [90, 110], + headProcessMedians: [90 + delta, 110 + delta], + baseMadPercent: 10, + headMadPercent: 12, + pairChangesPercent: pairs, } } - -function platform( - platformName: 'android' | 'ios', - comparisons: MetricComparison[], +function report( + metrics: MetricComparison[], suiteComparable = true ): PlatformComparison { - return { - schemaVersion: 1, - platform: platformName, - baseSha: BASE_SHA, - headSha: HEAD_SHA, - baseSuiteHash: 'c'.repeat(64), - headSuiteHash: suiteComparable ? 'c'.repeat(64) : 'd'.repeat(64), - suiteComparable, - advisoryMode: true, - rerunRecommended: comparisons.some( - (entry) => entry.verdict === 'inconclusive' - ), - hasRegression: comparisons.some((entry) => entry.verdict === 'regression'), - comparisons, - } + return { platform: 'ios', ...options, suiteComparable, comparisons: metrics } } - -describe('performance report Markdown', () => { - test('shows decisive changes and collapses all other results', () => { - const comparisons = [ - metric('javascript/control/add-numbers', 'unchanged', 1), - metric('turbo-module/control/add-numbers', 'inconclusive', 7), - metric('nitro-cpp/primitive/simple-func', 'regression', 15), - metric('nitro-cpp/primitive/add-numbers', 'improvement', -12), - metric('nitro-cpp/variant/number-or-string', 'unchanged', 2), - metric('nitro-cpp/promise/immediate', 'advisory', 20), - ] - const markdown = renderPerformanceReportMarkdown( - [{ comparison: platform('android', comparisons) }], - { - advisory: true, - repository: 'margelo/nitro', - baseSha: BASE_SHA, - headSha: HEAD_SHA, - workflowRunUrl: - 'https://github.com/margelo/nitro/actions/runs/123456789', - } - ) - - const decisive = markdown.split('
')[0]! - const collapsed = markdown.split('
')[1]! - expect(decisive).toContain('C++ simpleFunc()') - expect(decisive).toContain( - '100.0 ns' - ) - expect(decisive).toContain('🔴 +15% slower') - expect(decisive).toContain('C++ addNumbers()') - expect(decisive).toContain( - '88.0 ns' - ) - expect(decisive).toContain('🟢 -12% faster') - expect(decisive).not.toContain('JavaScript') - expect(collapsed).toContain('All Benchmarks') - expect(collapsed).toContain( - 'JavaScript addNumbers()' - ) - expect(collapsed).toContain('number | string variant') - expect(collapsed).toContain('⚪ ~1% unchanged') - expect(collapsed).toContain('🟡 +7% slower (noisy)') - expect(collapsed).toContain('ℹ️ +20% slower (advisory)') - expect(collapsed).toContain(' \n \n ') - expect(markdown).not.toContain('| Benchmark |') - expect(markdown).toContain( - `Benchmarking Code Diff [\`${BASE_SHA.slice(0, 8)}\`...\`${HEAD_SHA.slice(0, 8)}\`](https://github.com/margelo/nitro/compare/${BASE_SHA}..${HEAD_SHA}) ([view raw output](https://github.com/margelo/nitro/actions/runs/123456789))` - ) - }) - - test('prints iOS before Android regardless of input order', () => { - const markdown = renderPerformanceReportMarkdown( - [ - { comparison: platform('android', []) }, - { comparison: platform('ios', []) }, - ], - { - advisory: true, - repository: 'margelo/nitro', - baseSha: BASE_SHA, - headSha: HEAD_SHA, - } - ) - expect(markdown.indexOf('### iOS')).toBeLessThan( - markdown.indexOf('### Android') - ) - expect(markdown.match(/Performance is unchanged! 😎/g)).toHaveLength(2) - }) - - test('makes incompatible benchmark definitions an explicit rebaseline', () => { - const markdown = renderPerformanceReportMarkdown( - [{ comparison: platform('ios', [], false) }], - { - advisory: true, - repository: 'margelo/nitro', - baseSha: BASE_SHA, - headSha: HEAD_SHA, - } - ) - - expect(markdown).toContain('Results require a new baseline') - expect(markdown).not.toContain('
') - }) +test('large Promise changes and disagreeing process pairs remain visible', () => { + const text = renderPerformanceReportMarkdown( + [ + report([ + metric('nitro-cpp/promise/immediate', 20, [-5, 45]), + metric('nitro-cpp/primitive/add-numbers', 2), + ]), + ], + options + ) + const main = text.split('
')[0]! + expect(main).toContain('immediatePromise()') + expect(main).toContain('+20% slower; process pairs disagree') + expect(main).not.toContain('addNumbers()') + expect(text).toContain('90.0 ns, 110.0 ns') + expect(text).toContain('10.0%, 12.0%') + expect(text).toContain('performance-report-2 (JSON artifact)') + expect(text).toContain('/actions/runs/123/artifacts/987') + expect(text).not.toMatch(/95%|unchanged!|calibrated.*budget/) +}) +test('small observed changes do not claim equality', () => { + const text = renderPerformanceReportMarkdown( + [report([metric('nitro-cpp/primitive/add-numbers', 0, [-30, 30])])], + options + ) + expect(text).toContain('This does not establish equal performance.') + expect(text).toContain('process pairs disagree') +}) +test('same revision and changed suites are explicit', () => { + expect( + renderPerformanceReportMarkdown([report([], false)], options) + ).toContain('require a new baseline') + expect( + renderPerformanceReportMarkdown([], { + ...options, + headSha: options.baseSha, + }) + ).toContain('Same-revision baseline run') }) diff --git a/scripts/performance/report-markdown.ts b/scripts/performance/report-markdown.ts index 3ab50124d..18e8b0fcf 100644 --- a/scripts/performance/report-markdown.ts +++ b/scripts/performance/report-markdown.ts @@ -1,16 +1,8 @@ -import type { MetricComparison, PlatformComparison } from './comparison' - -export interface PlatformReportMarkdownInput { - comparison: PlatformComparison -} - -export interface PerformanceReportMarkdownOptions { - advisory: boolean - repository: string - baseSha: string - headSha: string - workflowRunUrl?: string -} +import { + REPORTING_THRESHOLD_PERCENT, + type MetricComparison, + type PlatformComparison, +} from './comparison' const OPERATION_NAMES: Readonly> = { 'add-numbers': 'addNumbers()', @@ -89,135 +81,101 @@ function formatPercent(value: number): string { function directionalChange(deltaPercent: number): string { if (deltaPercent > 0) return `+${formatPercent(deltaPercent)}% slower` if (deltaPercent < 0) return `-${formatPercent(deltaPercent)}% faster` - return '~0% unchanged' -} - -function difference(metric: MetricComparison): string { - switch (metric.verdict) { - case 'regression': - return `🔴 ${directionalChange(metric.deltaPercent)}` - case 'improvement': - return `🟢 ${directionalChange(metric.deltaPercent)}` - case 'inconclusive': - return `🟡 ${directionalChange(metric.deltaPercent)} (noisy)` - case 'advisory': - return `ℹ️ ${directionalChange(metric.deltaPercent)} (advisory)` - case 'unchanged': - return `⚪ ~${formatPercent(metric.deltaPercent)}% unchanged` - } + return '~0% observed change' } -function measurement( - metric: MetricComparison, - revision: 'base' | 'head' +function table( + metrics: readonly MetricComparison[], + platform: PlatformComparison['platform'] ): string { - const before = metric.baseMedianNsPerOp - const after = metric.headMedianNsPerOp - const value = revision === 'base' ? before : after - const isFaster = revision === 'base' ? before < after : after < before - const formatted = formatNumber(value) - return isFaster ? `${formatted}` : formatted + return [ + '| Benchmark | Base p50 | Head p50 | Observed change |', + '| --- | ---: | ---: | --- |', + ...metrics.map((metric) => { + const pairMin = Math.min(...metric.pairChangesPercent) + const pairMax = Math.max(...metric.pairChangesPercent) + const quality = + pairMin <= -REPORTING_THRESHOLD_PERCENT && + pairMax >= REPORTING_THRESHOLD_PERCENT + ? '; process pairs disagree' + : '' + return `| ${benchmarkName(metric.id, platform)} | ${formatNumber(metric.baseMedianNsPerOp)} | ${formatNumber(metric.headMedianNsPerOp)} | ${directionalChange(metric.deltaPercent)}${quality} |` + }), + ].join('\n') } -function renderMetricTable( - metrics: readonly MetricComparison[], - platform: PlatformComparison['platform'], - indentation = 0 +export function renderPerformanceReportMarkdown( + platforms: readonly PlatformComparison[], + options: { + repository: string + baseSha: string + headSha: string + workflowRunUrl?: string + artifactId?: number + runAttempt?: number + } ): string { - const indent = ' '.repeat(indentation) - const level1 = ' '.repeat(indentation + 2) - const level2 = ' '.repeat(indentation + 4) - const level3 = ' '.repeat(indentation + 6) const lines = [ - `${indent}
`, - `${level1}`, - `${level2}`, - `${level3}`, - `${level3}`, - `${level3}`, - `${level3}`, - `${level2}`, - `${level1}`, - `${level1}`, + '## Performance Report', + '', + '> **Report only:** Measurements do not fail this PR. Process pairs describe this run; they do not establish statistical confidence.', ] - for (const metric of metrics) { + if (options.baseSha === options.headSha) { lines.push( - `${level2}`, - `${level3}`, - `${level3}`, - `${level3}`, - `${level3}`, - `${level2}` + '', + 'Same-revision baseline run. Differences show measurement variation, not a code change.' ) } - lines.push(`${level1}`, `${indent}
BenchmarkBeforeAfterDifference
${benchmarkName(metric.id, platform)}${measurement(metric, 'base')}${measurement(metric, 'head')}${difference(metric)}
`) - return lines.join('\n') -} - -function renderPlatform(input: PlatformReportMarkdownInput): string[] { - const { comparison } = input - const name = platformName(comparison.platform) - const lines = [`### ${name}`] - if (!comparison.suiteComparable) { + for (const platform of [...platforms].sort((a, b) => + b.platform.localeCompare(a.platform) + )) { + lines.push('', `### ${platformName(platform.platform)}`, '') + if (!platform.suiteComparable) { + lines.push( + 'Benchmark definitions changed. Results require a new baseline and are not compared.' + ) + continue + } + const changed = platform.comparisons.filter( + (metric) => Math.abs(metric.deltaPercent) >= REPORTING_THRESHOLD_PERCENT + ) + lines.push( + changed.length === 0 + ? `No observed change reached the ${REPORTING_THRESHOLD_PERCENT}% reporting threshold. This does not establish equal performance.` + : table(changed, platform.platform) + ) lines.push( '', - '> Benchmark definitions changed in this PR. Results require a new baseline and are not compared.' + '
', + 'All benchmarks and process variation', + '', + table(platform.comparisons, platform.platform), + '', + '| Benchmark | Base process p50 | Head process p50 | Paired changes | Sample MAD / p50 (base, head) |', + '| --- | --- | --- | --- | --- |' + ) + for (const metric of platform.comparisons) { + lines.push( + `| ${benchmarkName(metric.id, platform.platform)} | ${metric.baseProcessMedians.map(formatNumber).join(', ')} | ${metric.headProcessMedians.map(formatNumber).join(', ')} | ${metric.pairChangesPercent.map(directionalChange).join(', ')} | ${metric.baseMadPercent.toFixed(1)}%, ${metric.headMadPercent.toFixed(1)}% |` + ) + } + lines.push( + '', + 'p50 is the median of timed batch averages in ns/op, not individual-call latency. MAD describes sample spread; ordered raw samples retain within-process drift.', + '', + '
' ) - return lines - } - - const changed = comparison.comparisons.filter( - (metric) => - metric.verdict === 'regression' || metric.verdict === 'improvement' - ) - const other = comparison.comparisons.filter( - (metric) => - metric.verdict !== 'regression' && metric.verdict !== 'improvement' - ) - lines.push( - '', - changed.length === 0 - ? 'Performance is unchanged! 😎' - : renderMetricTable(changed, comparison.platform), - '', - '
', - ' All Benchmarks', - other.length === 0 - ? '

Every benchmark had a decisive change.

' - : renderMetricTable(other, comparison.platform, 2), - '
' - ) - return lines -} - -export function renderPerformanceReportMarkdown( - platforms: readonly PlatformReportMarkdownInput[], - options: PerformanceReportMarkdownOptions -): string { - const orderedPlatforms = [...platforms].sort( - ({ comparison: left }, { comparison: right }) => - left.platform === right.platform ? 0 : left.platform === 'ios' ? -1 : 1 - ) - const lines = [ - '## Performance Report', - '', - options.advisory - ? '> ⚠️ **Advisory:** Results do not fail this PR while the baseline is being calibrated.' - : '> Stable metrics are enforced against their calibrated regression budgets.', - ] - for (const platform of orderedPlatforms) { - lines.push('', ...renderPlatform(platform)) } - - const compareUrl = `https://github.com/${options.repository}/compare/${options.baseSha}..${options.headSha}` - const rawOutput = - options.workflowRunUrl == null - ? '' - : ` ([view raw output](${options.workflowRunUrl}))` lines.push( '', - `Benchmarking Code Diff [\`${options.baseSha.slice(0, 8)}\`...\`${options.headSha.slice(0, 8)}\`](${compareUrl})${rawOutput}`, + `Benchmarking Code Diff [\`${options.baseSha.slice(0, 8)}\`...\`${options.headSha.slice(0, 8)}\`](https://github.com/${options.repository}/compare/${options.baseSha}..${options.headSha})${options.workflowRunUrl == null ? '' : ` ([view CI run](${options.workflowRunUrl}))`}`, '' ) + if (options.artifactId != null && options.workflowRunUrl != null) { + lines.push( + `Raw measurements: [performance-report-${options.runAttempt} (JSON artifact)](${options.workflowRunUrl}/artifacts/${options.artifactId}). Run ${options.workflowRunUrl.split('/').at(-1)}, attempt ${options.runAttempt}. Download requires GitHub access.`, + '' + ) + } return lines.join('\n') } diff --git a/scripts/performance/report-validation.test.ts b/scripts/performance/report-validation.test.ts index 18c5209cb..4b948699f 100644 --- a/scripts/performance/report-validation.test.ts +++ b/scripts/performance/report-validation.test.ts @@ -3,7 +3,6 @@ import { mkdir, mkdtemp, readFile, rm } from 'node:fs/promises' import os from 'node:os' import path from 'node:path' import type { BenchmarkRunResult } from '../../apps/benchmark/src/benchmarks/types' -import { compareRuns } from './comparison' const REPOSITORY = 'margelo/nitro' const FORK_REPOSITORY = 'contributor/nitro' @@ -24,6 +23,7 @@ function run( return { schemaVersion: 1, suiteVersion: 1, + benchmarkCount: 1, configuration: { runId: `${platform}-${revision}-${sequence}`, reverse: sequence === 2, @@ -54,15 +54,9 @@ function run( version: 1, family: 'primitive', implementation: 'nitro-cpp', - advisory: false, iterations: 10_000, chunkIterations: 10_000, samplesNsPerOp: samples, - medianNsPerOp: center, - p95NsPerOp: center + 1, - medianAbsoluteDeviationNsPerOp: 1, - robustCoefficientOfVariationPercent: 1.4826, - medianConfidenceInterval95: [center - 1, center + 1], checksum: 42, }, ], @@ -94,22 +88,15 @@ async function createFixture(root: string): Promise<{ } } - const comparisons = (['android', 'ios'] as const).map((platform) => - compareRuns( - [run(platform, 'base', 1), run(platform, 'base', 2)], - [run(platform, 'head', 1), run(platform, 'head', 2)], - true - ) - ) await writeJson(path.join(artifact, 'performance-report.json'), { - schemaVersion: 1, + schemaVersion: 2, eventName: 'pull_request', repository: REPOSITORY, pullRequestNumber: 123, baseSha: BASE_SHA, headSha: HEAD_SHA, - generatedAt: '2026-09-04T00:00:00.000Z', - comparisons, + workflowRunId: 123456789, + runAttempt: 1, }) const trustedEvent = path.join(root, 'workflow-run.json') @@ -117,6 +104,7 @@ async function createFixture(root: string): Promise<{ repository: { full_name: REPOSITORY }, workflow_run: { id: 123456789, + run_attempt: 1, html_url: 'https://github.com/margelo/nitro/actions/runs/123456789', event: 'pull_request', head_sha: HEAD_SHA, @@ -126,6 +114,7 @@ async function createFixture(root: string): Promise<{ const trustedPullRequest = path.join(root, 'pull-request.json') await writeJson(trustedPullRequest, { number: 123, + state: 'open', base: { sha: BASE_SHA, repo: { full_name: REPOSITORY } }, head: { sha: HEAD_SHA, repo: { full_name: FORK_REPOSITORY } }, }) @@ -145,6 +134,8 @@ async function validate(fixture: Awaited>) { REPOSITORY, '--trusted-workflow-event', fixture.trustedEvent, + '--artifact-id', + '987', '--trusted-pull-request', fixture.trustedPullRequest, ], @@ -176,9 +167,11 @@ describe('trusted performance report validation', () => { expect(markdown).toContain( 'C++ addNumbers()' ) - expect(markdown).toContain('All Benchmarks') expect(markdown).toContain( - `Benchmarking Code Diff [\`${BASE_SHA.slice(0, 8)}\`...\`${HEAD_SHA.slice(0, 8)}\`](https://github.com/margelo/nitro/compare/${BASE_SHA}..${HEAD_SHA}) ([view raw output](https://github.com/margelo/nitro/actions/runs/123456789))` + 'All benchmarks and process variation' + ) + expect(markdown).toContain( + `Benchmarking Code Diff [\`${BASE_SHA.slice(0, 8)}\`...\`${HEAD_SHA.slice(0, 8)}\`](https://github.com/margelo/nitro/compare/${BASE_SHA}..${HEAD_SHA}) ([view CI run](https://github.com/margelo/nitro/actions/runs/123456789))` ) const bmf = JSON.parse( await readFile(path.join(fixture.output, 'bencher-ios.json'), 'utf8') @@ -196,7 +189,7 @@ describe('trusted performance report validation', () => { } }) - test('rejects a PR whose trusted head does not match the workflow run', async () => { + test('skips a PR that advanced after measurement', async () => { const root = await mkdtemp(path.join(os.tmpdir(), 'nitro-performance-')) try { const fixture = await createFixture(root) @@ -206,7 +199,8 @@ describe('trusted performance report validation', () => { pullRequest.head.sha = 'd'.repeat(40) await writeJson(fixture.trustedPullRequest, pullRequest) const result = await validate(fixture) - expect(result.exitCode).not.toBe(0) + expect(result.exitCode).toBe(0) + expect(result.error).toContain('Skipping stale') } finally { await rm(root, { recursive: true, force: true }) } @@ -225,4 +219,33 @@ describe('trusted performance report validation', () => { await rm(root, { recursive: true, force: true }) } }) + test.each([ + 'attempt', + 'repository', + 'raw-sha', + 'raw-order', + 'raw-count', + 'raw-id', + 'raw-mode', + ])('rejects mismatched or malformed raw provenance: %s', async (kind) => { + const root = await mkdtemp(path.join(os.tmpdir(), 'nitro-performance-')) + try { + const fixture = await createFixture(root) + const manifest = path.join(fixture.artifact, 'performance-report.json') + const raw = path.join(fixture.artifact, 'raw/ios/head-1.json') + const file = kind === 'attempt' || kind === 'repository' ? manifest : raw + const value = JSON.parse(await readFile(file, 'utf8')) + if (kind === 'attempt') value.runAttempt = 2 + if (kind === 'repository') value.repository = 'other/repo' + if (kind === 'raw-sha') value.configuration.commitSha = BASE_SHA + if (kind === 'raw-order') value.configuration.reverse = true + if (kind === 'raw-count') value.metrics[0].samplesNsPerOp.pop() + if (kind === 'raw-id') value.metrics[0].id = '' + if (kind === 'raw-mode') value.environment.dev = true + await writeJson(file, value) + expect((await validate(fixture)).exitCode).not.toBe(0) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) }) diff --git a/scripts/performance/report.ts b/scripts/performance/report.ts index 4be0f19de..6d0cc840c 100644 --- a/scripts/performance/report.ts +++ b/scripts/performance/report.ts @@ -1,85 +1,47 @@ -import { readFile } from 'node:fs/promises' -import { parseArguments, repeatedArgument, requiredArgument } from './args' -import type { PlatformComparison } from './comparison' -import { renderPerformanceReportMarkdown } from './report-markdown' +import { parseArguments, requiredArgument } from './args' -interface PerformanceReport { - schemaVersion: 1 +/** The artifact contains raw runs plus provenance. Only trusted code derives a report. */ +export interface PerformanceReport { + schemaVersion: 2 eventName: 'pull_request' | 'push' | 'schedule' | 'workflow_dispatch' repository: string pullRequestNumber: number | null baseSha: string headSha: string - generatedAt: string - comparisons: PlatformComparison[] + workflowRunId: number + runAttempt: number } -function isComparison(value: unknown): value is PlatformComparison { - if (value == null || typeof value !== 'object') return false - const comparison = value as Partial - return ( - comparison.schemaVersion === 1 && - (comparison.platform === 'ios' || comparison.platform === 'android') && - typeof comparison.baseSha === 'string' && - typeof comparison.headSha === 'string' && - typeof comparison.suiteComparable === 'boolean' && - Array.isArray(comparison.comparisons) - ) -} - -const argumentsMap = parseArguments(Bun.argv.slice(2)) -const comparisonFiles = repeatedArgument(argumentsMap, 'comparison') -const output = requiredArgument(argumentsMap, 'output') -const markdownOutput = requiredArgument(argumentsMap, 'markdown-output') -const repository = requiredArgument(argumentsMap, 'repository') -const eventName = requiredArgument(argumentsMap, 'event-name') -if ( - eventName !== 'pull_request' && - eventName !== 'push' && - eventName !== 'schedule' && - eventName !== 'workflow_dispatch' -) { - throw new Error(`Unsupported event: ${eventName}`) +export interface ReportMetadata extends Omit< + PerformanceReport, + 'schemaVersion' | 'workflowRunId' | 'runAttempt' +> { + platforms: ('android' | 'ios')[] } -const pullRequest = Number(argumentsMap.get('pull-request')?.[0] ?? '0') || null -const comparisons = await Promise.all( - comparisonFiles.map(async (file) => { - const value: unknown = JSON.parse(await readFile(file, 'utf8')) - if (!isComparison(value)) - throw new Error(`Invalid comparison file: ${file}`) - return value - }) -) -if (comparisons.length === 0) throw new Error('No comparisons supplied.') -const baseSha = comparisons[0]!.baseSha -const headSha = comparisons[0]!.headSha -if ( - comparisons.some( - (comparison) => - comparison.baseSha !== baseSha || comparison.headSha !== headSha +if (import.meta.main) { + const args = parseArguments(Bun.argv.slice(2)) + const eventName = requiredArgument(args, 'event-name') + if ( + eventName !== 'pull_request' && + eventName !== 'push' && + eventName !== 'schedule' && + eventName !== 'workflow_dispatch' + ) { + throw new Error(`Unsupported event: ${eventName}`) + } + const report: PerformanceReport = { + schemaVersion: 2, + eventName, + repository: requiredArgument(args, 'repository'), + pullRequestNumber: Number(requiredArgument(args, 'pull-request')) || null, + baseSha: requiredArgument(args, 'base-sha'), + headSha: requiredArgument(args, 'head-sha'), + workflowRunId: Number(process.env.GITHUB_RUN_ID), + runAttempt: Number(process.env.GITHUB_RUN_ATTEMPT), + } + await Bun.write( + requiredArgument(args, 'output'), + `${JSON.stringify(report, null, 2)}\n` ) -) { - throw new Error('Platform comparisons refer to different commits.') } - -const report: PerformanceReport = { - schemaVersion: 1, - eventName, - repository, - pullRequestNumber: pullRequest, - baseSha, - headSha, - generatedAt: new Date().toISOString(), - comparisons, -} -const advisory = comparisons.every((comparison) => comparison.advisoryMode) -const summary = renderPerformanceReportMarkdown( - comparisons.map((comparison) => ({ comparison })), - { advisory, repository, baseSha, headSha } -) - -await Promise.all([ - Bun.write(output, `${JSON.stringify(report, null, 2)}\n`), - Bun.write(markdownOutput, summary), -]) diff --git a/scripts/performance/run-device.ts b/scripts/performance/run-device.ts index fd9e697a1..eaae3f7e5 100644 --- a/scripts/performance/run-device.ts +++ b/scripts/performance/run-device.ts @@ -1,3 +1,4 @@ +import { median } from '../../apps/benchmark/src/benchmarks/statistics' import path from 'node:path' import { mkdir, readFile } from 'node:fs/promises' import { parseArguments, requiredArgument } from './args' @@ -163,7 +164,7 @@ async function runCase(index: number) { ) const metric = result.metrics[0]! console.info( - `[NitroBenchmark] case ${index + 1}/${result.benchmarkCount}: ${metric.id}, ${metric.iterations} ops/sample, median timed batch ${((metric.medianNsPerOp * metric.iterations) / 1e6).toFixed(1)} ms` + `[NitroBenchmark] case ${index + 1}/${result.benchmarkCount}: ${metric.id}, ${metric.iterations} ops/sample, median timed batch ${((median(metric.samplesNsPerOp) * metric.iterations) / 1e6).toFixed(1)} ms` ) return result } catch (error) { diff --git a/scripts/performance/run-sequence.ts b/scripts/performance/run-sequence.ts index 1a3a6bae2..c4ca5c7c3 100644 --- a/scripts/performance/run-sequence.ts +++ b/scripts/performance/run-sequence.ts @@ -1,11 +1,6 @@ import { mkdir, readFile } from 'node:fs/promises' import path from 'node:path' import { parseArguments, requiredArgument } from './args' -import { - compareRuns, - renderPlatformMarkdown, - toBencherMetricFormat, -} from './comparison' import { validateBenchmarkRun } from './schema' import { calculateSuiteHash } from './suite-hash' @@ -28,7 +23,6 @@ const device = requiredArgument(argumentsMap, 'device') const osVersion = requiredArgument(argumentsMap, 'os-version') const architecture = requiredArgument(argumentsMap, 'architecture') const toolchain = requiredArgument(argumentsMap, 'toolchain') -const advisoryMode = argumentsMap.get('mode')?.[0] !== 'enforce' await mkdir(outputDirectory, { recursive: true }) const [baseSuiteHash, headSuiteHash] = await Promise.all([ @@ -36,9 +30,6 @@ const [baseSuiteHash, headSuiteHash] = await Promise.all([ calculateSuiteHash(headRoot), ]) -const baseResults: string[] = [] -const headResults: string[] = [] - async function runOne( revision: 'base' | 'head', sequence: number, @@ -88,56 +79,9 @@ async function runOne( console.info( `[NitroBenchmark] ${new Date().toISOString()} ${runId}: complete; ${result.metrics.length} metrics, suite ${(result.durationMs / 1_000).toFixed(1)}s, wall ${((performance.now() - startedAt) / 1_000).toFixed(1)}s` ) - if (isBase) { - baseResults.push(output) - } else { - headResults.push(output) - } -} - -async function load(files: readonly string[]) { - return Promise.all( - files.map(async (file) => - validateBenchmarkRun(JSON.parse(await readFile(file, 'utf8'))) - ) - ) } await runOne('base', 1, false) await runOne('head', 1, false) await runOne('head', 2, true) await runOne('base', 2, true) - -let baseRuns = await load(baseResults) -let headRuns = await load(headResults) -let comparison = compareRuns(baseRuns, headRuns, advisoryMode) -if (comparison.rerunRecommended && comparison.suiteComparable) { - const noisyCount = comparison.comparisons.filter( - (metric) => metric.verdict === 'inconclusive' - ).length - console.info( - `[NitroBenchmark] ${noisyCount} inconclusive metrics; running the single permitted repeat pair.` - ) - await runOne('head', 3, false) - await runOne('base', 3, true) - baseRuns = await load(baseResults) - headRuns = await load(headResults) - comparison = compareRuns(baseRuns, headRuns, advisoryMode) -} - -await Promise.all([ - Bun.write( - path.join(outputDirectory, `comparison-${platform}.json`), - `${JSON.stringify(comparison, null, 2)}\n` - ), - Bun.write( - path.join(outputDirectory, `summary-${platform}.md`), - `${renderPlatformMarkdown(comparison)}\n` - ), - Bun.write( - path.join(outputDirectory, `bencher-${platform}.json`), - `${JSON.stringify(toBencherMetricFormat(headRuns), null, 2)}\n` - ), -]) - -if (!advisoryMode && comparison.hasRegression) process.exitCode = 1 diff --git a/scripts/performance/runner.test.ts b/scripts/performance/runner.test.ts index 92ea25cfe..3015eb77c 100644 --- a/scripts/performance/runner.test.ts +++ b/scripts/performance/runner.test.ts @@ -166,7 +166,7 @@ describe('benchmark runner', () => { expect(result.checksum).toBe(2 * 500_500 + 125_250) }) - test('chunks advisory async methods without timing cleanup', async () => { + test('chunks async methods without timing cleanup', async () => { let now = 0 spyOn(performance, 'now').mockImplementation(() => now) const result = await executeBatch( diff --git a/scripts/performance/schema.ts b/scripts/performance/schema.ts index 35153629b..ce8f0c77e 100644 --- a/scripts/performance/schema.ts +++ b/scripts/performance/schema.ts @@ -132,14 +132,10 @@ function validateMetric(value: unknown, index: number): BenchmarkMetric { finiteNumber( sample, `metrics[${index}].samples[${sampleIndex}]`, - 0, + Number.MIN_VALUE, MAX_NS_PER_OPERATION ) ) - const interval = value.medianConfidenceInterval95 - if (!Array.isArray(interval) || interval.length !== 2) { - throw new Error(`metrics[${index}] has an invalid confidence interval.`) - } const implementation = value.implementation if ( implementation !== 'javascript' && @@ -171,16 +167,12 @@ function validateMetric(value: unknown, index: number): BenchmarkMetric { if (!METRIC_ID_PATTERN.test(id)) { throw new Error(`metrics[${index}] has an invalid ID.`) } - if (typeof value.advisory !== 'boolean') { - throw new Error(`metrics[${index}].advisory must be a boolean.`) - } const iterations = positiveInteger( value.iterations, `metrics[${index}].iterations` ) - // Older v1 runs used one uninterrupted chunk per sample. const chunkIterations = positiveInteger( - value.chunkIterations ?? iterations, + value.chunkIterations, `metrics[${index}].chunkIterations` ) if (chunkIterations > iterations) { @@ -193,48 +185,9 @@ function validateMetric(value: unknown, index: number): BenchmarkMetric { version: positiveInteger(value.version, `metrics[${index}].version`), family: family as BenchmarkMetric['family'], implementation, - advisory: value.advisory, iterations, chunkIterations, samplesNsPerOp, - medianNsPerOp: finiteNumber( - value.medianNsPerOp, - `metrics[${index}].medianNsPerOp`, - 0, - MAX_NS_PER_OPERATION - ), - p95NsPerOp: finiteNumber( - value.p95NsPerOp, - `metrics[${index}].p95NsPerOp`, - 0, - MAX_NS_PER_OPERATION - ), - medianAbsoluteDeviationNsPerOp: finiteNumber( - value.medianAbsoluteDeviationNsPerOp, - `metrics[${index}].medianAbsoluteDeviationNsPerOp`, - 0, - MAX_NS_PER_OPERATION - ), - robustCoefficientOfVariationPercent: finiteNumber( - value.robustCoefficientOfVariationPercent, - `metrics[${index}].robustCoefficientOfVariationPercent`, - 0, - 1_000_000 - ), - medianConfidenceInterval95: [ - finiteNumber( - interval[0], - `metrics[${index}].interval[0]`, - 0, - MAX_NS_PER_OPERATION - ), - finiteNumber( - interval[1], - `metrics[${index}].interval[1]`, - 0, - MAX_NS_PER_OPERATION - ), - ], checksum: finiteNumber(value.checksum, `metrics[${index}].checksum`), } } @@ -254,10 +207,7 @@ export function validateBenchmarkRun(value: unknown): BenchmarkRunResult { throw new Error('metrics must be a non-empty bounded array.') } const validatedMetrics = metrics.map(validateMetric) - const benchmarkCount = positiveInteger( - value.benchmarkCount ?? metrics.length, - 'benchmarkCount' - ) + const benchmarkCount = positiveInteger(value.benchmarkCount, 'benchmarkCount') if (benchmarkCount > MAX_METRICS || benchmarkCount < metrics.length) { throw new Error('benchmarkCount is outside the bounded suite size.') } @@ -266,6 +216,14 @@ export function validateBenchmarkRun(value: unknown): BenchmarkRunResult { ) { throw new Error('Metric IDs must be unique.') } + const sampleCount = value.runner.sampleCount + if ( + validatedMetrics.some( + (metric) => metric.samplesNsPerOp.length !== sampleCount + ) + ) { + throw new Error('Sample count does not match runner settings.') + } const startedAt = stringValue(value.startedAt, 'startedAt') if (Number.isNaN(Date.parse(startedAt))) { throw new Error('startedAt must be an ISO timestamp.') diff --git a/scripts/performance/select-report.test.ts b/scripts/performance/select-report.test.ts new file mode 100644 index 000000000..517590b4f --- /dev/null +++ b/scripts/performance/select-report.test.ts @@ -0,0 +1,42 @@ +import { expect, test } from 'bun:test' +import { selectReportArtifact } from './select-report' + +test('docs-only and cancelled measurements skip cleanly', () => { + expect( + selectReportArtifact( + 'success', + 1, + [], + [{ name: 'nitro-performance', conclusion: 'skipped' }] + ) + ).toBeUndefined() + expect(selectReportArtifact('cancelled', 1, [], [])).toBeUndefined() +}) +test('a relevant build failure or missing results remains a failure', () => { + expect(() => selectReportArtifact('failure', 1, [], [])).toThrow( + 'workflow failure' + ) + expect(() => selectReportArtifact('success', 1, [], [])).toThrow( + 'exactly one' + ) +}) +test('selects the immutable artifact for the triggering attempt only', () => { + const artifacts = [1, 2].map((id) => ({ + id, + name: `performance-report-${id}`, + expired: false, + })) + expect(selectReportArtifact('success', 2, artifacts, [])).toBe(2) + expect(() => selectReportArtifact('success', 3, artifacts, [])).toThrow() + expect(() => + selectReportArtifact('success', 2, [...artifacts, artifacts[1]!], []) + ).toThrow() + expect(() => + selectReportArtifact( + 'success', + 2, + [{ ...artifacts[1]!, expired: true }], + [] + ) + ).toThrow() +}) diff --git a/scripts/performance/select-report.ts b/scripts/performance/select-report.ts new file mode 100644 index 000000000..68392504f --- /dev/null +++ b/scripts/performance/select-report.ts @@ -0,0 +1,79 @@ +import { appendFile, readFile } from 'node:fs/promises' + +interface Artifact { + id: number + name: string + expired: boolean +} +interface Job { + name: string + conclusion: string | null +} + +export function selectReportArtifact( + conclusion: string | null, + attempt: number, + artifacts: readonly Artifact[], + jobs: readonly Job[] +): number | undefined { + if (conclusion === 'cancelled' || conclusion === 'skipped') return undefined + if (conclusion !== 'success') + throw new Error( + `Performance workflow ${conclusion}; no measurements published.` + ) + const name = `performance-report-${attempt}` + const matches = artifacts.filter( + (artifact) => artifact.name === name && !artifact.expired + ) + if ( + matches.length === 0 && + jobs.some( + (job) => job.name === 'nitro-performance' && job.conclusion === 'skipped' + ) + ) + return undefined + if (matches.length !== 1) + throw new Error(`Expected exactly one unexpired ${name} artifact.`) + const id = matches[0]!.id + if (!Number.isSafeInteger(id) || id < 1) + throw new Error('Invalid artifact ID.') + return id +} + +if (import.meta.main) { + const event = JSON.parse( + await readFile(process.env.GITHUB_EVENT_PATH!, 'utf8') + ) + const run = event.workflow_run + const root = `https://api.github.com/repos/${process.env.GITHUB_REPOSITORY}/actions/runs/${run.id}` + async function get(endpoint: string): Promise { + const response = await fetch(`${root}${endpoint}`, { + headers: { + Authorization: `Bearer ${process.env.GH_TOKEN}`, + Accept: 'application/vnd.github+json', + }, + signal: AbortSignal.timeout(30_000), + }) + if (!response.ok) + throw new Error(`GitHub artifact lookup failed: ${response.status}`) + return (await response.json()) as T + } + // Query the exact attempt; rerunning collection must not pick another attempt's results. + const [artifactResponse, jobResponse] = await Promise.all([ + get<{ total_count: number; artifacts: Artifact[] }>( + '/artifacts?per_page=100' + ), + get<{ total_count: number; jobs: Job[] }>( + `/attempts/${run.run_attempt}/jobs?per_page=100` + ), + ]) + if (artifactResponse.total_count > 100 || jobResponse.total_count > 100) + throw new Error('Performance run exceeds the artifact/job lookup limit.') + const id = selectReportArtifact( + run.conclusion, + run.run_attempt, + artifactResponse.artifacts, + jobResponse.jobs + ) + await appendFile(process.env.GITHUB_OUTPUT!, `artifact_id=${id ?? ''}\n`) +} diff --git a/scripts/performance/statistics.test.ts b/scripts/performance/statistics.test.ts index 0138b6dcd..996609fed 100644 --- a/scripts/performance/statistics.test.ts +++ b/scripts/performance/statistics.test.ts @@ -1,47 +1,14 @@ -import { describe, expect, test } from 'bun:test' +import { expect, test } from 'bun:test' import { - bootstrapMedianConfidenceInterval, - bootstrapPercentChangeConfidenceInterval, median, medianAbsoluteDeviation, - quantile, - robustCoefficientOfVariationPercent, } from '../../apps/benchmark/src/benchmarks/statistics' -describe('benchmark statistics', () => { - test('calculates robust summary statistics', () => { - expect(median([1, 2, 3, 4])).toBe(2.5) - expect(quantile([1, 2, 3, 4, 5], 0.95)).toBeCloseTo(4.8) - expect(medianAbsoluteDeviation([1, 2, 3, 4, 100])).toBe(1) - expect( - robustCoefficientOfVariationPercent([98, 99, 100, 101, 102]) - ).toBeCloseTo(1.4826) - }) - - test('produces deterministic bootstrap intervals', () => { - const first = bootstrapMedianConfidenceInterval( - [9, 10, 11, 12, 13], - 1_000, - 'seed' - ) - const second = bootstrapMedianConfidenceInterval( - [9, 10, 11, 12, 13], - 1_000, - 'seed' - ) - expect(first).toEqual(second) - expect(first[0]).toBeLessThanOrEqual(11) - expect(first[1]).toBeGreaterThanOrEqual(11) - }) - - test('detects a clear percent change', () => { - const interval = bootstrapPercentChangeConfidenceInterval( - [99, 100, 100, 101, 100], - [114, 115, 115, 116, 115], - 2_000, - 'regression' - ) - expect(interval[0]).toBeGreaterThan(10) - expect(interval[1]).toBeLessThan(20) - }) +test('median and MAD preserve their input and resist an isolated outlier', () => { + const values = [100, 2, 3, 4, 1] + expect(median(values)).toBe(3) + expect(medianAbsoluteDeviation(values)).toBe(1) + expect(values).toEqual([100, 2, 3, 4, 1]) + expect(median([1, 2, 3, 4])).toBe(2.5) + expect(() => median([])).toThrow() }) diff --git a/scripts/performance/validate-report.ts b/scripts/performance/validate-report.ts index eae6415dc..120849c9c 100644 --- a/scripts/performance/validate-report.ts +++ b/scripts/performance/validate-report.ts @@ -1,12 +1,8 @@ -import { mkdir, readdir, readFile, stat } from 'node:fs/promises' +import type { PerformanceReport } from './report' +import { appendFile, mkdir, readdir, readFile, stat } from 'node:fs/promises' import path from 'node:path' import { parseArguments, requiredArgument } from './args' -import { - compareRuns, - type MetricComparison, - type PlatformComparison, - toBencherMetricFormat, -} from './comparison' +import { compareRuns, toBencherMetricFormat } from './comparison' import { renderPerformanceReportMarkdown } from './report-markdown' import { isSafeSha, validateBenchmarkRun } from './schema' import type { BenchmarkRunResult } from '../../apps/benchmark/src/benchmarks/types' @@ -18,6 +14,7 @@ interface TrustedWorkflowRunEvent { repository: { full_name: string } workflow_run: { id: number + run_attempt: number html_url: string event: 'pull_request' | 'push' | 'schedule' | 'workflow_dispatch' head_sha: string @@ -27,21 +24,11 @@ interface TrustedWorkflowRunEvent { interface TrustedPullRequest { number: number + state: string base: { sha: string; repo: { full_name: string } } head: { sha: string; repo: { full_name: string } } } -interface PerformanceReport { - schemaVersion: 1 - eventName: 'pull_request' | 'push' | 'schedule' | 'workflow_dispatch' - repository: string - pullRequestNumber: number | null - baseSha: string - headSha: string - generatedAt: string - comparisons: PlatformComparison[] -} - function object(value: unknown, name: string): Record { if (value == null || typeof value !== 'object' || Array.isArray(value)) { throw new Error(`${name} must be an object.`) @@ -104,6 +91,10 @@ function validateTrustedWorkflowRunEvent( }, workflow_run: { id, + run_attempt: finiteNumber( + workflowRun.run_attempt, + 'workflow_run.run_attempt' + ), html_url: boundedString( workflowRun.html_url, 'event.workflow_run.html_url' @@ -139,6 +130,7 @@ function validateTrustedPullRequest(value: unknown): TrustedPullRequest { } return { number, + state: boundedString(pullRequest.state, 'pull_request.state'), base: { sha: baseSha, repo: { @@ -160,97 +152,9 @@ function validateTrustedPullRequest(value: unknown): TrustedPullRequest { } } -function validateMetricComparison(value: unknown): MetricComparison { - const metric = object(value, 'metric comparison') - const id = boundedString(metric.id, 'metric.id') - if (!METRIC_ID_PATTERN.test(id)) throw new Error(`Unsafe metric ID: ${id}`) - const interval = metric.deltaConfidenceInterval95 - if (!Array.isArray(interval) || interval.length !== 2) { - throw new Error(`Invalid confidence interval for ${id}.`) - } - const verdict = metric.verdict - if ( - verdict !== 'improvement' && - verdict !== 'regression' && - verdict !== 'unchanged' && - verdict !== 'inconclusive' && - verdict !== 'advisory' - ) { - throw new Error(`Invalid verdict for ${id}.`) - } - if (typeof metric.advisory !== 'boolean') { - throw new Error(`Invalid advisory state for ${id}.`) - } - return { - id, - advisory: metric.advisory, - baseMedianNsPerOp: finiteNumber(metric.baseMedianNsPerOp, `${id}.base`), - headMedianNsPerOp: finiteNumber(metric.headMedianNsPerOp, `${id}.head`), - deltaPercent: finiteNumber(metric.deltaPercent, `${id}.delta`), - deltaConfidenceInterval95: [ - finiteNumber(interval[0], `${id}.interval[0]`), - finiteNumber(interval[1], `${id}.interval[1]`), - ], - baseRobustCvPercent: finiteNumber( - metric.baseRobustCvPercent, - `${id}.baseCv` - ), - headRobustCvPercent: finiteNumber( - metric.headRobustCvPercent, - `${id}.headCv` - ), - budgetPercent: finiteNumber(metric.budgetPercent, `${id}.budget`), - verdict, - } -} - -function validateComparison(value: unknown): PlatformComparison { - const comparison = object(value, 'comparison') - const platform = comparison.platform - if (platform !== 'ios' && platform !== 'android') { - throw new Error('Invalid comparison platform.') - } - const baseSha = boundedString(comparison.baseSha, 'comparison.baseSha') - const headSha = boundedString(comparison.headSha, 'comparison.headSha') - if (!isSafeSha(baseSha) || !isSafeSha(headSha)) { - throw new Error('Comparison SHAs are invalid.') - } - if ( - typeof comparison.suiteComparable !== 'boolean' || - typeof comparison.advisoryMode !== 'boolean' || - typeof comparison.rerunRecommended !== 'boolean' || - typeof comparison.hasRegression !== 'boolean' - ) { - throw new Error('Comparison flags are invalid.') - } - if ( - !Array.isArray(comparison.comparisons) || - comparison.comparisons.length > 100 - ) { - throw new Error('Comparison metrics are invalid.') - } - return { - schemaVersion: 1, - platform, - baseSha, - headSha, - baseSuiteHash: boundedString(comparison.baseSuiteHash, 'baseSuiteHash'), - headSuiteHash: boundedString(comparison.headSuiteHash, 'headSuiteHash'), - suiteComparable: comparison.suiteComparable, - advisoryMode: comparison.advisoryMode, - rerunRecommended: comparison.rerunRecommended, - hasRegression: comparison.hasRegression, - comparisons: comparison.comparisons.map(validateMetricComparison), - } -} - function validateReport(value: unknown): PerformanceReport { const report = object(value, 'report') - if ( - report.schemaVersion !== 1 || - !Array.isArray(report.comparisons) || - report.comparisons.length !== 2 - ) { + if (report.schemaVersion !== 2) { throw new Error('Invalid performance report.') } const pullRequestNumber = @@ -277,26 +181,15 @@ function validateReport(value: unknown): PerformanceReport { if (!isSafeSha(baseSha) || !isSafeSha(headSha)) { throw new Error('Report SHAs are invalid.') } - const comparisons = report.comparisons.map(validateComparison) - if ( - !comparisons.some((comparison) => comparison.platform === 'android') || - !comparisons.some((comparison) => comparison.platform === 'ios') - ) { - throw new Error('Report must contain Android and iOS exactly once.') - } - const generatedAt = boundedString(report.generatedAt, 'report.generatedAt') - if (Number.isNaN(Date.parse(generatedAt))) { - throw new Error('Report timestamp is invalid.') - } return { - schemaVersion: 1, + schemaVersion: 2, eventName, repository: boundedString(report.repository, 'report.repository'), pullRequestNumber, baseSha, headSha, - generatedAt, - comparisons, + workflowRunId: finiteNumber(report.workflowRunId, 'report.workflowRunId'), + runAttempt: finiteNumber(report.runAttempt, 'report.runAttempt'), } } @@ -319,7 +212,9 @@ if ( trustedWorkflowEvent.repository.full_name !== expectedRepository || trustedWorkflowEvent.workflow_run.event !== report.eventName || report.repository !== expectedRepository || - report.headSha !== trustedWorkflowEvent.workflow_run.head_sha + report.headSha !== trustedWorkflowEvent.workflow_run.head_sha || + report.workflowRunId !== trustedWorkflowEvent.workflow_run.id || + report.runAttempt !== trustedWorkflowEvent.workflow_run.run_attempt ) { throw new Error( 'Artifact event metadata does not match the triggering workflow.' @@ -342,14 +237,24 @@ if (report.eventName === 'pull_request') { pullRequest.base.repo.full_name !== expectedRepository || pullRequest.head.repo.full_name !== trustedWorkflowEvent.workflow_run.head_repository.full_name || - report.pullRequestNumber !== pullRequest.number || - report.baseSha !== pullRequest.base.sha || - report.headSha !== pullRequest.head.sha + report.pullRequestNumber !== pullRequest.number ) { throw new Error( 'Artifact metadata does not match the trusted pull request.' ) } + if ( + pullRequest.state !== 'open' || + report.baseSha !== pullRequest.base.sha || + report.headSha !== pullRequest.head.sha + ) { + console.info( + 'Skipping stale performance results: the pull request advanced or closed.' + ) + if (process.env.GITHUB_OUTPUT != null) + await appendFile(process.env.GITHUB_OUTPUT, 'stale=true\n') + process.exit(0) + } } else { if (report.pullRequestNumber !== null) { throw new Error( @@ -357,35 +262,28 @@ if (report.eventName === 'pull_request') { ) } } -if ( - report.comparisons.some( - (comparison) => - comparison.baseSha !== report.baseSha || - comparison.headSha !== report.headSha - ) -) { - throw new Error('Platform comparison metadata does not match the report.') -} - async function loadRawRuns( platform: 'android' | 'ios', revision: 'base' | 'head', expectedSha: string ): Promise { const directory = path.join(artifactDirectory, 'raw', platform) - const filePattern = new RegExp(`^${revision}-[1-3]\\.json$`) + const filePattern = new RegExp(`^${revision}-[1-2]\\.json$`) const files = (await readdir(directory)) .filter((file) => filePattern.test(file)) .sort() - if (files.length < 2 || files.length > 3) { - throw new Error(`Expected two or three ${platform} ${revision} runs.`) + if (files.length !== 2) { + throw new Error(`Expected two ${platform} ${revision} runs.`) } return Promise.all( files.map(async (file) => { const run = validateBenchmarkRun( await readBoundedJson(path.join(directory, file)) ) + const sequence = Number(file.match(/-(\d)\.json$/)![1]) if ( + run.configuration.runId !== `${platform}-${revision}-${sequence}` || + run.configuration.reverse !== (sequence === 2) || run.configuration.platform !== platform || run.configuration.commitSha !== expectedSha || run.runner.targetBatchDurationMs !== 150 || @@ -405,33 +303,29 @@ async function loadRawRuns( } const rebuiltComparisons = await Promise.all( - report.comparisons.map(async (uploadedComparison) => { - if (!uploadedComparison.advisoryMode) { - throw new Error('Performance enforcement cannot be enabled by PR code.') - } + (['android', 'ios'] as const).map(async (platform) => { const [baseRuns, headRuns] = await Promise.all([ - loadRawRuns(uploadedComparison.platform, 'base', report.baseSha), - loadRawRuns(uploadedComparison.platform, 'head', report.headSha), + loadRawRuns(platform, 'base', report.baseSha), + loadRawRuns(platform, 'head', report.headSha), ]) if (baseRuns.length !== headRuns.length) { throw new Error('Base and head run counts must match.') } - const comparison = compareRuns(baseRuns, headRuns, true) + const comparison = compareRuns(baseRuns, headRuns) return { comparison, baseRuns, headRuns } }) ) await mkdir(outputDirectory, { recursive: true }) const markdown = renderPerformanceReportMarkdown( - rebuiltComparisons.map(({ comparison }) => ({ - comparison, - })), + rebuiltComparisons.map(({ comparison }) => comparison), { - advisory: true, repository: expectedRepository, baseSha: report.baseSha, headSha: report.headSha, workflowRunUrl, + artifactId: Number(requiredArgument(argumentsMap, 'artifact-id')), + runAttempt: report.runAttempt, } ) await Bun.write(path.join(outputDirectory, 'performance-summary.md'), markdown) diff --git a/scripts/performance/workflow.test.ts b/scripts/performance/workflow.test.ts index f1ae00bb8..aff4d45ea 100644 --- a/scripts/performance/workflow.test.ts +++ b/scripts/performance/workflow.test.ts @@ -46,52 +46,14 @@ test('Android performance CI requires KVM and cannot fall back to software emula expect(emulator?.['script']).toContain('/ KVM') }) -test('pre-merge publishing uses pinned code on a separate, same-repository-only job', async () => { - const workflow = Bun.YAML.parse( +test('one trusted publisher handles internal and fork reports without executing PR code', async () => { + const entry = Bun.YAML.parse( await readFile( new URL('../../.github/workflows/performance.yml', import.meta.url), 'utf8' ) ) as any - const job = workflow.jobs['publish-pr'] - expect(job.needs).toEqual(['prepare', 'nitro-performance']) - expect(job.if).toContain("github.event_name == 'pull_request'") - expect(job.if).toContain( - 'github.event.pull_request.head.repo.full_name == github.repository' - ) - expect(job.if).toContain( - "needs.prepare.outputs.base_benchmark_available == 'true'" - ) - expect(workflow.permissions).toEqual({ contents: 'read' }) - expect(job.permissions['checks']).toBe('write') - const checkout = job.steps.find( - (s: any) => s.name === 'Checkout pinned reporting code' - ) - expect(checkout.with.ref).toMatch(/^[0-9a-f]{40}$/) - expect(checkout.with['persist-credentials']).toBe(false) - expect(JSON.stringify(job)).not.toContain('pull_request.head.sha') - expect(JSON.stringify(job)).not.toContain('bun install') - const comment = job.steps.find( - (s: any) => s.name === 'Post paired comparison to the PR' - ) - expect(comment.if).toBeUndefined() - const bencher = job.steps.find( - (s: any) => s.name === 'Install pinned Bencher CLI' - ) - expect(bencher.with.version).toBe('0.6.12') - const publish = job.steps.find( - (s: any) => s.name === 'Publish to Bencher and GitHub' - ) - expect(publish.env.BENCHER_API_KEY).toBe('${{ secrets.BENCHER_KEY }}') - for (const name of ['prepare', 'android', 'ios', 'nitro-performance']) { - expect(JSON.stringify(workflow.jobs[name])).not.toContain( - 'secrets.BENCHER_KEY' - ) - } -}) - -test('default-branch reporter handles forks without duplicating same-repository PR reports', async () => { - const workflow = Bun.YAML.parse( + const publisher = Bun.YAML.parse( await readFile( new URL( '../../.github/workflows/performance-report.yml', @@ -100,48 +62,23 @@ test('default-branch reporter handles forks without duplicating same-repository 'utf8' ) ) as any - expect(workflow.jobs.publish.if).toContain( - "github.event.workflow_run.event != 'pull_request'" - ) - expect(workflow.jobs.publish.if).toContain( - 'github.event.workflow_run.head_repository.full_name != github.repository' + expect(entry.permissions).toEqual({ contents: 'read' }) + expect(entry.jobs['publish-pr']).toBeUndefined() + expect(publisher.jobs.publish.if).toBeUndefined() + const source = JSON.stringify(publisher) + expect(source).not.toMatch( + /pull_request.head.sha|bun install|NITRO_BENCHER_ENABLED/ ) - const bencher = workflow.jobs.publish.steps.find( - (s: any) => s.name === 'Install Bencher CLI' + expect(source).toContain('secrets.BENCHER_KEY') + const steps = publisher.jobs.publish.steps as any[] + expect( + steps.find((s) => s.name === 'Download performance report').with[ + 'artifact-ids' + ] + ).toBe('${{ steps.select.outputs.artifact_id }}') + expect( + steps.findIndex((s) => s.name === 'Verify pinned Bencher binary') + ).toBeLessThan( + steps.findIndex((s) => s.name === 'Publish to Bencher and GitHub') ) - expect(bencher.with.version).toBe('0.6.12') -}) - -test('both publishers verify the reviewed CLI digest before exposing the Bencher key', async () => { - for (const [file, jobName] of [ - ['performance.yml', 'publish-pr'], - ['performance-report.yml', 'publish'], - ]) { - const workflow = Bun.YAML.parse( - await readFile( - new URL(`../../.github/workflows/${file}`, import.meta.url), - 'utf8' - ) - ) as any - const steps = workflow.jobs[jobName!].steps as { - name?: string - run?: string - if?: string - env?: Record - }[] - const verifyIndex = steps.findIndex( - (step) => step.name === 'Verify pinned Bencher binary' - ) - const publishIndex = steps.findIndex( - (step) => step.name === 'Publish to Bencher and GitHub' - ) - expect(verifyIndex).toBeGreaterThanOrEqual(0) - expect(verifyIndex).toBeLessThan(publishIndex) - expect(steps[verifyIndex]?.if).toBe(steps[publishIndex]?.if) - expect(steps[verifyIndex]?.run).toContain( - 'c2d3a6a7fae654246134e5ced1408bdb9ba4e198b0ac3b903af17a06574a7e08' - ) - expect(steps[verifyIndex]?.run).toContain('sha256sum --check -') - expect(steps[verifyIndex]?.env).toBeUndefined() - } })