uefi-bench: Comment benchmark deltas on PRs - #209
Draft
kat-perez wants to merge 4 commits into
Draft
Conversation
Run the microbenchmarks at the base commit and on the pull request, then post both alongside a per-benchmark change so a reviewer has something to weigh a number against. This is a reviewer aid, not a gate: it never fails the build, and a change smaller than the combined run-to-run spread is labelled as such, because shared runners move these numbers more than most real changes do. The bench job uploads an artifact and a workflow_run job posts the comment, so pull requests from forks are covered despite their read-only token. Report the orchestrator bench in reference cycles like the other two. It was the only bench still using wall-clock time, which would have made the table compare values that are not commensurable. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kat-perez
requested review from
maxperga-msft,
philgweber,
rogurr and
williampMSFT
September 3, 2026 21:03
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The PR-comment workflow currently trusts artifact-provided PR metadata and the renderer omits removed benchmarks, which can lead to unsafe or misleading PR comments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds CI automation to run patina_boot Criterion microbenchmarks on PRs and post a single, continuously-updated PR comment showing base-vs-PR deltas, while also standardizing the orchestrator bench to report cycles/iter via the shared support::Cycles measurement.
Changes:
- Update
benches/orchestrator.rsto use the sharedsupport::Cyclesmeasurement so all benches report cycles. - Add
uefi-benchworkflow to run benchmarks twice (base SHA and PR head/merge SHA) and upload normalized result artifacts. - Add
uefi-bench-pr-commentworkflow (triggered viaworkflow_run) to render a delta table and create/update a single PR comment.
File summaries
| File | Description |
|---|---|
| uefi/crates/patina_boot/benches/orchestrator.rs | Switch orchestrator bench to support::Cycles measurement for unit consistency across benches. |
| .github/workflows/uefi-bench.yml | New PR workflow to benchmark base + PR, then upload results as an artifact. |
| .github/workflows/uefi-bench-pr-comment.yml | New workflow_run workflow to render artifact results and update a single PR comment. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+58
to
+60
| run: | | ||
| set -euo pipefail | ||
| echo "pr_number=$(cat ./bench-results/NR)" >> "$GITHUB_OUTPUT" |
Comment on lines
+97
to
+121
| NR == FNR { | ||
| bval[$1] = $2; bunit[$1] = $3; bvar[$1] = $4 | ||
| next | ||
| } | ||
| { | ||
| name = $1; hval = $2; hunit = $3; hvar = $4 | ||
| if (!(name in bval)) { | ||
| printf "| `%s` | — | %s %s | new | ± %s |\n", name, hval, hunit, hvar | ||
| next | ||
| } | ||
| if (bunit[name] != hunit) { | ||
| printf "| `%s` | %s %s | %s %s | unit changed | ± %s |\n", \ | ||
| name, bval[name], bunit[name], hval, 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 | ||
| if (magnitude <= bvar[name] + hvar) { | ||
| change = change " (within spread)" | ||
| } | ||
| printf "| `%s` | %s %s | %s %s | %s | ± %s |\n", \ | ||
| name, bval[name], bunit[name], hval, hunit, change, hvar | ||
| } |
| env: | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| run: | | ||
| set -uo pipefail |
Job-level env cannot use the runner context, so the workflow file failed to parse and GitHub reported a workflow file issue instead of running it. Use RUNNER_TEMP from the shell environment instead. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The benchmark workflows have a baseline-checkout robustness bug and the PR-comment renderer currently omits “removed” benchmarks, reducing correctness and usefulness of the reported deltas.
Review details
Suppressed comments (2)
.github/workflows/uefi-bench.yml:86
- In the base-commit benchmark step,
set -uo pipefaildoes not enable-e, so a failure ofgit checkout --detach "$BASE_SHA"would not stop the script and could accidentally benchmark the wrong commit while still producing a baseline file. Using-ekeeps the intended behavior (baseline may be empty ifcargo benchfails) while preventing silent checkout failures.
set -uo pipefail
git checkout --quiet --detach "$BASE_SHA"
.github/workflows/uefi-bench-pr-comment.yml:105
- The results renderer only iterates over
head.tsv, so benchmarks that exist in the base commit but were removed in this PR are omitted from the table. This contradicts the PR description’s claim that “benchmark removed” is handled, and it also hides potentially important signal (a removed bench is itself a meaningful change). Track which base benchmarks were seen inhead.tsvand emit a final “removed” row for any base-only entry.
{
name = $1; hval = $2; hunit = $3; hvar = $4
if (!(name in bval)) {
printf "| `%s` | — | %s %s | new | ± %s |\n", name, hval, hunit, hvar
next
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
cargo bench --benches also runs the lib unittest target, which is libtest and rejects criterion's --output-format. Enumerate the bench targets from cargo metadata and name them, so the run also works on base commits that predate a given bench. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The PR-comment rendering pipeline needs hardening (sanitize artifact-derived fields and handle removed benchmarks) and the workflows have reliability issues (base-step error handling and concurrency key collisions).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/uefi-bench-pr-comment.yml:34
- The concurrency group key is based on
workflow_run.head_branch, which is not guaranteed to be unique across PRs (e.g., multiple forks commonly use the same branch name likemain/feature), so unrelated runs can cancel each other and prevent comments from being posted/updated. Use the PR number (available on theworkflow_runpayload) as the concurrency key so updates only cancel within the same PR.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+98
to
+122
| bval[$1] = $2; bunit[$1] = $3; bvar[$1] = $4 | ||
| next | ||
| } | ||
| { | ||
| name = $1; hval = $2; hunit = $3; hvar = $4 | ||
| if (!(name in bval)) { | ||
| printf "| `%s` | — | %s %s | new | ± %s |\n", name, hval, hunit, hvar | ||
| next | ||
| } | ||
| if (bunit[name] != hunit) { | ||
| printf "| `%s` | %s %s | %s %s | unit changed | ± %s |\n", \ | ||
| name, bval[name], bunit[name], hval, 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 | ||
| if (magnitude <= bvar[name] + hvar) { | ||
| change = change " (within spread)" | ||
| } | ||
| printf "| `%s` | %s %s | %s %s | %s | ± %s |\n", \ | ||
| name, bval[name], bunit[name], hval, hunit, change, hvar | ||
| } | ||
| ' base.tsv head.tsv >> comment.md |
Comment on lines
+85
to
+87
| set -uo pipefail | ||
| git checkout --quiet --detach "$BASE_SHA" | ||
|
|
kat-perez
marked this pull request as draft
September 3, 2026 21:35
Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kat-perez
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #122
What
Runs the
patina_bootmicrobenchmarks on every PR that touches the crate, and posts the results as a single PR comment that is updated in place.The benches are run twice — once at the PR's base commit, once at the merge result — so the comment shows a per-benchmark change rather than a bare number a reviewer has nothing to weigh against.
This is a reviewer aid, not a gate
It never fails the build. Shared CI runners move these numbers more than most real changes do, so a change smaller than the combined run-to-run spread is labelled
(within spread)and should be read as no signal. The point is to make an unexpected swing visible during review on PRs topatina_boot, not to block merges.Regression gating is deliberately out of scope here (#123).
Example output
connect_all/1connect_all/16expand_device_path/1bds_phase_compositenew_benchFork safety
uefi-benchruns in the PR context, which is read-only for fork PRs, so it uploads an artifact.uefi-bench-pr-commenttriggers onworkflow_runin the base-repo context and does the writing. Same split the existingcargo-vet/cargo-vet-pr-commentpair already uses.Unit fix included
benches/orchestrator.rswas the only bench still reporting wall-clockns/iter; the other two reportcycles/itervia the sharedsupport::Cyclesmeasurement. Left alone, the table would have compared values that are not commensurable. It now reports cycles like the rest.You can see this working on this very PR:
bds_phase_compositewill render asunit changed, since base is still onns/iter.Validation
cargo fmt --check,cargo clippy --all-targets -- -D warnings,cargo test(94 passed) all cleanNote on
patina_bootbench coverageThe end-to-end
BootOrchestrator::execute()bench (#124) is still blocked — it needs aStandardBootServicesfactory that OpenDevicePartnership/patina#1743 removes, per #202. This PR wires up the benches that exist today; #124 drops into the same table when it lands.