diff --git a/.github/workflows/ValidatePullRequest.yml b/.github/workflows/ValidatePullRequest.yml index e393a69ed..2034a6bdc 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 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 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,10 +270,11 @@ jobs: hypervisor: ${{ matrix.hypervisor }} cpu_vendor: ${{ matrix.cpu_vendor }} arch: ${{ matrix.arch }} + baseline_ref: HEAD^1...HEAD^2 - # 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 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 needs: @@ -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..33f8f0513 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. 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 - 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..063969b00 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 }} + + # 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 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/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/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/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"] } diff --git a/src/hyperlight_ci/src/baseline.rs b/src/hyperlight_ci/src/baseline.rs new file mode 100644 index 000000000..a36cd33de --- /dev/null +++ b/src/hyperlight_ci/src/baseline.rs @@ -0,0 +1,250 @@ +// 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, + commit: String, +} + +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 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"]); + + 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, + 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 + /// [`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, +{ + read_git(args).map(|_| ()) +} + +fn read_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(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`. +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(); + } + + /// `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())); + std::fs::create_dir_all(&root).unwrap(); + + let worktree = Worktree { + path: root.join("worktree"), + commit: String::new(), + }; + 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..065b2875c 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)] @@ -52,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) @@ -79,12 +83,152 @@ 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, 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, + + /// 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, None).await; + }; + + 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()) + .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); + + // 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() + .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"), 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"), + Some(measured_against), + ) + .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; + } + + // 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 if !measured { + clear_baseline(&path, name)?; + } + } + Ok(()) +} + +/// 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. +/// +/// 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()); + bench_args.push(args.baseline_name.clone()); + bench_args +} + +async fn measure( + args: &BenchArgs, + bench_args: Vec, + baseline: Option, +) -> anyhow::Result<()> { let config = args .config_file .as_deref() @@ -94,37 +238,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) @@ -167,5 +314,80 @@ pub async fn run(mut args: BenchArgs) -> 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)] +mod tests { + 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(); + } + + /// 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())); + 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. + #[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..0419b8eff 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, @@ -336,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"); @@ -552,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)] @@ -44,7 +48,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")) @@ -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();