From a81233b0ce047e04e2cfdd3ca8851eb320015105 Mon Sep 17 00:00:00 2001 From: Kat Perez Date: Wed, 9 Sep 2026 11:34:19 -0400 Subject: [PATCH] uefi-bench: Comment benchmark deltas on PRs Runs the patina_boot microbenchmarks at the base commit and at the merge result, and posts the per-benchmark change as a single PR comment that is updated in place. It is a reviewer aid, not a gate. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/uefi-bench-pr-comment.yml | 230 ++++++++++++++++++ .github/workflows/uefi-bench.yml | 114 +++++++++ uefi/crates/patina_boot/Cargo.toml | 6 + .../patina_boot/benches/orchestrator.rs | 15 +- 4 files changed, 363 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/uefi-bench-pr-comment.yml create mode 100644 .github/workflows/uefi-bench.yml diff --git a/.github/workflows/uefi-bench-pr-comment.yml b/.github/workflows/uefi-bench-pr-comment.yml new file mode 100644 index 0000000..b4d9b4d --- /dev/null +++ b/.github/workflows/uefi-bench-pr-comment.yml @@ -0,0 +1,230 @@ +# GitHub Actions: posts patina_boot benchmark results back to the originating +# pull request. +# +# SPDX-License-Identifier: MIT +# + +# This workflow triggers after the uefi-bench workflow has run. uefi-bench runs +# in the pull request context, which is read-only for pull requests from forks, +# so it uploads its results as an artifact instead of commenting. This workflow +# runs in the base repository context, where it has pull-requests: write, and +# renders the artifact into a single comment that is updated in place. +# +# The comment is a reviewer aid, not a gate. Changes smaller than the combined +# run-to-run spread are labeled as such, because shared CI runners routinely +# move these numbers by more than a real code change would. + +name: uefi-bench PR comment + +on: + workflow_run: + workflows: [uefi-bench] + types: + - completed + +permissions: + contents: read + pull-requests: write + +concurrency: + # A workflow_run payload carries no pull request number for pull requests + # from forks, so this cannot key on the number the way uefi-bench does. Head + # repository and branch together identify the same pull request; branch alone + # would not, because every fork has a `main`. + group: >- + ${{ github.workflow }}-${{ + github.event.workflow_run.head_repository.full_name || 'unknown' }}-${{ + github.event.workflow_run.head_branch || github.run_id }} + cancel-in-progress: true + +jobs: + comment: + name: post results + runs-on: ubuntu-latest + # Only pull request runs have a comment to post to, and a failed bench run + # uploads no results worth rendering. uefi-bench already fails loudly in + # that case. + if: >- + github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.conclusion == 'success' + steps: + - name: Download results + uses: actions/download-artifact@v4 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + name: bench-results + path: bench-results/ + run-id: ${{ github.event.workflow_run.id }} + + # A workflow_run payload carries no pull request number for pull requests + # from forks, so uefi-bench carries it across in the artifact. That + # artifact is produced by a job running in the pull request context, + # whose workflow file a fork can modify, so the value is untrusted: it + # has to be a plain number, and it has to name a pull request whose head + # really is the commit this run benchmarked. Without the second check a + # fork could point the comment at any issue in the repository and have + # the bot post there. + - name: Get PR number + id: get-pr-number + shell: bash + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + RUN_HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + run: | + set -euo pipefail + + pr_number="$(tr -d '[:space:]' < ./bench-results/NR)" + if ! [[ "$pr_number" =~ ^[1-9][0-9]*$ ]]; then + echo "artifact does not carry a pull request number" >&2 + exit 1 + fi + + claimed_head="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${pr_number}" --jq .head.sha)" + if [ "$claimed_head" != "$RUN_HEAD_SHA" ]; then + echo "pull request ${pr_number} is at ${claimed_head}, not the benchmarked ${RUN_HEAD_SHA}" >&2 + exit 1 + fi + + echo "pr_number=${pr_number}" >> "$GITHUB_OUTPUT" + + - name: Render results table + shell: bash + env: + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + run: | + set -euo pipefail + + # Benchmark names and units come from the fork's own bench source and + # end up inside a comment posted by the bot, so they are stripped of + # anything that could break out of a table cell. + + # Bencher lines look like: + # test connect_all/512 ... bench: 142019 cycles/iter (+/- 39534) + # Reduce both runs to name/value/unit/spread so they can be joined. + # Control characters are removed first because the fields are tab + # separated: a tab embedded in a benchmark name would otherwise shift + # every field after it. + normalize() { + tr -d '\000-\011\013-\037' < "$1" \ + | sed -nE 's@^test (.+) \.\.\. bench:[[:space:]]+([0-9]+) ([^ ]+) \(\+/- ([0-9]+)\)$@\1\t\2\t\3\t\4@p' + } + + normalize ./bench-results/results-base.txt > base.tsv + normalize ./bench-results/results-head.txt > head.tsv + + if [ ! -s head.tsv ]; then + echo "No benchmark lines could be parsed from the results artifact" >&2 + exit 1 + fi + + # A missing baseline is a case uefi-bench deliberately allows: a base + # commit that will not benchmark yields an empty file rather than a + # failed run. Say so, or every row reading "new" looks like this pull + # request added the whole suite. + baseline_note="" + if [ ! -s base.tsv ]; then + baseline_note="The base commit produced no benchmark results, so every benchmark below is reported as new and no change can be computed." + fi + + cat < comment.md + ## patina_boot benchmarks + + Base commit vs. this pull request at \`${HEAD_SHA}\`, benchmarked as merged into the base. + ${baseline_note} + + | Benchmark | Base | This PR | Change | Spread | + | --- | ---: | ---: | ---: | ---: | + EOF + + # The unit travels with each value so that a benchmark quietly + # switching its measurement shows up instead of being compared as if + # the numbers were still commensurable. + awk -F'\t' ' + function sanitize(text) { + # Neutralize Markdown table and code-span syntax, plus the angle + # brackets that would otherwise let a benchmark name inject raw + # HTML into the rendered comment. + gsub(/[|`<>\\]/, " ", text) + return text + } + # Selected by name rather than the usual NR == FNR: an empty base + # file consumes no records, which leaves NR == FNR true for the + # head file and silently files every head result as a baseline. + FILENAME == "base.tsv" { + if (!($1 in bval)) { border[++bcount] = $1 } + bval[$1] = $2; bunit[$1] = $3; bvar[$1] = $4 + next + } + { + name = $1; hval = $2; hunit = $3; hvar = $4 + seen[name] = 1 + label = sanitize(name) + if (!(name in bval)) { + printf "| `%s` | — | %s %s | new | ± %s |\n", \ + label, hval, sanitize(hunit), hvar + next + } + if (bunit[name] != hunit) { + printf "| `%s` | %s %s | %s %s | unit changed | ± %s |\n", \ + label, bval[name], sanitize(bunit[name]), hval, sanitize(hunit), hvar + next + } + diff = hval - bval[name] + pct = bval[name] > 0 ? diff / bval[name] * 100 : 0 + change = sprintf("%+.1f%%", pct) + magnitude = diff < 0 ? -diff : diff + # Both runs contribute noise, so the two spreads add. The same + # combined figure is printed, so the table shows the number the + # "within spread" decision was actually made against. + spread = bvar[name] + hvar + if (magnitude <= spread) { + change = change " (within spread)" + } + printf "| `%s` | %s %s | %s %s | %s | ± %s |\n", \ + label, bval[name], sanitize(bunit[name]), hval, sanitize(hunit), change, spread + } + END { + # Measured at the base commit but absent here: removed by this + # pull request. Emitted in base-file order so the table stays + # stable from run to run. + for (i = 1; i <= bcount; i++) { + name = border[i] + if (name in seen) { continue } + printf "| `%s` | %s %s | — | removed | — |\n", \ + sanitize(name), bval[name], sanitize(bunit[name]) + } + } + ' base.tsv head.tsv >> comment.md + + cat <<'EOF' >> comment.md + + Informational only; this does not gate the merge. Shared CI + runners are noisy, so treat a change marked "within spread" + as no signal and confirm anything surprising locally with + `cargo bench -p patina_boot` before acting on it. + + + EOF + + - name: Find existing comment + id: find-comment + uses: peter-evans/find-comment@v3 + with: + issue-number: ${{ steps.get-pr-number.outputs.pr_number }} + comment-author: 'github-actions[bot]' + body-includes: 'comment-tag: [uefi-bench]' + + # An empty comment-id creates a new comment; a populated one replaces the + # previous results so the pull request keeps a single, current comment. + - name: Post results + uses: peter-evans/create-or-update-comment@v4 + with: + comment-id: ${{ steps.find-comment.outputs.comment-id }} + issue-number: ${{ steps.get-pr-number.outputs.pr_number }} + body-path: comment.md + edit-mode: replace diff --git a/.github/workflows/uefi-bench.yml b/.github/workflows/uefi-bench.yml new file mode 100644 index 0000000..c9ca66e --- /dev/null +++ b/.github/workflows/uefi-bench.yml @@ -0,0 +1,114 @@ +# GitHub Actions: runs the patina_boot microbenchmarks on the pull request and +# on its base commit, and saves both result sets for the PR comment workflow. +# +# SPDX-License-Identifier: MIT +# + +# The same benches are run twice, at the base commit and at the merge result, +# so the comment can show a per-benchmark change rather than a bare number. +# +# Results are uploaded as an artifact rather than commented on from here: a +# pull_request run from a fork gets a read-only token and cannot post comments. +# The companion uefi-bench-pr-comment workflow does the writing. +# +# The numbers never gate the merge: nothing here compares them against a +# threshold. The job itself still fails if the benches will not build or +# produce no results, the same as any other check. + +permissions: + contents: read + +on: + pull_request: + paths: + - 'uefi/crates/patina_boot/**' + - '.github/workflows/uefi-bench.yml' + +concurrency: + # Keyed on the pull request number rather than the branch: fork branch names + # collide constantly (every fork has a `main`), and a bare branch key would + # let one pull request cancel another's run. + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: true + +name: uefi-bench + +jobs: + bench: + name: bench / patina_boot + runs-on: ubuntu-latest + steps: + # Full history: the base commit must be present locally to check out and + # benchmark it. + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + # `rust-toolchain.toml` pins the channel, components, and UEFI targets, + # but `rustup show` doesn't reliably install missing pieces in CI. The + # benches are host builds, so the UEFI targets are skipped here. Keep the + # channel in step with rust-toolchain.toml: benchmarking on a different + # channel than the rest of CI uses would measure a different compiler. + # + # The unstable features the Patina SDK needs are unlocked on stable by + # the RUSTC_BOOTSTRAP/allow-features settings in the crate's + # .cargo/config.toml, whose `[build]` section covers host builds and so + # covers the benches. + - name: Install Rust toolchain + uses: dtolnay/rust-toolchain@master + with: + toolchain: 1.95.0 + components: rust-src + + - name: Benchmark base commit + shell: bash + working-directory: uefi/crates/patina_boot + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + # Both bench runs write here. Kept outside the working tree so that + # switching commits between runs cannot disturb it. + mkdir -p "$RUNNER_TEMP/bench-results" + + # A failed checkout must abort. Left to continue, the step would + # benchmark the merge commit and upload it as the baseline, which + # renders as "no change" on every benchmark instead of as an error. + git checkout --quiet --detach "$BASE_SHA" + + # The base commit may not build, or may predate a bench, and that is + # not this pull request's problem: report a missing baseline rather + # than aborting the run. + if cargo bench --benches -- --output-format bencher > base-output.txt 2>&1; then + grep '^test ' base-output.txt > "$RUNNER_TEMP/bench-results/results-base.txt" || true + else + echo "Base commit $BASE_SHA failed to benchmark; reporting no baseline." >&2 + sed -n '1,40p' base-output.txt >&2 + : > "$RUNNER_TEMP/bench-results/results-base.txt" + fi + + - name: Benchmark pull request + shell: bash + working-directory: uefi/crates/patina_boot + env: + HEAD_SHA: ${{ github.sha }} + run: | + set -euo pipefail + git checkout --quiet --detach "$HEAD_SHA" + + cargo bench --benches -- --output-format bencher | tee head-output.txt + if ! grep '^test ' head-output.txt > "$RUNNER_TEMP/bench-results/results-head.txt"; then + echo "No benchmark result lines found in the bench output" >&2 + exit 1 + fi + + # A workflow_run payload carries no pull request number for pull + # requests from forks, so the comment workflow cannot look it up. + # Carry it across in the artifact. + echo "${{ github.event.number }}" > "$RUNNER_TEMP/bench-results/NR" + + - uses: actions/upload-artifact@v4 + with: + name: bench-results + path: ${{ runner.temp }}/bench-results/ + overwrite: true diff --git a/uefi/crates/patina_boot/Cargo.toml b/uefi/crates/patina_boot/Cargo.toml index 1ce400b..3d3bc01 100644 --- a/uefi/crates/patina_boot/Cargo.toml +++ b/uefi/crates/patina_boot/Cargo.toml @@ -16,6 +16,12 @@ keywords = ["uefi", "edk2", "patina", "boot", "firmware"] categories = ["embedded", "no-std"] readme = "README.md" +[lib] +# Excluded from `cargo bench` so `--benches` selects only the criterion +# targets. The lib unittest target is libtest and rejects criterion's +# arguments. +bench = false + [dependencies] log = { version = "0.4", default-features = false } patina = { version = "23", features = ["unstable-device-path"] } diff --git a/uefi/crates/patina_boot/benches/orchestrator.rs b/uefi/crates/patina_boot/benches/orchestrator.rs index 2f6daa4..1454439 100644 --- a/uefi/crates/patina_boot/benches/orchestrator.rs +++ b/uefi/crates/patina_boot/benches/orchestrator.rs @@ -7,6 +7,10 @@ //! Add `-- --output-format bencher` for libtest-style lines that //! standard perf-tracking tooling consumes. //! +//! Reports elapsed reference cycles (`rdtsc`) per iteration via the shared +//! [`support::Cycles`] measurement, matching the other benches in this crate +//! so every reported number shares one unit. +//! //! ## License //! //! Copyright (c) Microsoft Corporation. @@ -24,6 +28,9 @@ use patina::uefi::boot_services::{MockBootServices, boxed::BootServicesBox}; use patina_boot::helpers; use r_efi::efi; +#[path = "support/mod.rs"] +mod support; + /// Build a `MockBootServices` whose method expectations cover the /// sequence `connect_all` + `signal_bds_phase_entry` + /// `signal_ready_to_boot` exercise: `locate_handle_buffer`, @@ -84,7 +91,7 @@ fn build_mock() -> &'static MockBootServices { /// table with stub function pointers) that does not exist yet. /// Pending that, this composite is the closest end-to-end measurement /// of the BDS chain achievable against the public helper surface. -fn bds_phase_composite(c: &mut Criterion) { +fn bds_phase_composite(c: &mut Criterion) { let mock = build_mock(); let iter_count = AtomicUsize::new(0); @@ -100,5 +107,9 @@ fn bds_phase_composite(c: &mut Criterion) { }); } -criterion_group!(benches, bds_phase_composite); +criterion_group! { + name = benches; + config = Criterion::default().with_measurement(support::Cycles); + targets = bds_phase_composite +} criterion_main!(benches);