From 6f737256dc7124b3fbddc106927277fbf9e21ddf Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Wed, 7 Oct 2026 14:33:22 +0100 Subject: [PATCH 1/6] Bump criterion-markdown to 0.2.2 A report shows criterion's typical estimate, the regression slope wherever linear sampling fits one, while the change beside it read the mean. The mean weights every sample alike and so carries the warm-up the slope leaves behind, which read as a regression on benchmarks whose first samples ran cold. 0.2.2 compares the estimate it displays, and holds a verdict back until the whole confidence interval clears its threshold. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jorge Prendes --- Cargo.lock | 4 ++-- src/hyperlight_ci/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3f365ed41..1288f7563 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -673,9 +673,9 @@ dependencies = [ [[package]] name = "criterion-markdown" -version = "0.2.1" +version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0c723767825fca388c4d768dfbe983d7ba3f7e0b3104292f85c8ca8bd6880822" +checksum = "3d6e13f76f7a56c31550cfa23e2c9d4f312afe96f51667c244a6402695b672e8" dependencies = [ "anyhow", "serde", diff --git a/src/hyperlight_ci/Cargo.toml b/src/hyperlight_ci/Cargo.toml index 087bcdd5f..3f2ab8816 100644 --- a/src/hyperlight_ci/Cargo.toml +++ b/src/hyperlight_ci/Cargo.toml @@ -12,7 +12,7 @@ workspace = true [dependencies] anyhow = "1" clap = { version = "4.6.1", features = ["derive"] } -criterion-markdown = "0.2.1" +criterion-markdown = "0.2.2" criterion-swarm = "0.2.2" regex = "1" serde = { version = "1.0", features = ["derive"] } From 89933682a4225d514efb3be45272e0ed1dad9bc9 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Wed, 7 Oct 2026 14:33:36 +0100 Subject: [PATCH 2/6] Measure the branch point beside the pull request A pull request was measured against the daily run its branch point carried. That run came from another machine, and the pools hold more than one processor model per vendor and are shared with whatever else is scheduled there. The difference between two runners read as a difference between two commits: one report announced a 1.58x regression for a change that touches only CI tooling, from a baseline that happened to land on faster silicon. `cargo ci bench --baseline-ref` measures a second commit before the one checked out, in a git worktree so the checkout in hand is never moved and a run that fails leaves nothing to restore. Each benchmark binary resolves its guests relative to its own source tree, so the baseline reads the worktree and the current tree keeps its own, and criterion compares the two from one directory. The guests the baseline needs are built by the job that already has the toolchain for them. `HEAD^1` names the branch point because the checkout is the merge its validation builds, which both jobs assert before relying on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jorge Prendes --- .github/workflows/ValidatePullRequest.yml | 28 +++- .github/workflows/dep_benchmarks.yml | 44 ++++- .github/workflows/dep_build_guests.yml | 37 +++- Justfile | 4 + src/hyperlight_ci/src/baseline.rs | 196 ++++++++++++++++++++++ src/hyperlight_ci/src/bench.rs | 131 ++++++++++++++- src/hyperlight_ci/src/bench_report.rs | 16 +- src/hyperlight_ci/src/main.rs | 1 + src/hyperlight_ci/src/manifest.rs | 2 +- 9 files changed, 431 insertions(+), 28 deletions(-) create mode 100644 src/hyperlight_ci/src/baseline.rs diff --git a/.github/workflows/ValidatePullRequest.yml b/.github/workflows/ValidatePullRequest.yml index e393a69ed..da68d480f 100644 --- a/.github/workflows/ValidatePullRequest.yml +++ b/.github/workflows/ValidatePullRequest.yml @@ -66,6 +66,26 @@ jobs: arch: ${{ matrix.arch }} config: ${{ matrix.config }} + # Guests for the commit the pull request branched from, so the benchmarks + # can measure both sides on one machine. Only the configuration the + # benchmark suite loads is needed. + build-baseline-guests: + needs: [docs-pr, update-guest-locks] + # Required because update-guest-locks is skipped on non-dependabot PRs, + # and a skipped dependency transitively skips all downstream jobs. + # See: https://github.com/actions/runner/issues/2205 + if: ${{ !cancelled() && !failure() }} + uses: ./.github/workflows/dep_build_guests.yml + secrets: inherit + with: + docs_only: ${{ needs.docs-pr.outputs.docs-only }} + arch: X64 + config: release + # The checkout is the merge this run validates, so its first parent + # is the commit the pull request branched from. + ref: HEAD^1 + artifact_suffix: -baseline + # Code checks (fmt, clippy, MSRV) - runs in parallel with build-guests code-checks: needs: [docs-pr, update-guest-locks] @@ -232,6 +252,7 @@ jobs: needs: - docs-pr - build-guests + - build-baseline-guests # Required because update-guest-locks is skipped on non-dependabot PRs, # and a skipped dependency transitively skips all downstream jobs. # See: https://github.com/actions/runner/issues/2205 @@ -249,6 +270,7 @@ jobs: hypervisor: ${{ matrix.hypervisor }} cpu_vendor: ${{ matrix.cpu_vendor }} arch: ${{ matrix.arch }} + baseline_ref: HEAD^1 # Report every configuration the run measured against the branch point the # pull request was built from, for the hyperlight-gh-bot to post as a PR @@ -277,9 +299,10 @@ jobs: run: | echo '## Benchmark Results' > pr-comment.md echo '' >> pr-comment.md + # No baseline is named: each run measured the branch point alongside + # the pull request, so the comparison it carries is the one to read. cargo ci bench-report --config-file bench_report.toml --reproduce \ - --candidate run:${{ github.run_id }} \ - --baseline base-of:${{ github.event.pull_request.number }} >> pr-comment.md + --candidate run:${{ github.run_id }} >> pr-comment.md env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -314,6 +337,7 @@ jobs: - docs-pr - update-guest-locks - build-guests + - build-baseline-guests - code-checks - check-golden-label - build-test diff --git a/.github/workflows/dep_benchmarks.yml b/.github/workflows/dep_benchmarks.yml index 5e54ecbb3..1fb6bc838 100644 --- a/.github/workflows/dep_benchmarks.yml +++ b/.github/workflows/dep_benchmarks.yml @@ -31,6 +31,14 @@ on: description: CPU architecture for the build, X64 or arm64 (passed from caller matrix) required: true type: string + baseline_ref: + description: >- + Commit to measure these results against. When set, its benchmarks run + first on this machine, so the comparison reflects the change rather + than the difference between two runners. + required: false + type: string + default: "" env: CARGO_TERM_COLOR: always RUST_BACKTRACE: full @@ -57,6 +65,9 @@ jobs: || '["self-hosted", "Linux", "arm64", "kvm"]') }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # The baseline pass checks out another commit, which needs its history. + fetch-depth: ${{ inputs.baseline_ref == '' && 1 || 0 }} - uses: hyperlight-dev/ci-setup-workflow@2f4142ba17cf573af44fc1e1f1ffc743daded5b3 # v1.10.0 with: @@ -90,17 +101,44 @@ jobs: name: c-guests-${{ inputs.arch }}-release path: src/tests/c_guests/bin/release/ + - name: Download baseline Rust guests + if: inputs.baseline_ref != '' + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: rust-guests-${{ inputs.arch }}-release-baseline + path: ${{ runner.temp }}/baseline-guests/rust/ + + - name: Download baseline C guests + if: inputs.baseline_ref != '' + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: c-guests-${{ inputs.arch }}-release-baseline + path: ${{ runner.temp }}/baseline-guests/c/ + + # Both commits are measured on this machine, so the comparison reflects + # them rather than the difference between two runners. `HEAD^1` is the + # branch point only because the checkout is the merge this run + # validates, so anything else would measure against the wrong commit. - name: Build run: just build release - name: Run benchmarks - run: just bench-ci main + run: | + if [ -n "${{ inputs.baseline_ref }}" ]; then + git rev-parse --verify --quiet HEAD^2 >/dev/null || { + echo "::error::HEAD is not a merge, so ${{ inputs.baseline_ref }} is not the branch point" + exit 1 + } + just bench-against ${{ inputs.baseline_ref }} "$RUNNER_TEMP/baseline-guests" + else + just bench-ci main + fi - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: benchmarks_${{ runner.os }}_${{ inputs.hypervisor }}_${{ inputs.cpu_vendor }} path: ./target/criterion/ if-no-files-found: error - # The longest GitHub keeps an artifact, so that a pull request can be - # compared against the branch point it was built from. + # The longest GitHub keeps an artifact, so that a run stays readable + # for as long as the history it contributes to. retention-days: 90 diff --git a/.github/workflows/dep_build_guests.yml b/.github/workflows/dep_build_guests.yml index c8da39b25..f818e71eb 100644 --- a/.github/workflows/dep_build_guests.yml +++ b/.github/workflows/dep_build_guests.yml @@ -18,6 +18,18 @@ on: description: Build configuration (debug or release) required: true type: string + ref: + description: >- + Commit to build the guests from, as a revision git resolves against + the checkout. Defaults to the triggering ref. + required: false + type: string + default: "" + artifact_suffix: + description: Appended to the artifact names, to keep separate builds apart. + required: false + type: string + default: "" env: CARGO_TERM_COLOR: always @@ -35,15 +47,32 @@ jobs: timeout-minutes: ${{ fromJSON(vars.BUILD_GUESTS_JOB_TIMEOUT || '15') }} # Spread load across vendor pools: AMD when run_id ends in 0,2,4,6,8, Intel otherwise. runs-on: ${{ fromJson(inputs.arch == 'X64' - && format('["self-hosted", "Linux", "X64", "1ES.Pool=hld-kvm-{0}", "JobId=build-guests-{1}-{2}-{3}-{4}"]', + && format('["self-hosted", "Linux", "X64", "1ES.Pool=hld-kvm-{0}", "JobId=build-guests-{1}{5}-{2}-{3}-{4}"]', (endsWith(github.run_id, '0') || endsWith(github.run_id, '2') || endsWith(github.run_id, '4') || endsWith(github.run_id, '6') || endsWith(github.run_id, '8')) && 'amd' || 'intel', inputs.config, github.run_id, github.run_number, - github.run_attempt) + github.run_attempt, + inputs.artifact_suffix) || '["self-hosted", "Linux", "arm64"]') }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # Resolving a revision needs the history it names. + fetch-depth: ${{ inputs.ref == '' && 1 || 0 }} + + # `HEAD^1` is where a pull request branched: the checkout is the merge + # its validation builds, and its first parent is the branch it targets. + # On anything but a merge `HEAD^1` is the previous commit, which would + # build guests for the wrong one without saying so. + - name: Check out the commit to build from + if: inputs.ref != '' + run: | + git rev-parse --verify --quiet HEAD^2 >/dev/null || { + echo "::error::HEAD is not a merge, so ${{ inputs.ref }} is not the branch point" + exit 1 + } + git checkout --detach ${{ inputs.ref }} - uses: hyperlight-dev/ci-setup-workflow@2f4142ba17cf573af44fc1e1f1ffc743daded5b3 # v1.10.0 with: @@ -103,7 +132,7 @@ jobs: - name: Upload Rust guests uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: - name: rust-guests-${{ inputs.arch }}-${{ inputs.config }} + name: rust-guests-${{ inputs.arch }}-${{ inputs.config }}${{ inputs.artifact_suffix }} path: | src/tests/rust_guests/bin/${{ inputs.config }}/ retention-days: 1 @@ -112,7 +141,7 @@ jobs: - name: Upload C guests uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: - name: c-guests-${{ inputs.arch }}-${{ inputs.config }} + name: c-guests-${{ inputs.arch }}-${{ inputs.config }}${{ inputs.artifact_suffix }} path: src/tests/c_guests/bin/${{ inputs.config }}/ retention-days: 1 if-no-files-found: error diff --git a/Justfile b/Justfile index 62c8c1813..37d07862a 100644 --- a/Justfile +++ b/Justfile @@ -462,6 +462,10 @@ tar-static-lib: (build-rust-capi "release") (build-rust-capi "debug") bench-ci baseline features="": cargo ci bench {{ if features == "" {''} else { "--features " + features } }} --verbose --save-baseline {{ baseline }} +# Measure a commit and the current tree on this machine, and compare them +bench-against commit guests features="": + cargo ci bench {{ if features == "" {''} else { "--features " + features } }} --baseline-ref {{ commit }} --baseline-guests {{ guests }} --verbose + bench features="": cargo ci bench {{ if features == "" {''} else { "--features " + features } }} --verbose diff --git a/src/hyperlight_ci/src/baseline.rs b/src/hyperlight_ci/src/baseline.rs new file mode 100644 index 000000000..99cf410b2 --- /dev/null +++ b/src/hyperlight_ci/src/baseline.rs @@ -0,0 +1,196 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2025 The Hyperlight Authors. +//! Measuring a second commit alongside the one checked out. +//! +//! A pull request is measured against where it branched from. Reading that +//! baseline from an earlier CI run compares two machines as much as two +//! commits, and the runners differ enough in processor and tenancy to show up +//! as a change. Measuring both here leaves only the commits between them. +//! +//! The baseline is built in a [git worktree], so the checkout in hand is never +//! moved and a run that fails leaves nothing to restore. Each benchmark binary +//! resolves its guests relative to its own source tree, so the baseline finds +//! the guests placed in the worktree and the current tree keeps its own. +//! +//! [git worktree]: https://git-scm.com/docs/git-worktree + +use std::ffi::OsStr; +use std::path::{Path, PathBuf}; +use std::process::Command; + +use anyhow::{Context, Result, bail}; + +/// Where the baseline checkout goes, under the build directory so that it is +/// ignored by git and removed by `cargo clean`. +const WORKTREE_DIR: &str = "target/baseline-worktree"; + +/// The guest binaries a benchmark run loads, as the directory each is +/// published and read from. +const GUEST_DIRS: [(&str, &str); 2] = [ + ("rust", "src/tests/rust_guests/bin/release"), + ("c", "src/tests/c_guests/bin/release"), +]; + +/// A checkout of another commit, removed when it goes out of scope. +pub(crate) struct Worktree { + path: PathBuf, +} + +impl Worktree { + /// Check `commit` out beside the current tree. + pub(crate) fn add(commit: &str) -> Result { + let path = PathBuf::from(WORKTREE_DIR); + + // A worktree left by an interrupted run holds the path and would + // otherwise be reported as already in use. + let _ = remove(&path); + + git(["worktree", "add", "--detach", WORKTREE_DIR, commit]) + .with_context(|| format!("Failed to check out {commit} to compare against"))?; + + Ok(Self { path }) + } + + pub(crate) fn path(&self) -> &Path { + &self.path + } + + /// Copy the guests of the commit this worktree holds into it. + /// + /// `source` holds one directory per guest language, named as + /// [`GUEST_DIRS`] names them. + pub(crate) fn place_guests(&self, source: &Path) -> Result<()> { + for (name, destination) in GUEST_DIRS { + let from = source.join(name); + if !from.is_dir() { + bail!( + "{} holds no `{name}` directory, so the baseline has no {name} guests to run", + source.display() + ); + } + copy_dir(&from, &self.path.join(destination))?; + } + Ok(()) + } +} + +impl Drop for Worktree { + fn drop(&mut self) { + // The run is over either way, so a worktree that will not go reports + // itself rather than failing what already finished. + if let Err(error) = remove(&self.path) { + eprintln!("{error:#}"); + } + } +} + +fn remove(path: &Path) -> Result<()> { + if !path.exists() { + return Ok(()); + } + git([ + OsStr::new("worktree"), + OsStr::new("remove"), + OsStr::new("--force"), + path.as_os_str(), + ]) + .with_context(|| format!("Failed to remove the worktree at {}", path.display())) +} + +fn git(args: I) -> Result<()> +where + I: IntoIterator, + S: AsRef, +{ + let output = Command::new("git") + .args(args) + .output() + .context("Failed to run git")?; + + if !output.status.success() { + bail!("{}", String::from_utf8_lossy(&output.stderr).trim()); + } + Ok(()) +} + +/// Replace `destination` with the contents of `source`. +fn copy_dir(source: &Path, destination: &Path) -> Result<()> { + if destination.exists() { + std::fs::remove_dir_all(destination) + .with_context(|| format!("Failed to clear {}", destination.display()))?; + } + std::fs::create_dir_all(destination) + .with_context(|| format!("Failed to create {}", destination.display()))?; + + for entry in + std::fs::read_dir(source).with_context(|| format!("Failed to read {}", source.display()))? + { + let entry = entry.context("Failed to read a directory entry")?; + let target = destination.join(entry.file_name()); + + if entry.file_type()?.is_dir() { + copy_dir(&entry.path(), &target)?; + } else { + std::fs::copy(entry.path(), &target).with_context(|| { + format!( + "Failed to copy {} to {}", + entry.path().display(), + target.display() + ) + })?; + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn copying_replaces_whatever_the_destination_held() { + let root = std::env::temp_dir().join(format!("hl-baseline-{}", std::process::id())); + let (source, destination) = (root.join("from"), root.join("to")); + + std::fs::create_dir_all(source.join("nested")).unwrap(); + std::fs::write(source.join("simpleguest"), b"new").unwrap(); + std::fs::write(source.join("nested").join("inner"), b"deep").unwrap(); + + std::fs::create_dir_all(&destination).unwrap(); + std::fs::write(destination.join("stale"), b"old").unwrap(); + + copy_dir(&source, &destination).unwrap(); + + assert_eq!( + std::fs::read(destination.join("simpleguest")).unwrap(), + b"new" + ); + assert_eq!( + std::fs::read(destination.join("nested").join("inner")).unwrap(), + b"deep" + ); + assert!( + !destination.join("stale").exists(), + "guests of another commit must not survive" + ); + + std::fs::remove_dir_all(root).unwrap(); + } + + #[test] + fn missing_guests_are_reported_against_the_directory_given() { + let root = std::env::temp_dir().join(format!("hl-empty-{}", std::process::id())); + std::fs::create_dir_all(&root).unwrap(); + + let worktree = Worktree { + path: root.join("worktree"), + }; + let error = worktree.place_guests(&root).unwrap_err().to_string(); + + assert!(error.contains("rust"), "{error}"); + + // Leaves nothing behind: the worktree was never created. + std::mem::forget(worktree); + std::fs::remove_dir_all(root).unwrap(); + } +} diff --git a/src/hyperlight_ci/src/bench.rs b/src/hyperlight_ci/src/bench.rs index dafcb7522..0e2f38829 100644 --- a/src/hyperlight_ci/src/bench.rs +++ b/src/hyperlight_ci/src/bench.rs @@ -10,8 +10,10 @@ use anyhow::Context; use criterion_swarm::{CriterionSwarm, OutputMode}; use crate::ballast::Ballast; +use crate::baseline::Worktree; +use crate::bench_report::DEFAULT_REPO; use crate::config::BenchConfig; -use crate::manifest; +use crate::{manifest, remote}; /// An output mode flag for `--build-output` / `--benchmarks-output`. #[derive(Clone, Debug)] @@ -79,12 +81,98 @@ pub struct BenchArgs { #[arg(long)] pub no_ballast: bool, + /// Measure this commit first and compare the run against it. + /// + /// Takes anything git resolves to a commit, a branch, tag or sha, or + /// `base-of:` for where a pull request branched. Both passes run on + /// this machine, so the comparison reflects the commits rather than the + /// difference between two runners. + #[arg(long, value_name = "COMMIT", requires = "baseline_guests")] + pub baseline_ref: Option, + + /// Repository a `base-of:` baseline is read from, `/` or + /// `remote:` for whichever one a git remote points at + #[arg(long, value_name = "REPO")] + pub repo: Option, + + /// Guest binaries built from `--baseline-ref`, as a directory holding one + /// `rust` and one `c` directory. + #[arg(long, value_name = "DIR")] + pub baseline_guests: Option, + + /// Name criterion keeps the `--baseline-ref` results under + #[arg(long, value_name = "NAME", default_value = "base")] + pub baseline_name: String, + /// Additional arguments to forward to criterion benchmarks #[arg(trailing_var_arg = true, allow_hyphen_values = true)] pub bench_args: Vec, } pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { + let Some(spec) = args.baseline_ref.take() else { + let bench_args = std::mem::take(&mut args.bench_args); + return measure(&args, bench_args).await; + }; + + let commit = resolve(&spec, args.repo.as_deref())?; + // Each pass runs from its own checkout, so the directory they write to is + // named outright rather than found relative to wherever that is. + let home = std::path::absolute(manifest::criterion_dir()) + .context("Failed to resolve where criterion keeps its results")?; + std::fs::create_dir_all(&home) + .with_context(|| format!("Failed to create {}", home.display()))?; + std::env::set_var("CRITERION_HOME", &home); + + let guests = args + .baseline_guests + .clone() + .expect("clap requires guests alongside a baseline commit"); + let worktree = Worktree::add(&commit)?; + worktree.place_guests(&guests)?; + + // A benchmark binary finds its guests relative to the source it was built + // from, so the baseline reads the worktree and this tree keeps its own. + let here = std::env::current_dir().context("Failed to read the working directory")?; + std::env::set_current_dir(worktree.path()) + .with_context(|| format!("Failed to enter {}", worktree.path().display()))?; + let measured = measure(&args, with(&args, "--save-baseline")).await; + std::env::set_current_dir(&here) + .with_context(|| format!("Failed to return to {}", here.display()))?; + measured.with_context(|| format!("Failed to measure {commit}"))?; + + drop(worktree); + + measure(&args, with(&args, "--baseline")).await +} + +/// The commit a baseline names. +/// +/// `base-of:` is where a pull request branched, read the way +/// `bench-report` reads the same spelling. Anything else is left to git, which +/// already understands branches, tags, shas and the revisions built from them. +fn resolve(spec: &str, repo: Option<&str>) -> anyhow::Result { + let Some(pull_request) = spec.strip_prefix("base-of:") else { + return Ok(spec.to_string()); + }; + + let pull_request = pull_request + .parse() + .map_err(|_| anyhow::anyhow!("`{pull_request}` is not a pull request number"))?; + let repo = remote::repository(repo.unwrap_or(DEFAULT_REPO))?; + + remote::merge_base_of(&repo, pull_request) +} + +/// The criterion arguments for one pass, naming the baseline it reads or writes. +fn with(args: &BenchArgs, flag: &str) -> Vec { + let mut bench_args = args.bench_args.clone(); + bench_args.push(flag.to_string()); + bench_args.push(args.baseline_name.clone()); + bench_args +} + +async fn measure(args: &BenchArgs, bench_args: Vec) -> anyhow::Result<()> { let config = args .config_file .as_deref() @@ -94,37 +182,40 @@ pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { let mut swarm = CriterionSwarm::builder().jobs(args.jobs); if !args.binary.is_empty() { - swarm = swarm.binaries(args.binary); + swarm = swarm.binaries(args.binary.clone()); } if !args.features.is_empty() { swarm = swarm.build_args(["--features".to_string(), args.features.join(",")]); } - for arg in args.bench_args { + for arg in bench_args { swarm = swarm.bench_arg(arg); } - if args.build_output.is_empty() { + let mut build_output = args.build_output.clone(); + let mut benchmarks_output = args.benchmarks_output.clone(); + + if build_output.is_empty() { let mode = if std::io::stderr().is_terminal() { OutputMode::SPINNER | OutputMode::SUMMARY } else { OutputMode::STREAM | OutputMode::SUMMARY }; - args.build_output.push(OutputModeFlags(mode)); + build_output.push(OutputModeFlags(mode)); } - if args.benchmarks_output.is_empty() { + if benchmarks_output.is_empty() { let mode = if std::io::stderr().is_terminal() { OutputMode::SPINNER | OutputMode::STREAM | OutputMode::SUMMARY } else { OutputMode::STREAM | OutputMode::SUMMARY }; - args.benchmarks_output.push(OutputModeFlags(mode)); + benchmarks_output.push(OutputModeFlags(mode)); } - let build_mode = merge_output_modes(&args.build_output); - let bench_mode = merge_output_modes(&args.benchmarks_output); + let build_mode = merge_output_modes(&build_output); + let bench_mode = merge_output_modes(&benchmarks_output); swarm = swarm.output( criterion_swarm::ProgressReporter::new() .build(build_mode) @@ -169,3 +260,25 @@ pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { manifest::write(benchmarks).context("Failed to write the benchmark manifest") } + +#[cfg(test)] +mod tests { + use super::resolve; + + /// git already reads branches, tags, shas and the revisions built from + /// them, so a baseline names one directly. + #[test] + fn a_commit_is_left_to_git() { + for spec in ["main", "HEAD^1", "3ee2d4cb", "v1.2.3"] { + assert_eq!(resolve(spec, None).unwrap(), spec); + } + } + + #[test] + fn a_pull_request_without_a_number_is_reported() { + let error = resolve("base-of:not-a-number", None) + .unwrap_err() + .to_string(); + assert!(error.contains("not a pull request number"), "{error}"); + } +} diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs index 643cc8368..bc50868c8 100644 --- a/src/hyperlight_ci/src/bench_report.rs +++ b/src/hyperlight_ci/src/bench_report.rs @@ -18,7 +18,7 @@ use crate::{manifest, remote}; const RUN_CACHE: &str = "target/ci-runs"; /// Whose runs a report reads, when nothing else says. -const DEFAULT_REPO: &str = "hyperlight-dev/hyperlight"; +pub(crate) const DEFAULT_REPO: &str = "hyperlight-dev/hyperlight"; /// Where results come from, either a criterion directory or CI. #[derive(Clone)] @@ -151,8 +151,8 @@ pub struct BenchReportArgs { pub candidate: Source, /// Results to compare against, in the same forms as the candidate. Defaults - /// to where a pull request branched, and otherwise to the previous run held - /// in the reported directory. + /// to the baseline the reported results carry, which a pull request run + /// measures alongside them. #[arg(long, value_name = "SOURCE")] pub baseline: Option, @@ -218,12 +218,10 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { let candidate = resolve(&args.candidate, &repo)?; - // Nothing within a pull request's results says what they mean, so they are - // measured against the branch point they were built from. - let source = args.baseline.clone().or(match &args.candidate { - Source::PullRequest(pull_request) => Some(Source::BaseOf(*pull_request)), - _ => None, - }); + // A run measures the commit a pull request branched from alongside the + // pull request itself, so its results carry the comparison with them and + // a baseline is only read when one is named. + let source = args.baseline.clone(); let mut baseline = Origin { commit: None, diff --git a/src/hyperlight_ci/src/main.rs b/src/hyperlight_ci/src/main.rs index 5c37698e8..c61234f27 100644 --- a/src/hyperlight_ci/src/main.rs +++ b/src/hyperlight_ci/src/main.rs @@ -1,6 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2025 The Hyperlight Authors. mod ballast; +mod baseline; mod bench; mod bench_report; mod config; diff --git a/src/hyperlight_ci/src/manifest.rs b/src/hyperlight_ci/src/manifest.rs index 28af0bd45..376656730 100644 --- a/src/hyperlight_ci/src/manifest.rs +++ b/src/hyperlight_ci/src/manifest.rs @@ -44,7 +44,7 @@ pub(crate) struct Host { } /// Where criterion keeps its results. -fn criterion_dir() -> PathBuf { +pub(crate) fn criterion_dir() -> PathBuf { env::var_os("CRITERION_HOME") .map(PathBuf::from) .unwrap_or_else(|| PathBuf::from("target").join("criterion")) From 68060fd2814b0158733b7b702541a6f7819d8216 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Wed, 7 Oct 2026 15:38:16 +0100 Subject: [PATCH 3/6] Address review of the paired benchmark pass A commit may add a benchmark the one it is measured against never had, which criterion's strict `--baseline` treats as a failure, so every pull request that extends the suite would fail its benchmark job. The comparison reads leniently now, and the baseline it is about to write is cleared first, so a copy left by an earlier run cannot stand in for a benchmark this one could not measure. A configuration named by a relative path was read after entering the worktree, from whatever the commit being measured carried at the same path. It is resolved before either pass runs. A pre-built `--binary` holds one commit and cannot represent two, so it is refused alongside a baseline rather than measuring the same executable twice. Worktrees are pruned before one is added, since `cargo clean` takes the directory while leaving the registration behind. `HEAD^1` is the commit a pull request merges into rather than where it branched, which is what the comparison wants and what the comments now say. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jorge Prendes --- .github/workflows/ValidatePullRequest.yml | 21 ++++--- .github/workflows/dep_benchmarks.yml | 6 +- .github/workflows/dep_build_guests.yml | 10 +-- src/hyperlight_ci/src/baseline.rs | 6 +- src/hyperlight_ci/src/bench.rs | 76 +++++++++++++++++++++-- 5 files changed, 95 insertions(+), 24 deletions(-) diff --git a/.github/workflows/ValidatePullRequest.yml b/.github/workflows/ValidatePullRequest.yml index da68d480f..4173cf416 100644 --- a/.github/workflows/ValidatePullRequest.yml +++ b/.github/workflows/ValidatePullRequest.yml @@ -66,9 +66,9 @@ jobs: arch: ${{ matrix.arch }} config: ${{ matrix.config }} - # Guests for the commit the pull request branched from, so the benchmarks - # can measure both sides on one machine. Only the configuration the - # benchmark suite loads is needed. + # Guests for the commit the pull request merges into, so the benchmarks can + # measure both sides on one machine. Only the configuration the benchmark + # suite loads is needed. build-baseline-guests: needs: [docs-pr, update-guest-locks] # Required because update-guest-locks is skipped on non-dependabot PRs, @@ -81,8 +81,8 @@ jobs: docs_only: ${{ needs.docs-pr.outputs.docs-only }} arch: X64 config: release - # The checkout is the merge this run validates, so its first parent - # is the commit the pull request branched from. + # The checkout is the merge this run validates, so its first parent is + # the commit the pull request merges into. ref: HEAD^1 artifact_suffix: -baseline @@ -272,9 +272,9 @@ jobs: arch: ${{ matrix.arch }} baseline_ref: HEAD^1 - # Report every configuration the run measured against the branch point the - # pull request was built from, for the hyperlight-gh-bot to post as a PR - # comment. Only runs for PRs (not merge groups) with code changes. + # Report every configuration the run measured against the commit the pull + # request merges into, for the hyperlight-gh-bot to post as a PR comment. + # Only runs for PRs (not merge groups) with code changes. benchmark-comment: name: post-benchmark-comment needs: @@ -299,8 +299,9 @@ jobs: run: | echo '## Benchmark Results' > pr-comment.md echo '' >> pr-comment.md - # No baseline is named: each run measured the branch point alongside - # the pull request, so the comparison it carries is the one to read. + # No baseline is named: each run measured the commit it merges into + # alongside the pull request, so the comparison it carries is the one + # to read. cargo ci bench-report --config-file bench_report.toml --reproduce \ --candidate run:${{ github.run_id }} >> pr-comment.md env: diff --git a/.github/workflows/dep_benchmarks.yml b/.github/workflows/dep_benchmarks.yml index 1fb6bc838..4a2c446c4 100644 --- a/.github/workflows/dep_benchmarks.yml +++ b/.github/workflows/dep_benchmarks.yml @@ -117,8 +117,8 @@ jobs: # Both commits are measured on this machine, so the comparison reflects # them rather than the difference between two runners. `HEAD^1` is the - # branch point only because the checkout is the merge this run - # validates, so anything else would measure against the wrong commit. + # commit the pull request merges into, because the checkout is the merge + # this run validates, so anything else would measure the wrong commit. - name: Build run: just build release @@ -126,7 +126,7 @@ jobs: run: | if [ -n "${{ inputs.baseline_ref }}" ]; then git rev-parse --verify --quiet HEAD^2 >/dev/null || { - echo "::error::HEAD is not a merge, so ${{ inputs.baseline_ref }} is not the branch point" + echo "::error::HEAD is not a merge, so ${{ inputs.baseline_ref }} is not what it merges into" exit 1 } just bench-against ${{ inputs.baseline_ref }} "$RUNNER_TEMP/baseline-guests" diff --git a/.github/workflows/dep_build_guests.yml b/.github/workflows/dep_build_guests.yml index f818e71eb..3f9f10417 100644 --- a/.github/workflows/dep_build_guests.yml +++ b/.github/workflows/dep_build_guests.yml @@ -61,15 +61,15 @@ jobs: # Resolving a revision needs the history it names. fetch-depth: ${{ inputs.ref == '' && 1 || 0 }} - # `HEAD^1` is where a pull request branched: the checkout is the merge - # its validation builds, and its first parent is the branch it targets. - # On anything but a merge `HEAD^1` is the previous commit, which would - # build guests for the wrong one without saying so. + # `HEAD^1` is the commit a pull request merges into: the checkout is the + # merge its validation builds, and its first parent is the branch it + # targets. On anything but a merge `HEAD^1` is the previous commit, which + # would build guests for the wrong one without saying so. - name: Check out the commit to build from if: inputs.ref != '' run: | git rev-parse --verify --quiet HEAD^2 >/dev/null || { - echo "::error::HEAD is not a merge, so ${{ inputs.ref }} is not the branch point" + echo "::error::HEAD is not a merge, so ${{ inputs.ref }} is not what it merges into" exit 1 } git checkout --detach ${{ inputs.ref }} diff --git a/src/hyperlight_ci/src/baseline.rs b/src/hyperlight_ci/src/baseline.rs index 99cf410b2..ca7b38fea 100644 --- a/src/hyperlight_ci/src/baseline.rs +++ b/src/hyperlight_ci/src/baseline.rs @@ -41,9 +41,11 @@ impl Worktree { pub(crate) fn add(commit: &str) -> Result { let path = PathBuf::from(WORKTREE_DIR); - // A worktree left by an interrupted run holds the path and would - // otherwise be reported as already in use. + // A worktree left by an interrupted run holds the path, and one whose + // directory went with `cargo clean` holds it without being there to + // find. Pruning clears both rather than leaving them to accumulate. let _ = remove(&path); + let _ = git(["worktree", "prune"]); git(["worktree", "add", "--detach", WORKTREE_DIR, commit]) .with_context(|| format!("Failed to check out {commit} to compare against"))?; diff --git a/src/hyperlight_ci/src/bench.rs b/src/hyperlight_ci/src/bench.rs index 0e2f38829..cd54a8a09 100644 --- a/src/hyperlight_ci/src/bench.rs +++ b/src/hyperlight_ci/src/bench.rs @@ -54,7 +54,9 @@ fn merge_output_modes(flags: &[OutputModeFlags]) -> OutputMode { #[derive(clap::Args)] pub struct BenchArgs { /// Pre-built benchmark binary to use (skip build step; can be specified multiple times) - #[arg(long)] + /// + /// One binary holds one commit, so a comparison builds its own. + #[arg(long, conflicts_with = "baseline_ref")] pub binary: Vec, /// Number of benchmarks to run in parallel (0 = all P-cores, default: 0) @@ -99,7 +101,6 @@ pub struct BenchArgs { /// `rust` and one `c` directory. #[arg(long, value_name = "DIR")] pub baseline_guests: Option, - /// Name criterion keeps the `--baseline-ref` results under #[arg(long, value_name = "NAME", default_value = "base")] pub baseline_name: String, @@ -116,6 +117,15 @@ pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { }; let commit = resolve(&spec, args.repo.as_deref())?; + + // Both passes read the configuration named here, not whatever the commit + // they measure happens to carry at the same relative path. + if let Some(config_file) = args.config_file.take() { + args.config_file = Some( + std::path::absolute(&config_file) + .with_context(|| format!("Failed to resolve {}", config_file.display()))?, + ); + } // Each pass runs from its own checkout, so the directory they write to is // named outright rather than found relative to wherever that is. let home = std::path::absolute(manifest::criterion_dir()) @@ -124,6 +134,12 @@ pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { .with_context(|| format!("Failed to create {}", home.display()))?; std::env::set_var("CRITERION_HOME", &home); + // A baseline left by an earlier run would otherwise stand in for any + // benchmark this one cannot measure, which is every benchmark the commit + // being measured against never had. + clear_baseline(&home, &args.baseline_name) + .with_context(|| format!("Failed to clear the {} baseline", args.baseline_name))?; + let guests = args .baseline_guests .clone() @@ -143,7 +159,29 @@ pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { drop(worktree); - measure(&args, with(&args, "--baseline")).await + measure(&args, with(&args, "--baseline-lenient")).await +} + +/// Drop every saved copy of the baseline `name`, leaving the measurements +/// beside them alone. +fn clear_baseline(home: &std::path::Path, name: &str) -> anyhow::Result<()> { + if !home.is_dir() { + return Ok(()); + } + + for entry in std::fs::read_dir(home)? { + let path = entry?.path(); + if !path.is_dir() { + continue; + } + if path.file_name().is_some_and(|dir| dir == name) { + std::fs::remove_dir_all(&path) + .with_context(|| format!("Failed to remove {}", path.display()))?; + } else { + clear_baseline(&path, name)?; + } + } + Ok(()) } /// The commit a baseline names. @@ -165,6 +203,9 @@ fn resolve(spec: &str, repo: Option<&str>) -> anyhow::Result { } /// The criterion arguments for one pass, naming the baseline it reads or writes. +/// +/// The comparison is lenient because a commit may add a benchmark the one it +/// is measured against never had, which strict reading treats as a failure. fn with(args: &BenchArgs, flag: &str) -> Vec { let mut bench_args = args.bench_args.clone(); bench_args.push(flag.to_string()); @@ -263,7 +304,34 @@ async fn measure(args: &BenchArgs, bench_args: Vec) -> anyhow::Result<() #[cfg(test)] mod tests { - use super::resolve; + use super::{clear_baseline, resolve}; + + /// Only the baseline being rebuilt goes, so a run holds on to the + /// measurements it is not about to replace. + #[test] + fn clearing_a_baseline_leaves_the_measurements_beside_it() { + let home = std::env::temp_dir().join(format!("hl-clear-{}", std::process::id())); + let bench = home.join("group").join("case"); + for set in ["base", "new", "change", "keepsake"] { + std::fs::create_dir_all(bench.join(set)).unwrap(); + std::fs::write(bench.join(set).join("estimates.json"), b"{}").unwrap(); + } + + clear_baseline(&home, "base").unwrap(); + + assert!(!bench.join("base").exists(), "the baseline is rebuilt"); + assert!(bench.join("new").exists(), "the last run stays"); + assert!(bench.join("change").exists(), "its comparison stays"); + assert!(bench.join("keepsake").exists(), "other baselines stay"); + + std::fs::remove_dir_all(home).unwrap(); + } + + #[test] + fn clearing_a_baseline_that_was_never_measured_is_no_work() { + let home = std::env::temp_dir().join(format!("hl-absent-{}", std::process::id())); + assert!(clear_baseline(&home, "base").is_ok()); + } /// git already reads branches, tags, shas and the revisions built from /// them, so a baseline names one directly. From 4e1097019e3e4008ddf24b8f64300192ede5b275 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Wed, 7 Oct 2026 15:43:50 +0100 Subject: [PATCH 4/6] Measure where the pull request branched `HEAD^1` is the commit a pull request merges into, which carries whatever landed on the branch it targets since the pull request left it. Those commits are not the pull request's to answer for. `HEAD^1...HEAD^2` names where the two parents of the validation merge branched apart, which is where the pull request did. `git checkout` and `git diff` already read a revision that way. `git worktree add` takes a reference rather than a revision, so the naming is done before it is handed a commit. The form also says when it does not hold: without a second parent there is nothing to branch apart from, where `HEAD^1` would quietly name the commit before. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jorge Prendes --- .github/workflows/ValidatePullRequest.yml | 21 +++++------ .github/workflows/dep_benchmarks.yml | 8 ++-- .github/workflows/dep_build_guests.yml | 10 ++--- src/hyperlight_ci/src/baseline.rs | 46 ++++++++++++++++++++++- src/hyperlight_ci/src/bench.rs | 8 ++-- 5 files changed, 67 insertions(+), 26 deletions(-) diff --git a/.github/workflows/ValidatePullRequest.yml b/.github/workflows/ValidatePullRequest.yml index 4173cf416..2034a6bdc 100644 --- a/.github/workflows/ValidatePullRequest.yml +++ b/.github/workflows/ValidatePullRequest.yml @@ -66,9 +66,9 @@ jobs: arch: ${{ matrix.arch }} config: ${{ matrix.config }} - # Guests for the commit the pull request merges into, so the benchmarks can - # measure both sides on one machine. Only the configuration the benchmark - # suite loads is needed. + # Guests for the commit the pull request branched from, so the benchmarks + # can measure both sides on one machine. Only the configuration the + # benchmark suite loads is needed. build-baseline-guests: needs: [docs-pr, update-guest-locks] # Required because update-guest-locks is skipped on non-dependabot PRs, @@ -81,9 +81,9 @@ jobs: docs_only: ${{ needs.docs-pr.outputs.docs-only }} arch: X64 config: release - # The checkout is the merge this run validates, so its first parent is - # the commit the pull request merges into. - ref: HEAD^1 + # The checkout is the merge this run validates, so where its two + # parents branched apart is where the pull request did. + ref: HEAD^1...HEAD^2 artifact_suffix: -baseline # Code checks (fmt, clippy, MSRV) - runs in parallel with build-guests @@ -270,10 +270,10 @@ jobs: hypervisor: ${{ matrix.hypervisor }} cpu_vendor: ${{ matrix.cpu_vendor }} arch: ${{ matrix.arch }} - baseline_ref: HEAD^1 + baseline_ref: HEAD^1...HEAD^2 # Report every configuration the run measured against the commit the pull - # request merges into, for the hyperlight-gh-bot to post as a PR comment. + # request branched from, for the hyperlight-gh-bot to post as a PR comment. # Only runs for PRs (not merge groups) with code changes. benchmark-comment: name: post-benchmark-comment @@ -299,9 +299,8 @@ jobs: run: | echo '## Benchmark Results' > pr-comment.md echo '' >> pr-comment.md - # No baseline is named: each run measured the commit it merges into - # alongside the pull request, so the comparison it carries is the one - # to read. + # No baseline is named: each run measured the branch point alongside + # the pull request, so the comparison it carries is the one to read. cargo ci bench-report --config-file bench_report.toml --reproduce \ --candidate run:${{ github.run_id }} >> pr-comment.md env: diff --git a/.github/workflows/dep_benchmarks.yml b/.github/workflows/dep_benchmarks.yml index 4a2c446c4..33f8f0513 100644 --- a/.github/workflows/dep_benchmarks.yml +++ b/.github/workflows/dep_benchmarks.yml @@ -116,9 +116,9 @@ jobs: path: ${{ runner.temp }}/baseline-guests/c/ # Both commits are measured on this machine, so the comparison reflects - # them rather than the difference between two runners. `HEAD^1` is the - # commit the pull request merges into, because the checkout is the merge - # this run validates, so anything else would measure the wrong commit. + # them rather than the difference between two runners. The branch point + # is read off the merge this run validates, so on anything else there is + # no second parent and the comparison would measure the wrong commit. - name: Build run: just build release @@ -126,7 +126,7 @@ jobs: run: | if [ -n "${{ inputs.baseline_ref }}" ]; then git rev-parse --verify --quiet HEAD^2 >/dev/null || { - echo "::error::HEAD is not a merge, so ${{ inputs.baseline_ref }} is not what it merges into" + echo "::error::HEAD is not a merge, so ${{ inputs.baseline_ref }} is not the branch point" exit 1 } just bench-against ${{ inputs.baseline_ref }} "$RUNNER_TEMP/baseline-guests" diff --git a/.github/workflows/dep_build_guests.yml b/.github/workflows/dep_build_guests.yml index 3f9f10417..063969b00 100644 --- a/.github/workflows/dep_build_guests.yml +++ b/.github/workflows/dep_build_guests.yml @@ -61,15 +61,15 @@ jobs: # Resolving a revision needs the history it names. fetch-depth: ${{ inputs.ref == '' && 1 || 0 }} - # `HEAD^1` is the commit a pull request merges into: the checkout is the - # merge its validation builds, and its first parent is the branch it - # targets. On anything but a merge `HEAD^1` is the previous commit, which - # would build guests for the wrong one without saying so. + # The checkout is the merge a pull request's validation builds, so where + # its two parents branched apart is where the pull request did. On + # anything but a merge there is no second parent to read, which would + # build guests for the wrong commit without saying so. - name: Check out the commit to build from if: inputs.ref != '' run: | git rev-parse --verify --quiet HEAD^2 >/dev/null || { - echo "::error::HEAD is not a merge, so ${{ inputs.ref }} is not what it merges into" + echo "::error::HEAD is not a merge, so ${{ inputs.ref }} is not the branch point" exit 1 } git checkout --detach ${{ inputs.ref }} diff --git a/src/hyperlight_ci/src/baseline.rs b/src/hyperlight_ci/src/baseline.rs index ca7b38fea..27f414688 100644 --- a/src/hyperlight_ci/src/baseline.rs +++ b/src/hyperlight_ci/src/baseline.rs @@ -47,7 +47,8 @@ impl Worktree { let _ = remove(&path); let _ = git(["worktree", "prune"]); - git(["worktree", "add", "--detach", WORKTREE_DIR, commit]) + let resolved = commit_of(commit)?; + git(["worktree", "add", "--detach", WORKTREE_DIR, &resolved]) .with_context(|| format!("Failed to check out {commit} to compare against"))?; Ok(Self { path }) @@ -100,6 +101,14 @@ fn remove(path: &Path) -> Result<()> { } fn git(args: I) -> Result<()> +where + I: IntoIterator, + S: AsRef, +{ + read_git(args).map(|_| ()) +} + +fn read_git(args: I) -> Result where I: IntoIterator, S: AsRef, @@ -112,7 +121,22 @@ where if !output.status.success() { bail!("{}", String::from_utf8_lossy(&output.stderr).trim()); } - Ok(()) + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) +} + +/// The commit a revision names. +/// +/// `git checkout` and `git diff` read `A...B` as the commit where the two +/// branched apart. `git worktree add` takes a reference rather than a +/// revision, so the naming is done here and it is handed a commit. +pub(crate) fn commit_of(revision: &str) -> Result { + if let Some((left, right)) = revision.split_once("...") { + return read_git(["merge-base", left, right]) + .with_context(|| format!("Failed to find where {left} and {right} branched apart")); + } + + read_git(["rev-parse", "--verify", &format!("{revision}^{{commit}}")]) + .with_context(|| format!("Failed to resolve {revision}")) } /// Replace `destination` with the contents of `source`. @@ -179,6 +203,24 @@ mod tests { std::fs::remove_dir_all(root).unwrap(); } + /// `A...B` names where two commits branched apart, which is what a pull + /// request's validation merge is read with. + #[test] + fn a_revision_names_one_commit() { + let head = commit_of("HEAD").unwrap(); + assert_eq!(head.len(), 40, "{head}"); + assert_eq!(commit_of("HEAD^{commit}").unwrap(), head); + + // A merge of a commit with itself branched apart at that commit. + assert_eq!(commit_of("HEAD...HEAD").unwrap(), head); + } + + #[test] + fn a_revision_that_names_nothing_is_reported() { + let error = commit_of("not-a-revision").unwrap_err().to_string(); + assert!(error.contains("not-a-revision"), "{error}"); + } + #[test] fn missing_guests_are_reported_against_the_directory_given() { let root = std::env::temp_dir().join(format!("hl-empty-{}", std::process::id())); diff --git a/src/hyperlight_ci/src/bench.rs b/src/hyperlight_ci/src/bench.rs index cd54a8a09..120845620 100644 --- a/src/hyperlight_ci/src/bench.rs +++ b/src/hyperlight_ci/src/bench.rs @@ -85,10 +85,10 @@ pub struct BenchArgs { /// Measure this commit first and compare the run against it. /// - /// Takes anything git resolves to a commit, a branch, tag or sha, or - /// `base-of:` for where a pull request branched. Both passes run on - /// this machine, so the comparison reflects the commits rather than the - /// difference between two runners. + /// Takes anything git resolves to a commit, including `A...B` for where + /// two branched apart, or `base-of:` for where a pull request did. + /// Both passes run on this machine, so the comparison reflects the commits + /// rather than the difference between two runners. #[arg(long, value_name = "COMMIT", requires = "baseline_guests")] pub baseline_ref: Option, From 2853acdb8a96aa87e4b1cab401bb114b9d8c7ea3 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Wed, 7 Oct 2026 15:52:01 +0100 Subject: [PATCH 5/6] Name the commit a run measured against A report named its baseline from the second set of results it read. A run that measures its own baseline reads no second set, so the report said only which commit it measured and left the reader to guess the other. The run records the commit it measured against in its manifest, beside what it measured and the machine it ran on, and the report names it from there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jorge Prendes --- src/hyperlight_ci/src/baseline.rs | 12 ++++++- src/hyperlight_ci/src/bench.rs | 20 +++++++++--- src/hyperlight_ci/src/bench_report.rs | 47 ++++++++++++++++++++++++++- src/hyperlight_ci/src/manifest.rs | 13 ++++++-- 4 files changed, 83 insertions(+), 9 deletions(-) diff --git a/src/hyperlight_ci/src/baseline.rs b/src/hyperlight_ci/src/baseline.rs index 27f414688..a36cd33de 100644 --- a/src/hyperlight_ci/src/baseline.rs +++ b/src/hyperlight_ci/src/baseline.rs @@ -34,6 +34,7 @@ const GUEST_DIRS: [(&str, &str); 2] = [ /// A checkout of another commit, removed when it goes out of scope. pub(crate) struct Worktree { path: PathBuf, + commit: String, } impl Worktree { @@ -51,13 +52,21 @@ impl Worktree { git(["worktree", "add", "--detach", WORKTREE_DIR, &resolved]) .with_context(|| format!("Failed to check out {commit} to compare against"))?; - Ok(Self { path }) + Ok(Self { + path, + commit: resolved, + }) } pub(crate) fn path(&self) -> &Path { &self.path } + /// The commit it holds, as the revision that named it resolved to. + pub(crate) fn commit(&self) -> &str { + &self.commit + } + /// Copy the guests of the commit this worktree holds into it. /// /// `source` holds one directory per guest language, named as @@ -228,6 +237,7 @@ mod tests { let worktree = Worktree { path: root.join("worktree"), + commit: String::new(), }; let error = worktree.place_guests(&root).unwrap_err().to_string(); diff --git a/src/hyperlight_ci/src/bench.rs b/src/hyperlight_ci/src/bench.rs index 120845620..2d2369434 100644 --- a/src/hyperlight_ci/src/bench.rs +++ b/src/hyperlight_ci/src/bench.rs @@ -113,7 +113,7 @@ pub struct BenchArgs { pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { let Some(spec) = args.baseline_ref.take() else { let bench_args = std::mem::take(&mut args.bench_args); - return measure(&args, bench_args).await; + return measure(&args, bench_args, None).await; }; let commit = resolve(&spec, args.repo.as_deref())?; @@ -152,14 +152,20 @@ pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { let here = std::env::current_dir().context("Failed to read the working directory")?; std::env::set_current_dir(worktree.path()) .with_context(|| format!("Failed to enter {}", worktree.path().display()))?; - let measured = measure(&args, with(&args, "--save-baseline")).await; + let measured = measure(&args, with(&args, "--save-baseline"), None).await; std::env::set_current_dir(&here) .with_context(|| format!("Failed to return to {}", here.display()))?; measured.with_context(|| format!("Failed to measure {commit}"))?; + let measured_against = worktree.commit().to_string(); drop(worktree); - measure(&args, with(&args, "--baseline-lenient")).await + measure( + &args, + with(&args, "--baseline-lenient"), + Some(measured_against), + ) + .await } /// Drop every saved copy of the baseline `name`, leaving the measurements @@ -213,7 +219,11 @@ fn with(args: &BenchArgs, flag: &str) -> Vec { bench_args } -async fn measure(args: &BenchArgs, bench_args: Vec) -> anyhow::Result<()> { +async fn measure( + args: &BenchArgs, + bench_args: Vec, + baseline: Option, +) -> anyhow::Result<()> { let config = args .config_file .as_deref() @@ -299,7 +309,7 @@ async fn measure(args: &BenchArgs, bench_args: Vec) -> anyhow::Result<() drop(ballast); result?; - manifest::write(benchmarks).context("Failed to write the benchmark manifest") + manifest::write(benchmarks, baseline).context("Failed to write the benchmark manifest") } #[cfg(test)] diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs index bc50868c8..0419b8eff 100644 --- a/src/hyperlight_ci/src/bench_report.rs +++ b/src/hyperlight_ci/src/bench_report.rs @@ -334,8 +334,15 @@ fn measured(repo: &str, candidate: &Origin, baseline: &Origin) -> Option format!("[`{short}`](https://github.com/{repo}/commit/{sha})") }; + // A run that measured its own baseline recorded which commit that was, so + // the report names it without a second set of results to read it from. + let carried = candidate + .inputs + .iter() + .find_map(|input| manifest::read(&input.dir).ok().flatten()?.baseline); + let mut lines = format!("Measured commit: {}", candidate.commit.as_ref().map(link)?); - if let Some(baseline) = baseline.commit.as_ref().map(link) { + if let Some(baseline) = baseline.commit.as_ref().or(carried.as_ref()).map(link) { lines.push_str(&format!("\nBaseline commit: {baseline}")); } lines.push_str("\n\n"); @@ -550,6 +557,44 @@ async fn discover_benchmarks(args: &BenchReportArgs, dir: &Path) -> Result, + /// The commit measured alongside this run, for a comparison that needs no + /// results from anywhere else. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub baseline: Option, } #[derive(Serialize, Deserialize)] @@ -104,8 +108,12 @@ fn hypervisor() -> Option { None } -/// Record `benchmarks` as the contents of the run about to start. -pub(crate) fn write(benchmarks: impl IntoIterator) -> Result<()> { +/// Record `benchmarks` as the contents of the run about to start, and +/// `baseline` as the commit it was measured against. +pub(crate) fn write( + benchmarks: impl IntoIterator, + baseline: Option, +) -> Result<()> { let (cpu_vendor, cpu_model) = cpu_vendor_and_model(); let mut benchmarks: Vec = benchmarks.into_iter().collect(); benchmarks.sort(); @@ -124,6 +132,7 @@ pub(crate) fn write(benchmarks: impl IntoIterator) -> Result<()> hypervisor: hypervisor(), }, benchmarks, + baseline, }; let dir = criterion_dir(); From 832a44fe7779397860b440d95dfb81107064d3ec Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Wed, 7 Oct 2026 17:13:16 +0100 Subject: [PATCH 6/6] Clear only the baselines that hold measurements A benchmark may be grouped under the name a baseline is saved under, and clearing the baseline took the group with it: its benchmarks, their last run and every other baseline saved beside them. Measurements tell the two apart. A saved baseline holds them, a group holds the benchmarks below it, so the group is walked into rather than removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jorge Prendes --- src/hyperlight_ci/src/bench.rs | 35 ++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/src/hyperlight_ci/src/bench.rs b/src/hyperlight_ci/src/bench.rs index 2d2369434..065b2875c 100644 --- a/src/hyperlight_ci/src/bench.rs +++ b/src/hyperlight_ci/src/bench.rs @@ -180,10 +180,15 @@ fn clear_baseline(home: &std::path::Path, name: &str) -> anyhow::Result<()> { if !path.is_dir() { continue; } - if path.file_name().is_some_and(|dir| dir == name) { + + // A benchmark may be grouped under the same name a baseline is saved + // under. Measurements tell the two apart: a saved baseline holds them, + // a group holds the benchmarks below it. + let measured = path.join("estimates.json").is_file(); + if measured && path.file_name().is_some_and(|dir| dir == name) { std::fs::remove_dir_all(&path) .with_context(|| format!("Failed to remove {}", path.display()))?; - } else { + } else if !measured { clear_baseline(&path, name)?; } } @@ -337,6 +342,32 @@ mod tests { std::fs::remove_dir_all(home).unwrap(); } + /// A benchmark may be grouped under the name a baseline is saved under, + /// and the group holds every measurement below it. + #[test] + fn clearing_a_baseline_spares_a_benchmark_of_the_same_name() { + let home = std::env::temp_dir().join(format!("hl-group-{}", std::process::id())); + let grouped = home.join("base").join("case"); + for set in ["base", "new"] { + std::fs::create_dir_all(grouped.join(set)).unwrap(); + std::fs::write(grouped.join(set).join("estimates.json"), b"{}").unwrap(); + } + + clear_baseline(&home, "base").unwrap(); + + assert!( + home.join("base").is_dir(), + "a group named after the baseline is not a saved result" + ); + assert!(grouped.join("new").exists(), "its measurements stay"); + assert!( + !grouped.join("base").exists(), + "the baseline saved within it still goes" + ); + + std::fs::remove_dir_all(home).unwrap(); + } + #[test] fn clearing_a_baseline_that_was_never_measured_is_no_work() { let home = std::env::temp_dir().join(format!("hl-absent-{}", std::process::id()));