From 627d95e352414bc8cfca2a4144487c1a17b25599 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Fri, 12 Jun 2026 09:42:53 +0100 Subject: [PATCH 01/12] Add hyperlight-ci crate for benchmark orchestration and reporting Introduce a new internal tooling crate (hyperlight-ci) that provides: - bench subcommand: Runs criterion benchmarks in parallel via criterion-swarm. Features include: - Configurable parallelism (-j N, defaults to all P-cores) - Configurable output modes (spinner, stream, summary) - Support for pre-built binaries (--binary) to skip rebuilds - Trailing args forwarded to criterion (filter, --exact, etc.) - bench-report subcommand: Generates markdown comparison tables from criterion's target/criterion/ JSON output via criterion-markdown. Features include: - Benchmark discovery via criterion-swarm - Optional allowlist filtering via --binary or trailing args - Output to stdout This replaces ad-hoc benchmark scripting with a unified tool suitable for both local development and CI report generation. Signed-off-by: Jorge Prendes --- Cargo.lock | 226 ++++++++++++++++++++++++-- Cargo.toml | 1 + src/hyperlight_ci/Cargo.toml | 19 +++ src/hyperlight_ci/src/bench.rs | 125 ++++++++++++++ src/hyperlight_ci/src/bench_report.rs | 74 +++++++++ src/hyperlight_ci/src/main.rs | 33 ++++ 6 files changed, 465 insertions(+), 13 deletions(-) create mode 100644 src/hyperlight_ci/Cargo.toml create mode 100644 src/hyperlight_ci/src/bench.rs create mode 100644 src/hyperlight_ci/src/bench_report.rs create mode 100644 src/hyperlight_ci/src/main.rs diff --git a/Cargo.lock b/Cargo.lock index de32f7fe70..1c64b0997e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -57,6 +57,16 @@ version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4b46cbb362ab8752921c97e041f5e366ee6297bd428a31275b9fcf1e380f7299" +[[package]] +name = "ansi-replace" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f8b155ab93213f41c886d3a46e335258428e52c7cf868e25cf099d50274496d" +dependencies = [ + "regex", + "stable-pattern", +] + [[package]] name = "anstream" version = "1.0.0" @@ -93,7 +103,7 @@ version = "1.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40c48f72fd53cd289104fc64099abca73db4166ad86ea0b4341abe65af83dadc" dependencies = [ - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -104,7 +114,7 @@ checksum = "291e6a250ff86cd4a820112fb8898808a366d8f9f58ce16d1f538353ad55747d" dependencies = [ "anstyle", "once_cell_polyfill", - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -498,6 +508,19 @@ version = "1.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1d07550c9036bf2ae0c684c4297d503f838287c83c53686d05370d0e139ae570" +[[package]] +name = "console" +version = "0.15.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "054ccb5b10f9f2cbf51eb355ca1d05c2d279ce1804688d0db74b4733a5aeafd8" +dependencies = [ + "encode_unicode", + "libc", + "once_cell", + "unicode-width", + "windows-sys 0.59.0", +] + [[package]] name = "const-oid" version = "0.10.2" @@ -592,6 +615,19 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "cpu-pin" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eb5bc1be026f7f066429ce0611e23a341db91b91ed701de4a1432d01d3ed1105" +dependencies = [ + "libc", + "mach2", + "once_cell", + "tokio", + "windows", +] + [[package]] name = "cpufeatures" version = "0.3.1" @@ -635,6 +671,17 @@ dependencies = [ "walkdir", ] +[[package]] +name = "criterion-markdown" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "72741fa695d01bfab5c47a083ef370db8235c90aee6033136ce49aa7dcd2a413" +dependencies = [ + "anyhow", + "serde", + "serde_json", +] + [[package]] name = "criterion-plot" version = "0.8.2" @@ -645,6 +692,23 @@ dependencies = [ "itertools 0.13.0", ] +[[package]] +name = "criterion-swarm" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "82693ab3e80479ca52770515f3837028800e365aac48a83edb86f920a6783232" +dependencies = [ + "ansi-replace", + "anyhow", + "clap", + "cpu-pin", + "indicatif", + "regex", + "serde_json", + "simple-pool", + "tokio", +] + [[package]] name = "crossbeam-channel" version = "0.5.17" @@ -840,7 +904,7 @@ dependencies = [ "libc", "option-ext", "redox_users", - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -886,6 +950,12 @@ dependencies = [ "zerocopy", ] +[[package]] +name = "encode_unicode" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "34aa73646ffb006b8f5147f3dc182bd4bcb190227ce861fc4a4844bf8e3cb2c0" + [[package]] name = "endian-type" version = "0.1.2" @@ -928,7 +998,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -1275,7 +1345,7 @@ dependencies = [ "gobject-sys", "libc", "system-deps", - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -1546,6 +1616,19 @@ dependencies = [ "tracing", ] +[[package]] +name = "hyperlight-ci" +version = "0.0.0" +dependencies = [ + "anyhow", + "clap", + "criterion-markdown", + "criterion-swarm", + "serde", + "serde_json", + "tokio", +] + [[package]] name = "hyperlight-common" version = "0.17.0" @@ -1731,7 +1814,7 @@ dependencies = [ "vmm-sys-util", "windows", "windows-result", - "windows-sys", + "windows-sys 0.61.2", "windows-version", ] @@ -1933,6 +2016,19 @@ dependencies = [ "serde_core", ] +[[package]] +name = "indicatif" +version = "0.17.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "183b3088984b400f4cfac3620d5e076c84da5364016b4f49473de574b2586235" +dependencies = [ + "console", + "number_prefix", + "portable-atomic", + "unicode-width", + "web-time", +] + [[package]] name = "ipnet" version = "2.12.2" @@ -2391,7 +2487,7 @@ checksum = "4b18443e9c262bfe8fa82f51666e2642c53393f7e5c27b3e1aeab922cff5b9d8" dependencies = [ "libc", "wasi", - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -2455,7 +2551,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -2488,6 +2584,12 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "number_prefix" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "830b246a0e5f20af87141b25c173cd1b609bd7779a4617d6ec582abaf90870f3" + [[package]] name = "object" version = "0.40.0" @@ -2499,6 +2601,12 @@ dependencies = [ "ruzstd", ] +[[package]] +name = "object-id" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c587bd1cd63959a8520442afc0f92a875d83deea175c7b48dd9f104a2c5070a9" + [[package]] name = "oci-spec" version = "0.10.0" @@ -3347,7 +3455,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -3558,6 +3666,16 @@ version = "0.3.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3a219298ac11a56ea9a6d2120044824d6f01aeb034955e7af7bc16858527deea" +[[package]] +name = "simple-pool" +version = "0.0.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "073382259dbeb56c3eaab04a1d330459f6490d1e518b2a8ee441c8bd00dbc092" +dependencies = [ + "object-id", + "parking_lot", +] + [[package]] name = "sketches-ddsketch" version = "0.3.1" @@ -3583,7 +3701,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c3d1e2c7f27f8d4cb10542a02c49005dbd6e93095799d6f3be745fae9f8fedd4" dependencies = [ "libc", - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -3601,6 +3719,15 @@ dependencies = [ "lock_api", ] +[[package]] +name = "stable-pattern" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4564168c00635f88eaed410d5efa8131afa8d8699a612c80c455a0ba05c21045" +dependencies = [ + "memchr", +] + [[package]] name = "stable_deref_trait" version = "1.2.1" @@ -3702,7 +3829,7 @@ dependencies = [ "getrandom 0.4.3", "once_cell", "rustix", - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -3777,7 +3904,7 @@ dependencies = [ "signal-hook-registry", "socket2", "tokio-macros", - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -4459,7 +4586,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -4569,6 +4696,15 @@ dependencies = [ "windows-link", ] +[[package]] +name = "windows-sys" +version = "0.59.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e38bc4d79ed67fd075bcc251a1c39b32a1776bbe92e5bef1f0bf1f8c531853b" +dependencies = [ + "windows-targets", +] + [[package]] name = "windows-sys" version = "0.61.2" @@ -4578,6 +4714,22 @@ dependencies = [ "windows-link", ] +[[package]] +name = "windows-targets" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b724f72796e036ab90c1021d4780d4d3d648aca59e491e6b98e725b84e99973" +dependencies = [ + "windows_aarch64_gnullvm", + "windows_aarch64_msvc", + "windows_i686_gnu", + "windows_i686_gnullvm", + "windows_i686_msvc", + "windows_x86_64_gnu", + "windows_x86_64_gnullvm", + "windows_x86_64_msvc", +] + [[package]] name = "windows-threading" version = "0.2.1" @@ -4596,6 +4748,54 @@ dependencies = [ "windows-link", ] +[[package]] +name = "windows_aarch64_gnullvm" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32a4622180e7a0ec044bb555404c800bc9fd9ec262ec147edd5989ccd0c02cd3" + +[[package]] +name = "windows_aarch64_msvc" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09ec2a7bb152e2252b53fa7803150007879548bc709c039df7627cabbd05d469" + +[[package]] +name = "windows_i686_gnu" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e9b5ad5ab802e97eb8e295ac6720e509ee4c243f69d781394014ebfe8bbfa0b" + +[[package]] +name = "windows_i686_gnullvm" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0eee52d38c090b3caa76c563b86c3a4bd71ef1a819287c19d586d7334ae8ed66" + +[[package]] +name = "windows_i686_msvc" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "240948bc05c5e7c6dabba28bf89d89ffce3e303022809e73deaefe4f6ec56c66" + +[[package]] +name = "windows_x86_64_gnu" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "147a5c80aabfbf0c7d901cb5895d1de30ef2907eb21fbbab29ca94c5b08b1a78" + +[[package]] +name = "windows_x86_64_gnullvm" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "24d5b23dc417412679681396f2b49f3de8c1473deb516bd34410872eff51ed0d" + +[[package]] +name = "windows_x86_64_msvc" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" + [[package]] name = "winnow" version = "0.7.15" diff --git a/Cargo.toml b/Cargo.toml index 2660825a04..0b1c83201f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,6 +6,7 @@ default-members = [ "src/hyperlight_testing", ] members = [ + "src/hyperlight_ci", "src/hyperlight_common", "src/hyperlight_guest", "src/hyperlight_host", diff --git a/src/hyperlight_ci/Cargo.toml b/src/hyperlight_ci/Cargo.toml new file mode 100644 index 0000000000..9243145387 --- /dev/null +++ b/src/hyperlight_ci/Cargo.toml @@ -0,0 +1,19 @@ +[package] +name = "hyperlight-ci" +edition = "2021" +# fields intentionally not set, to avoid accidentally publishing this crate to crates.io +description = """ +Hyperlight's CI and development tools. +""" + +[lints] +workspace = true + +[dependencies] +anyhow = "1" +clap = { version = "4.6.1", features = ["derive"] } +criterion-markdown = "0.1.2" +criterion-swarm = "0.2" +serde = { version = "1.0", features = ["derive"] } +serde_json = "1.0" +tokio = { version = "1", features = ["rt", "macros"] } \ No newline at end of file diff --git a/src/hyperlight_ci/src/bench.rs b/src/hyperlight_ci/src/bench.rs new file mode 100644 index 0000000000..3ab0965665 --- /dev/null +++ b/src/hyperlight_ci/src/bench.rs @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2025 The Hyperlight Authors. +//! The `bench` subcommand: runs criterion benchmarks in parallel via criterion-swarm. + +use std::io::IsTerminal; +use std::path::PathBuf; + +use anyhow::Context; +use criterion_swarm::{CriterionSwarm, OutputMode}; + +/// An output mode flag for `--build-output` / `--benchmarks-output`. +#[derive(Clone, Debug)] +pub(crate) struct OutputModeFlags(OutputMode); + +impl OutputModeFlags { + /// Parse a single token into an `OutputMode` flag. + fn parse_one(s: &str) -> Result { + match s.trim().to_ascii_lowercase().as_str() { + "spinner" => Ok(OutputMode::SPINNER), + "stream" => Ok(OutputMode::STREAM), + "summary" => Ok(OutputMode::SUMMARY), + "none" | "silent" => Ok(OutputMode::SILENT), + other => Err(format!( + "unknown output mode `{other}` (expected: spinner, stream, summary, none, silent)" + )), + } + } +} + +impl std::str::FromStr for OutputModeFlags { + type Err = String; + fn from_str(s: &str) -> Result { + let mut mode = OutputMode::SILENT; + for part in s.split(',') { + mode |= Self::parse_one(part)?; + } + Ok(Self(mode)) + } +} + +/// Merge a `Vec` into a single `OutputMode` by OR-ing them together. +fn merge_output_modes(flags: &[OutputModeFlags]) -> OutputMode { + flags.iter().fold(OutputMode::SILENT, |acc, f| acc | f.0) +} + +/// Command-line arguments for the `bench` subcommand. +#[derive(clap::Args)] +pub struct BenchArgs { + /// Pre-built benchmark binary to use (skip build step; can be specified multiple times) + #[arg(long)] + pub binary: Vec, + + /// Number of benchmarks to run in parallel (0 = all P-cores, default: 0) + #[arg(long, short, default_value_t = 0)] + pub jobs: usize, + + /// Build output mode (comma-separated or repeated): spinner, stream, summary, none + #[arg(long, value_delimiter = ',')] + pub build_output: Vec, + + /// Benchmarks output mode (comma-separated or repeated): spinner, stream, summary, none + #[arg(long, value_delimiter = ',')] + pub benchmarks_output: Vec, + + /// Additional features to pass to cargo when building benchmarks (can be specified multiple times) + #[arg(short = 'F', long)] + pub features: Vec, + + /// 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 mut swarm = CriterionSwarm::builder().jobs(args.jobs); + + if !args.binary.is_empty() { + swarm = swarm.binaries(args.binary); + } + + if !args.features.is_empty() { + swarm = swarm.build_args(["--features".to_string(), args.features.join(",")]); + } + + for arg in args.bench_args { + swarm = swarm.bench_arg(arg); + } + + if args.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)); + } + + if args.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)); + } + + let build_mode = merge_output_modes(&args.build_output); + let bench_mode = merge_output_modes(&args.benchmarks_output); + swarm = swarm.output( + criterion_swarm::ProgressReporter::new() + .build(build_mode) + .benchmarks(bench_mode), + ); + + let swarm = swarm + .prepare() + .await + .context("Failed to prepare criterion swarm")?; + if bench_mode == (bench_mode | OutputMode::SUMMARY) { + let total = swarm.benchmarks().len(); + let jobs = swarm.jobs().min(total); + println!("Running {total} benchmarks with parallelism {jobs}"); + } + swarm.run().await.context("Failed to run criterion swarm") +} diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs new file mode 100644 index 0000000000..1410b94dae --- /dev/null +++ b/src/hyperlight_ci/src/bench_report.rs @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2025 The Hyperlight Authors. +//! The `bench-report` subcommand: generates a markdown table from existing +//! criterion benchmark results in `target/criterion/`. + +use std::path::PathBuf; + +use anyhow::{Context, Result}; +use clap::Args; +use criterion_swarm::{CriterionSwarm, NoopReporter}; + +/// Command-line arguments for the `bench-report` subcommand. +#[derive(Args)] +pub struct BenchReportArgs { + /// Benchmark binary to list benchmarks from (can be specified multiple times). + /// When provided, only benchmarks available in these binaries are included. + #[arg(long)] + pub binary: Vec, + + /// Path to the criterion output directory + #[arg(long, default_value = "target/criterion")] + pub criterion_dir: PathBuf, + + /// Wrap the output in a collapsible
tag with the given summary text. + #[arg(long)] + pub collapsible: Option, + + /// Additional arguments to forward to criterion benchmarks (e.g. filter, --exact) + #[arg(trailing_var_arg = true, allow_hyphen_values = true)] + pub bench_args: Vec, +} + +/// Entry point for the bench-report subcommand. +pub async fn run(args: BenchReportArgs) -> Result<()> { + let allowlist = build_allowlist(&args).await?; + + let options = criterion_markdown::RenderOptions { + collapsible: args.collapsible, + }; + let markdown = + criterion_markdown::render_with_options(&args.criterion_dir, &allowlist, &options)?; + + print!("{markdown}"); + + Ok(()) +} + +/// Builds an allowlist of benchmark full_ids by discovering benchmarks via CriterionSwarm. +/// +/// All trailing arguments (filter, --exact, etc.) are forwarded as bench args +/// to CriterionSwarm so it handles filtering during discovery. +async fn build_allowlist(args: &BenchReportArgs) -> Result> { + let mut swarm = CriterionSwarm::builder(); + + if !args.binary.is_empty() { + swarm = swarm.binaries(&args.binary); + } + + for arg in &args.bench_args { + swarm = swarm.bench_arg(arg); + } + + let discovered = swarm + .output(NoopReporter) + .prepare() + .await + .context("Failed to discover benchmarks")?; + + Ok(discovered + .benchmarks() + .into_iter() + .map(str::to_string) + .collect()) +} diff --git a/src/hyperlight_ci/src/main.rs b/src/hyperlight_ci/src/main.rs new file mode 100644 index 0000000000..76f9135bac --- /dev/null +++ b/src/hyperlight_ci/src/main.rs @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2025 The Hyperlight Authors. +mod bench; +mod bench_report; + +use clap::{Parser, Subcommand}; + +#[derive(Parser)] +#[command( + name = "hyperlight-ci", + about = "Hyperlight's CI and development tools" +)] +struct Cli { + #[command(subcommand)] + command: Commands, +} + +#[derive(Subcommand)] +enum Commands { + /// Run benchmarks using the benchmark binary directly + Bench(bench::BenchArgs), + /// Generate a markdown table from existing criterion benchmark results + BenchReport(bench_report::BenchReportArgs), +} + +#[tokio::main(flavor = "current_thread")] +async fn main() -> anyhow::Result<()> { + let cli = Cli::parse(); + match cli.command { + Commands::Bench(args) => bench::run(args).await, + Commands::BenchReport(args) => bench_report::run(args).await, + } +} From 88c4c4c8265141a6764902b0bb8623f872e9947a Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Fri, 12 Jun 2026 09:46:50 +0100 Subject: [PATCH 02/12] Integrate hyperlight-ci into CI workflows and Just recipes - Add cargo alias (`cargo ci`) for convenient hyperlight-ci invocation - Update dep_benchmarks workflow to use `cargo ci bench` and generate a markdown report via `cargo ci bench-report`, posting results as a PR comment per hypervisor/cpu matrix entry - Add benchmarks job to ValidatePullRequest workflow with hypervisor and cpu matrix, gated behind docs-only and build-guests checks - Grant pull-requests: write permission for PR comment posting - Simplify Justfile bench recipes to delegate to `cargo ci bench` - Update benchmarking docs to reflect the new workflow Signed-off-by: Jorge Prendes --- .cargo/config.toml | 3 ++ .github/hyperlight-bot.yml | 8 ++++ .github/workflows/ValidatePullRequest.yml | 58 +++++++++++++++++++++++ .github/workflows/dep_benchmarks.yml | 11 ++++- Justfile | 6 +-- 5 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 .github/hyperlight-bot.yml diff --git a/.cargo/config.toml b/.cargo/config.toml index b1c8f87b3c..0c3c4d34a0 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -1,2 +1,5 @@ [target.'cfg(target_os = "macos")'] runner = "dev/macos-sign-and-run.sh" + +[alias] # command aliases +ci = ["run", "--quiet", "--package=hyperlight-ci", "--"] diff --git a/.github/hyperlight-bot.yml b/.github/hyperlight-bot.yml new file mode 100644 index 0000000000..758fc3e550 --- /dev/null +++ b/.github/hyperlight-bot.yml @@ -0,0 +1,8 @@ +# Configuration for the hyperlight-gh-bot GitHub App. +# See: https://github.com/jprendes/hyperlight-gh-bot + +# Name of the artifact containing the comment body. +artifact_name: "pr-comment" + +# Regex matched against the job name to filter which jobs trigger the bot. +job_filter: "post-benchmark-comment" diff --git a/.github/workflows/ValidatePullRequest.yml b/.github/workflows/ValidatePullRequest.yml index 0c8d75a2dc..41c6f77a04 100644 --- a/.github/workflows/ValidatePullRequest.yml +++ b/.github/workflows/ValidatePullRequest.yml @@ -226,6 +226,63 @@ jobs: arch: ${{ matrix.arch }} target: ${{ matrix.target }} + # Run benchmarks and post results as PR comment + benchmarks: + needs: + - docs-pr + - build-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 + if: ${{ !cancelled() && !failure() }} + strategy: + fail-fast: false + matrix: + arch: [X64] + hypervisor: ['hyperv-ws2025', mshv3, kvm] + cpu_vendor: [amd, intel] + uses: ./.github/workflows/dep_benchmarks.yml + secrets: inherit + with: + docs_only: ${{ needs.docs-pr.outputs.docs-only }} + hypervisor: ${{ matrix.hypervisor }} + cpu_vendor: ${{ matrix.cpu_vendor }} + arch: ${{ matrix.arch }} + + # Combine benchmark reports into a single artifact 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: + - docs-pr + - benchmarks + if: ${{ !cancelled() && !failure() && needs.docs-pr.outputs.docs-only == 'false' && github.event_name == 'pull_request' }} + runs-on: ubuntu-latest + steps: + - name: Download benchmark reports + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + pattern: benchmark-report_* + path: reports/ + + - name: Combine benchmark reports + run: | + echo '## Benchmark Results' > pr-comment.md + echo '' >> pr-comment.md + for f in reports/benchmark-report_*/benchmark.md; do + [ -f "$f" ] || continue + cat "$f" >> pr-comment.md + echo '' >> pr-comment.md + done + + - name: Upload PR comment artifact + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: pr-comment + path: pr-comment.md + if-no-files-found: warn + retention-days: 1 + spelling: name: spell check with typos runs-on: ubuntu-latest @@ -254,6 +311,7 @@ jobs: - build-test - run-examples - fuzzing + - benchmarks - spelling - license-headers if: always() diff --git a/.github/workflows/dep_benchmarks.yml b/.github/workflows/dep_benchmarks.yml index dc81ed9705..8a7cda1014 100644 --- a/.github/workflows/dep_benchmarks.yml +++ b/.github/workflows/dep_benchmarks.yml @@ -60,7 +60,6 @@ on: required: false type: number default: 5 - env: CARGO_TERM_COLOR: always RUST_BACKTRACE: full @@ -143,6 +142,16 @@ jobs: - name: Run benchmarks run: just bench-ci main + - name: Create benchmarks report + run: cargo ci bench-report --collapsible '${{ inputs.hypervisor }} / ${{ inputs.cpu_vendor }} (${{ runner.os }})' > target/criterion/benchmark.md + + - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: benchmark-report_${{ runner.os }}_${{ inputs.hypervisor }}_${{ inputs.cpu_vendor }} + path: target/criterion/benchmark.md + if-no-files-found: error + retention-days: 1 + - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: benchmarks_${{ runner.os }}_${{ inputs.hypervisor }}_${{ inputs.cpu_vendor }} diff --git a/Justfile b/Justfile index 577e24b8b9..75a9a8a764 100644 --- a/Justfile +++ b/Justfile @@ -437,12 +437,10 @@ bench-download os hypervisor cpu_vendor tag="": # Warning: compares to and then OVERWRITES the given baseline bench-ci baseline features="": - @# Benchmarks are always run with release builds for meaningful results - cargo bench --profile=release {{ if features =="" {''} else { "--features " + features } }} -- --verbose --save-baseline {{ baseline }} + cargo ci bench {{ if features == "" {''} else { "--features " + features } }} --verbose --save-baseline {{ baseline }} bench features="": - @# Benchmarks are always run with release builds for meaningful results - cargo bench --profile=release {{ if features =="" {''} else { "--features " + features } }} -- --verbose + cargo ci bench {{ if features == "" {''} else { "--features " + features } }} --verbose ############### ### FUZZING ### From c533ba330a97b72bca2d60006c0f594570d4fc7f Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Mon, 21 Sep 2026 17:54:47 +0100 Subject: [PATCH 03/12] feat(ci): filter benchmark reports through a config file `bench_report.toml` lists regular expressions matched against criterion benchmark ids. A benchmark is selected when it matches `allowlist` and no `denylist` entry, and an empty `allowlist` keeps everything the denylist does not exclude. Omitting both lists disables filtering. `cargo ci bench-report --config-file` reports the selection, and `cargo ci bench --config-file` runs it. CI keeps running every benchmark and filters at report time. Both subcommands apply the selection through `CriterionSwarm::retain`, added in criterion-swarm 0.2.1, so running and reporting cannot drift apart. An allowlist pattern that matches no benchmark fails, so a rename surfaces instead of dropping out of the comment silently. A denylist pattern matching nothing is accepted, because a benchmark may be absent on some platforms. Unknown keys are rejected so a stale key cannot silently disable filtering. The initial lists keep 47 of 116 benchmarks: those whose median drifted by at most 5% across five back-to-back runs on an idle machine, less the snapshot cold start and restore families. Signed-off-by: Jorge Prendes --- .github/workflows/dep_benchmarks.yml | 2 +- Cargo.lock | 6 +- bench_report.toml | 84 ++++++++++ src/hyperlight_ci/Cargo.toml | 6 +- src/hyperlight_ci/src/bench.rs | 24 ++- src/hyperlight_ci/src/bench_report.rs | 18 ++- src/hyperlight_ci/src/config.rs | 224 ++++++++++++++++++++++++++ src/hyperlight_ci/src/main.rs | 1 + 8 files changed, 355 insertions(+), 10 deletions(-) create mode 100644 bench_report.toml create mode 100644 src/hyperlight_ci/src/config.rs diff --git a/.github/workflows/dep_benchmarks.yml b/.github/workflows/dep_benchmarks.yml index 8a7cda1014..fcd624af6b 100644 --- a/.github/workflows/dep_benchmarks.yml +++ b/.github/workflows/dep_benchmarks.yml @@ -143,7 +143,7 @@ jobs: run: just bench-ci main - name: Create benchmarks report - run: cargo ci bench-report --collapsible '${{ inputs.hypervisor }} / ${{ inputs.cpu_vendor }} (${{ runner.os }})' > target/criterion/benchmark.md + run: cargo ci bench-report --config-file bench_report.toml --collapsible '${{ inputs.hypervisor }} / ${{ inputs.cpu_vendor }} (${{ runner.os }})' > target/criterion/benchmark.md - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: diff --git a/Cargo.lock b/Cargo.lock index 1c64b0997e..7c45da51b2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -694,9 +694,9 @@ dependencies = [ [[package]] name = "criterion-swarm" -version = "0.2.0" +version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "82693ab3e80479ca52770515f3837028800e365aac48a83edb86f920a6783232" +checksum = "4bbb01e7dd0d588f78fe0bd2fdc12e921875542754473f22b09da4f9edbc2770" dependencies = [ "ansi-replace", "anyhow", @@ -1624,9 +1624,11 @@ dependencies = [ "clap", "criterion-markdown", "criterion-swarm", + "regex", "serde", "serde_json", "tokio", + "toml 1.1.6+spec-1.1.0", ] [[package]] diff --git a/bench_report.toml b/bench_report.toml new file mode 100644 index 0000000000..6a0627bfb8 --- /dev/null +++ b/bench_report.toml @@ -0,0 +1,84 @@ +# Benchmarks whose results appear in pull request comments. +# +# Entries are regular expressions matched against criterion benchmark ids such +# as `sandboxes/create_initialized/default`. CI runs every benchmark and reports +# only the ones selected here, so noisy benchmarks can be dropped from the +# comment without losing their results in the uploaded criterion artifact. +# +# A benchmark is reported when it matches `allowlist` and no `denylist` entry. +# An empty `allowlist` keeps every benchmark the denylist does not exclude, and +# leaving both lists empty disables filtering. +# +# An allowlist entry matching no benchmark fails the report, so a renamed or +# deleted benchmark surfaces instead of silently vanishing from the comment. A +# denylist entry matching nothing is accepted, because a benchmark may be absent +# on some platforms. +# +# cargo ci bench-report --config-file bench_report.toml +# cargo ci bench --config-file bench_report.toml +# +# The lists below keep the benchmarks whose median drifted by at most 5% across +# five back-to-back local runs on an idle 32 core machine. CI runners are +# smaller and noisier, so treat that as a lower bound on the drift CI sees. +# Regenerate by running the suite several times with `--save-baseline` and +# comparing `target/criterion///estimates.json`. +# +# `snapshot_files/cold_start_via_snapshot` and `snapshots/restore` stay out +# whatever they measure. Both families have members far outside the threshold, +# and `snapshots/restore/small` swung 1.43x on a later run after measuring 1.25% +# over the five sampled here. +# +# The `hyperlight_common` groups were restructured by the virtqueue transport +# work, which replaced the `alloc_*`, `free*`, `recycle_pool` and +# `segmented_payload` groups with `payload_allocation` and `slot_pool`, and +# shortened the `virtq_*_allocator_strategy` names. The entries below are the +# members of the new groups that hold to the threshold. +# +# The `snapshot_files`, `snapshots`, `sandboxes/sandbox_from_snapshot` and +# `guest_calls/call_with_restore` entries keep the drift measured before that +# work landed. Taking a snapshot fails on this machine now, so those families +# could not be re-measured. +# +# The commented entries widen the allowlist back to the full suite. Uncommenting +# all of them and emptying the denylist reports all 87 benchmarks. The trailing +# count is how many extra benchmarks each one pulls in. +allowlist = [ + # "^function_call_serialization/", # adds 2 + "^guest_calls/", + # "^guest_functions_with_large_parameters/", # adds 1 + "^payload_allocation/", + "^sample_workloads/", + # "^sandboxes/", # adds 12 + # "^shared_memory/", # adds 4 + "^shared_memory/copy_to_slice/1MB$", + "^shared_memory/fill/1MB$", + # "^slot_pool/", # adds 2 + "^slot_pool/alloc_dealloc_128$", + # "^snapshot_files/", # adds 19 + "^snapshot_files/load_snapshot/large$", + "^snapshot_files/load_snapshot_unverified/large$", + "^snapshot_files/load_snapshot_unverified/medium$", + "^snapshot_files/load_snapshot_unverified/small$", + "^snapshot_files/save_snapshot/large$", + # "^snapshots/", # adds 8 + "^virtq_readonly/", + # "^virtq_readwrite/", # adds 4 + "^virtq_readwrite/slot_pool_segmented/65536$", + "^virtq_readwrite/slot_pool_segmented/8192$", +] + +# Noisy members of otherwise stable groups, with the measured drift. +# +# Every `sandboxes` benchmark drifted by more than 9%, reaching 218% on +# `create_initialized/small`, so that group is absent from the allowlist rather +# than listed here. `function_call_serialization` (9% to 15%) and +# `guest_functions_with_large_parameters` (8%) are absent for the same reason, +# as are the `slot_pool` and `virtq_readwrite` members left out above. +denylist = [ + "^guest_calls/call_with_restore/large$", # 13% + "^guest_calls/call_with_restore/medium$", # 16% + "^guest_calls/call_with_restore/small$", # 11% + "^guest_calls/interrupt_latency$", # 51% + "^virtq_readonly/slot_pool_segmented/8192$", # 19% + "^virtq_readonly/slot_pool_segmented_fragmented/8192$", # 11% +] diff --git a/src/hyperlight_ci/Cargo.toml b/src/hyperlight_ci/Cargo.toml index 9243145387..0f8138e317 100644 --- a/src/hyperlight_ci/Cargo.toml +++ b/src/hyperlight_ci/Cargo.toml @@ -13,7 +13,9 @@ workspace = true anyhow = "1" clap = { version = "4.6.1", features = ["derive"] } criterion-markdown = "0.1.2" -criterion-swarm = "0.2" +criterion-swarm = "0.2.2" +regex = "1" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" -tokio = { version = "1", features = ["rt", "macros"] } \ No newline at end of file +tokio = { version = "1", features = ["rt", "macros"] } +toml = "1" \ No newline at end of file diff --git a/src/hyperlight_ci/src/bench.rs b/src/hyperlight_ci/src/bench.rs index 3ab0965665..6bf817219c 100644 --- a/src/hyperlight_ci/src/bench.rs +++ b/src/hyperlight_ci/src/bench.rs @@ -2,12 +2,15 @@ // Copyright 2025 The Hyperlight Authors. //! The `bench` subcommand: runs criterion benchmarks in parallel via criterion-swarm. +use std::collections::HashSet; use std::io::IsTerminal; use std::path::PathBuf; use anyhow::Context; use criterion_swarm::{CriterionSwarm, OutputMode}; +use crate::config::BenchConfig; + /// An output mode flag for `--build-output` / `--benchmarks-output`. #[derive(Clone, Debug)] pub(crate) struct OutputModeFlags(OutputMode); @@ -66,12 +69,22 @@ pub struct BenchArgs { #[arg(short = 'F', long)] pub features: Vec, + /// Run only the benchmarks selected by this config file + #[arg(long, value_name = "PATH")] + pub config_file: Option, + /// 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 config = args + .config_file + .as_deref() + .map(BenchConfig::load) + .transpose()?; + let mut swarm = CriterionSwarm::builder().jobs(args.jobs); if !args.binary.is_empty() { @@ -112,10 +125,19 @@ pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { .benchmarks(bench_mode), ); - let swarm = swarm + let mut swarm = swarm .prepare() .await .context("Failed to prepare criterion swarm")?; + + if let Some(config) = &config { + let selected: HashSet = config + .select(swarm.benchmarks().into_iter().map(str::to_string))? + .into_iter() + .collect(); + swarm.retain(|name| selected.contains(name)); + } + if bench_mode == (bench_mode | OutputMode::SUMMARY) { let total = swarm.benchmarks().len(); let jobs = swarm.jobs().min(total); diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs index 1410b94dae..a9606dcfcb 100644 --- a/src/hyperlight_ci/src/bench_report.rs +++ b/src/hyperlight_ci/src/bench_report.rs @@ -9,6 +9,8 @@ use anyhow::{Context, Result}; use clap::Args; use criterion_swarm::{CriterionSwarm, NoopReporter}; +use crate::config::BenchConfig; + /// Command-line arguments for the `bench-report` subcommand. #[derive(Args)] pub struct BenchReportArgs { @@ -25,6 +27,10 @@ pub struct BenchReportArgs { #[arg(long)] pub collapsible: Option, + /// Report only the benchmarks selected by this config file + #[arg(long, value_name = "PATH")] + pub config_file: Option, + /// Additional arguments to forward to criterion benchmarks (e.g. filter, --exact) #[arg(trailing_var_arg = true, allow_hyphen_values = true)] pub bench_args: Vec, @@ -32,24 +38,28 @@ pub struct BenchReportArgs { /// Entry point for the bench-report subcommand. pub async fn run(args: BenchReportArgs) -> Result<()> { - let allowlist = build_allowlist(&args).await?; + let mut benchmarks = discover_benchmarks(&args).await?; + + if let Some(path) = &args.config_file { + benchmarks = BenchConfig::load(path)?.select(benchmarks)?; + } let options = criterion_markdown::RenderOptions { collapsible: args.collapsible, }; let markdown = - criterion_markdown::render_with_options(&args.criterion_dir, &allowlist, &options)?; + criterion_markdown::render_with_options(&args.criterion_dir, &benchmarks, &options)?; print!("{markdown}"); Ok(()) } -/// Builds an allowlist of benchmark full_ids by discovering benchmarks via CriterionSwarm. +/// Discovers benchmark full_ids via CriterionSwarm. /// /// All trailing arguments (filter, --exact, etc.) are forwarded as bench args /// to CriterionSwarm so it handles filtering during discovery. -async fn build_allowlist(args: &BenchReportArgs) -> Result> { +async fn discover_benchmarks(args: &BenchReportArgs) -> Result> { let mut swarm = CriterionSwarm::builder(); if !args.binary.is_empty() { diff --git a/src/hyperlight_ci/src/config.rs b/src/hyperlight_ci/src/config.rs new file mode 100644 index 0000000000..355de9a7a6 --- /dev/null +++ b/src/hyperlight_ci/src/config.rs @@ -0,0 +1,224 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2025 The Hyperlight Authors. +//! The benchmark report configuration shared by the `bench` and `bench-report` subcommands. + +use std::path::Path; + +use anyhow::{Context, Result, bail}; +use regex::RegexSet; +use serde::Deserialize; + +/// Unknown keys are rejected so a stale key cannot silently disable filtering. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct ConfigFile { + #[serde(default)] + allowlist: Vec, + #[serde(default)] + denylist: Vec, +} + +/// Benchmark id patterns selecting which results are reported. +#[derive(Debug)] +pub struct BenchConfig { + allow: RegexSet, + deny: RegexSet, +} + +impl BenchConfig { + /// Read `allowlist` and `denylist` pattern arrays from a TOML file. + pub fn load(path: &Path) -> Result { + let text = std::fs::read_to_string(path) + .with_context(|| format!("Failed to read benchmark config {}", path.display()))?; + + Self::parse(&text).with_context(|| format!("Invalid benchmark config {}", path.display())) + } + + fn parse(text: &str) -> Result { + let file: ConfigFile = toml::from_str(text)?; + + Ok(Self { + allow: RegexSet::new(&file.allowlist)?, + deny: RegexSet::new(&file.denylist)?, + }) + } + + /// Keep the selected benchmarks, rejecting allowlist patterns that match nothing. + /// + /// Empty lists keep every benchmark. A denylist pattern matching nothing is + /// accepted, because a benchmark may be absent on some platforms. + pub fn select(&self, benchmarks: impl IntoIterator) -> Result> { + let mut used = vec![false; self.allow.len()]; + let mut selected = Vec::new(); + + for benchmark in benchmarks { + let matches = self.allow.matches(&benchmark); + for index in matches.iter() { + used[index] = true; + } + + let allowed = self.allow.is_empty() || matches.matched_any(); + if allowed && !self.deny.is_match(&benchmark) { + selected.push(benchmark); + } + } + + let stale: Vec<&str> = self + .allow + .patterns() + .iter() + .zip(&used) + .filter(|(_, used)| !**used) + .map(|(pattern, _)| pattern.as_str()) + .collect(); + + if !stale.is_empty() { + bail!( + "Benchmark allowlist patterns match no benchmark: {}", + stale.join(", ") + ); + } + + if selected.is_empty() && self.filters() { + bail!("Benchmark config excludes every benchmark"); + } + + Ok(selected) + } + + /// Whether the config restricts the reported benchmarks at all. + fn filters(&self) -> bool { + !self.allow.is_empty() || !self.deny.is_empty() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn benchmarks(names: &[&str]) -> Vec { + names.iter().map(|name| name.to_string()).collect() + } + + #[test] + fn selects_only_allowlisted_benchmarks() { + let config = BenchConfig::parse(r#"allowlist = ["^sandboxes/", "^guest_calls/"]"#).unwrap(); + let selected = config + .select(benchmarks(&[ + "sandboxes/create", + "snapshots/save", + "guest_calls/small", + ])) + .unwrap(); + + assert_eq!(selected, ["sandboxes/create", "guest_calls/small"]); + } + + #[test] + fn denylist_subtracts_from_the_allowlist() { + let config = BenchConfig::parse( + r#" + allowlist = ["^sandboxes/"] + denylist = ["^sandboxes/noisy"] + "#, + ) + .unwrap(); + + let selected = config + .select(benchmarks(&["sandboxes/create", "sandboxes/noisy_case"])) + .unwrap(); + + assert_eq!(selected, ["sandboxes/create"]); + } + + #[test] + fn empty_allowlist_keeps_everything_not_denied() { + let config = BenchConfig::parse(r#"denylist = ["^snapshots/"]"#).unwrap(); + let selected = config + .select(benchmarks(&["sandboxes/create", "snapshots/save"])) + .unwrap(); + + assert_eq!(selected, ["sandboxes/create"]); + } + + #[test] + fn rejects_allowlist_patterns_matching_nothing() { + let config = + BenchConfig::parse(r#"allowlist = ["^sandboxes/", "^renamed_away/"]"#).unwrap(); + let error = config + .select(benchmarks(&["sandboxes/create"])) + .unwrap_err() + .to_string(); + + assert!(error.contains("^renamed_away/"), "{error}"); + assert!(!error.contains("^sandboxes/"), "{error}"); + } + + #[test] + fn accepts_denylist_patterns_matching_nothing() { + let config = BenchConfig::parse( + r#" + allowlist = ["^sandboxes/"] + denylist = ["^windows_only/"] + "#, + ) + .unwrap(); + + let selected = config.select(benchmarks(&["sandboxes/create"])).unwrap(); + assert_eq!(selected, ["sandboxes/create"]); + } + + #[test] + fn rejects_a_config_that_excludes_everything() { + let config = BenchConfig::parse( + r#" + allowlist = ["^sandboxes/"] + denylist = ["^sandboxes/"] + "#, + ) + .unwrap(); + + let error = config + .select(benchmarks(&["sandboxes/create"])) + .unwrap_err() + .to_string(); + + assert!(error.contains("excludes every benchmark"), "{error}"); + } + + #[test] + fn config_without_keys_keeps_every_benchmark() { + let config = BenchConfig::parse("").unwrap(); + let selected = config + .select(benchmarks(&["sandboxes/create", "snapshots/save"])) + .unwrap(); + + assert_eq!(selected, ["sandboxes/create", "snapshots/save"]); + } + + #[test] + fn empty_lists_keep_every_benchmark() { + let config = BenchConfig::parse("allowlist = []\ndenylist = []").unwrap(); + let selected = config + .select(benchmarks(&["sandboxes/create", "snapshots/save"])) + .unwrap(); + + assert_eq!(selected, ["sandboxes/create", "snapshots/save"]); + } + + #[test] + fn rejects_unknown_keys() { + let error = BenchConfig::parse(r#"patterns = ["^sandboxes/"]"#) + .unwrap_err() + .to_string(); + assert!(error.contains("unknown field"), "{error}"); + } + + #[test] + fn rejects_invalid_pattern() { + let error = BenchConfig::parse(r#"allowlist = ["^sandboxes/("]"#) + .unwrap_err() + .to_string(); + assert!(error.contains("regex parse error"), "{error}"); + } +} diff --git a/src/hyperlight_ci/src/main.rs b/src/hyperlight_ci/src/main.rs index 76f9135bac..1ce703aae5 100644 --- a/src/hyperlight_ci/src/main.rs +++ b/src/hyperlight_ci/src/main.rs @@ -2,6 +2,7 @@ // Copyright 2025 The Hyperlight Authors. mod bench; mod bench_report; +mod config; use clap::{Parser, Subcommand}; From 0552dd98172f291e30f3e42602372ce33fa46f44 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Tue, 22 Sep 2026 22:39:50 +0100 Subject: [PATCH 04/12] feat(ci): hold a sandbox resident while benchmarking A vCPU created without an in-kernel LAPIC bumps the kernel's `kvm_has_noapic_vcpu` static key, and teardown drops it again. Each transition through zero rewrites kernel text and IPIs every core. Benchmarks that create and drop sandboxes leave no resident VM between iterations, so they cross that boundary constantly: a full suite run issues 176k broadcast IPIs, against 560 with one sandbox held. `cargo ci bench` now keeps one sandbox alive for the duration of a run, so that cost lands on neither the benchmark that triggers it nor its neighbours. Excluding benchmarks that ran on CPU 0, which has its own much larger effect, median drift for the sandbox group falls from 6.5% to 1.7%. Pass `--no-ballast` to measure the cold path instead. The helper is an example rather than a dependency of this crate, keeping hyperlight-host out of the CI tool's build. It exits on stdin EOF, so it cannot outlive the run even when this process is killed without unwinding. Signed-off-by: Jorge Prendes --- src/hyperlight_ci/src/ballast.rs | 98 ++++++++++++++++++++ src/hyperlight_ci/src/bench.rs | 17 +++- src/hyperlight_ci/src/main.rs | 1 + src/hyperlight_host/examples/ballast/main.rs | 29 ++++++ 4 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 src/hyperlight_ci/src/ballast.rs create mode 100644 src/hyperlight_host/examples/ballast/main.rs diff --git a/src/hyperlight_ci/src/ballast.rs b/src/hyperlight_ci/src/ballast.rs new file mode 100644 index 0000000000..32113fca7c --- /dev/null +++ b/src/hyperlight_ci/src/ballast.rs @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2025 The Hyperlight Authors. +//! A resident VM held for the duration of a benchmark run. + +use std::io::{BufRead, BufReader}; +use std::path::PathBuf; +use std::process::{Child, ChildStdin, Command, Stdio}; + +use anyhow::{Context, bail}; + +/// Line the helper prints once its sandbox is live. +const READY: &str = "ballast ready"; + +/// A sandbox held alive for a whole benchmark run. +/// +/// Benchmarks that create and drop sandboxes leave the host with no resident +/// VM between iterations. Crossing that boundary toggles a kernel static key, +/// and each toggle patches kernel text and IPIs every core. That cost lands on +/// whichever benchmark happens to trigger it, so holding one sandbox resident +/// keeps it out of the measurements. +pub(crate) struct Ballast { + child: Child, + /// The helper exits when this closes, which the OS does for us if this + /// process is killed before [`Drop`] can run. + _stdin: ChildStdin, +} + +impl Ballast { + /// Build the helper, start it, and wait until its sandbox is live. + pub(crate) fn start() -> anyhow::Result { + let exe = build().context("Failed to build the ballast helper")?; + + let mut child = Command::new(&exe) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .with_context(|| format!("Failed to start the ballast helper {}", exe.display()))?; + + let stdin = child.stdin.take().expect("stdin is piped"); + let stdout = child.stdout.take().expect("stdout is piped"); + let mut line = String::new(); + BufReader::new(stdout) + .read_line(&mut line) + .context("Failed to read readiness from the ballast helper")?; + + if line.trim() != READY { + bail!("Ballast helper did not become ready"); + } + + Ok(Self { + child, + _stdin: stdin, + }) + } +} + +impl Drop for Ballast { + fn drop(&mut self) { + let _ = self.child.kill(); + let _ = self.child.wait(); + } +} + +/// Build the ballast example and return the executable cargo produced. +fn build() -> anyhow::Result { + let cargo = std::env::var("CARGO").unwrap_or_else(|_| "cargo".to_string()); + + let output = Command::new(cargo) + .args([ + "build", + "--release", + "--package", + "hyperlight-host", + "--example", + "ballast", + "--message-format=json-render-diagnostics", + ]) + .stderr(Stdio::inherit()) + .output()?; + + if !output.status.success() { + bail!("cargo build failed for the ballast helper"); + } + + for line in output.stdout.as_slice().lines() { + let Ok(line) = line else { continue }; + let Ok(msg) = serde_json::from_str::(&line) else { + continue; + }; + if msg["reason"] == "compiler-artifact" && msg["target"]["name"] == "ballast" { + if let Some(exe) = msg["executable"].as_str() { + return Ok(PathBuf::from(exe)); + } + } + } + + bail!("cargo reported no executable for the ballast helper") +} diff --git a/src/hyperlight_ci/src/bench.rs b/src/hyperlight_ci/src/bench.rs index 6bf817219c..5c1480773f 100644 --- a/src/hyperlight_ci/src/bench.rs +++ b/src/hyperlight_ci/src/bench.rs @@ -9,6 +9,7 @@ use std::path::PathBuf; use anyhow::Context; use criterion_swarm::{CriterionSwarm, OutputMode}; +use crate::ballast::Ballast; use crate::config::BenchConfig; /// An output mode flag for `--build-output` / `--benchmarks-output`. @@ -73,6 +74,10 @@ pub struct BenchArgs { #[arg(long, value_name = "PATH")] pub config_file: Option, + /// Run without holding a sandbox resident for the duration of the run + #[arg(long)] + pub no_ballast: bool, + /// Additional arguments to forward to criterion benchmarks #[arg(trailing_var_arg = true, allow_hyphen_values = true)] pub bench_args: Vec, @@ -143,5 +148,15 @@ pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { let jobs = swarm.jobs().min(total); println!("Running {total} benchmarks with parallelism {jobs}"); } - swarm.run().await.context("Failed to run criterion swarm") + + // Held until the run finishes. + let ballast = if args.no_ballast { + None + } else { + Some(Ballast::start()?) + }; + + let result = swarm.run().await.context("Failed to run criterion swarm"); + drop(ballast); + result } diff --git a/src/hyperlight_ci/src/main.rs b/src/hyperlight_ci/src/main.rs index 1ce703aae5..24b3183438 100644 --- a/src/hyperlight_ci/src/main.rs +++ b/src/hyperlight_ci/src/main.rs @@ -1,5 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2025 The Hyperlight Authors. +mod ballast; mod bench; mod bench_report; mod config; diff --git a/src/hyperlight_host/examples/ballast/main.rs b/src/hyperlight_host/examples/ballast/main.rs new file mode 100644 index 0000000000..2d96fdcc17 --- /dev/null +++ b/src/hyperlight_host/examples/ballast/main.rs @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2025 The Hyperlight Authors. +//! Holds one sandbox alive and then idles. +//! +//! A vCPU created without an in-kernel LAPIC bumps the kernel's +//! `kvm_has_noapic_vcpu` static key, and teardown drops it again. Each +//! transition through zero patches kernel text, which IPIs every core. +//! Keeping one sandbox resident holds the count above zero, so a benchmark +//! that creates and drops sandboxes never crosses that boundary. +//! Parks after startup, so it consumes no CPU while a benchmark runs. + +use std::io::Read; + +use hyperlight_host::SandboxBuilder; + +fn main() -> hyperlight_host::Result<()> { + let _sandbox = + SandboxBuilder::from_file(hyperlight_testing::simple_guest_as_pathbuf()).build()?; + + // Readers wait for this line before starting to measure. + println!("ballast ready"); + + // Stdin reaches EOF when the parent closes it or dies, so the sandbox never + // outlives the run that asked for it. + let mut discard = Vec::new(); + let _ = std::io::stdin().read_to_end(&mut discard); + + Ok(()) +} From 8a14508cbbba0a3ba046041eba8c52f9eaa360b4 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Wed, 23 Sep 2026 15:01:45 +0100 Subject: [PATCH 05/12] feat(ci): record and read what each benchmark run measured Criterion stores an id per result directory but nothing about the run as a whole, and results accumulate: a directory keeps benchmarks that no longer exist, indistinguishable from the ones just measured. CI compounds this by unpacking a baseline into the same directory before running, so its uploaded artifact holds 155 result directories for a suite of 87. `cargo ci bench` writes `benchmarks.json` next to the results, listing the benchmarks the run covers along with a timestamp and the host it ran on. The list is the post-filter set, so it describes what was measured rather than what was discovered. `bench-report` takes the ids from there when the results carry one, so a criterion directory from elsewhere, a CI artifact say, reads without a toolchain and without the checkout matching. Naming them by listing the binaries builds them first and describes the current checkout instead, which is the same thing only while reporting a local run of the current tree. Rendering the downloaded results of a full suite takes 11ms rather than a build. Explicit `--binary` or trailing bench args still list the binaries, since both name benchmarks the manifest cannot filter, as do results from before the manifest existed. Signed-off-by: Jorge Prendes --- src/hyperlight_ci/src/bench.rs | 4 + src/hyperlight_ci/src/bench_report.rs | 15 +++- src/hyperlight_ci/src/main.rs | 1 + src/hyperlight_ci/src/manifest.rs | 118 ++++++++++++++++++++++++++ 4 files changed, 135 insertions(+), 3 deletions(-) create mode 100644 src/hyperlight_ci/src/manifest.rs diff --git a/src/hyperlight_ci/src/bench.rs b/src/hyperlight_ci/src/bench.rs index 5c1480773f..6fe4c99390 100644 --- a/src/hyperlight_ci/src/bench.rs +++ b/src/hyperlight_ci/src/bench.rs @@ -11,6 +11,7 @@ use criterion_swarm::{CriterionSwarm, OutputMode}; use crate::ballast::Ballast; use crate::config::BenchConfig; +use crate::manifest; /// An output mode flag for `--build-output` / `--benchmarks-output`. #[derive(Clone, Debug)] @@ -149,6 +150,9 @@ pub async fn run(mut args: BenchArgs) -> anyhow::Result<()> { println!("Running {total} benchmarks with parallelism {jobs}"); } + manifest::write(swarm.benchmarks().into_iter().map(str::to_string)) + .context("Failed to write the benchmark manifest")?; + // Held until the run finishes. let ballast = if args.no_ballast { None diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs index a9606dcfcb..e25f79f4c1 100644 --- a/src/hyperlight_ci/src/bench_report.rs +++ b/src/hyperlight_ci/src/bench_report.rs @@ -10,6 +10,7 @@ use clap::Args; use criterion_swarm::{CriterionSwarm, NoopReporter}; use crate::config::BenchConfig; +use crate::manifest; /// Command-line arguments for the `bench-report` subcommand. #[derive(Args)] @@ -55,11 +56,19 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { Ok(()) } -/// Discovers benchmark full_ids via CriterionSwarm. +/// Benchmark ids for the results being reported. /// -/// All trailing arguments (filter, --exact, etc.) are forwarded as bench args -/// to CriterionSwarm so it handles filtering during discovery. +/// A run records what it measured, so prefer that: listing the binaries builds +/// them and describes the current checkout rather than the run in hand, which +/// differ whenever results come from elsewhere. Explicit binaries or bench args +/// ask for the binaries, and older results carry no manifest. async fn discover_benchmarks(args: &BenchReportArgs) -> Result> { + if args.binary.is_empty() && args.bench_args.is_empty() { + if let Some(benchmarks) = manifest::read(&args.criterion_dir)? { + return Ok(benchmarks); + } + } + let mut swarm = CriterionSwarm::builder(); if !args.binary.is_empty() { diff --git a/src/hyperlight_ci/src/main.rs b/src/hyperlight_ci/src/main.rs index 24b3183438..587e2e23ca 100644 --- a/src/hyperlight_ci/src/main.rs +++ b/src/hyperlight_ci/src/main.rs @@ -4,6 +4,7 @@ mod ballast; mod bench; mod bench_report; mod config; +mod manifest; use clap::{Parser, Subcommand}; diff --git a/src/hyperlight_ci/src/manifest.rs b/src/hyperlight_ci/src/manifest.rs new file mode 100644 index 0000000000..3836aafde0 --- /dev/null +++ b/src/hyperlight_ci/src/manifest.rs @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2025 The Hyperlight Authors. +//! A record of what a benchmark run measured, and on what. + +use std::path::{Path, PathBuf}; +use std::time::{SystemTime, UNIX_EPOCH}; +use std::{env, fs}; + +use anyhow::{Context, Result}; +use serde::{Deserialize, Serialize}; + +/// Name of the manifest within the criterion results directory. +const FILE_NAME: &str = "benchmarks.json"; + +/// Written alongside the criterion results, so a run can be interpreted without +/// the benchmark binaries that produced it. +/// +/// Criterion records an id per result directory but nothing about the run as a +/// whole. Results also accumulate: a directory carries benchmarks that no +/// longer exist, indistinguishable from the ones just measured. This lists what +/// the run actually covered. +#[derive(Serialize, Deserialize)] +struct Manifest { + /// Seconds since the Unix epoch. Criterion timestamps nothing, and archived + /// results lose their file times. + timestamp: u64, + host: Host, + benchmarks: Vec, +} + +#[derive(Serialize, Deserialize)] +struct Host { + os: String, + arch: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + logical_cpus: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + cpu_vendor: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + cpu_model: Option, +} + +/// Where criterion keeps its results. +fn criterion_dir() -> PathBuf { + env::var_os("CRITERION_HOME") + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from("target").join("criterion")) +} + +#[cfg(target_os = "linux")] +fn cpu_vendor_and_model() -> (Option, Option) { + let Ok(text) = fs::read_to_string("/proc/cpuinfo") else { + return (None, None); + }; + let field = |key: &str| { + text.lines() + .find_map(|l| l.split_once(':').filter(|(k, _)| k.trim() == key)) + .map(|(_, v)| v.trim().to_string()) + }; + (field("vendor_id"), field("model name")) +} + +#[cfg(target_os = "windows")] +fn cpu_vendor_and_model() -> (Option, Option) { + // e.g. "Intel64 Family 6 Model 154 Stepping 3, GenuineIntel" + let model = env::var("PROCESSOR_IDENTIFIER").ok(); + let vendor = model + .as_deref() + .and_then(|m| m.rsplit_once(',')) + .map(|(_, v)| v.trim().to_string()); + (vendor, model) +} + +#[cfg(not(any(target_os = "linux", target_os = "windows")))] +fn cpu_vendor_and_model() -> (Option, Option) { + (None, None) +} + +/// Record `benchmarks` as the contents of the run about to start. +pub(crate) fn write(benchmarks: impl IntoIterator) -> Result<()> { + let (cpu_vendor, cpu_model) = cpu_vendor_and_model(); + let mut benchmarks: Vec = benchmarks.into_iter().collect(); + benchmarks.sort(); + + let manifest = Manifest { + timestamp: SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or_default(), + host: Host { + os: env::consts::OS.to_string(), + arch: env::consts::ARCH.to_string(), + logical_cpus: std::thread::available_parallelism().ok().map(Into::into), + cpu_vendor, + cpu_model, + }, + benchmarks, + }; + + let dir = criterion_dir(); + fs::create_dir_all(&dir).with_context(|| format!("Failed to create {}", dir.display()))?; + let path = dir.join(FILE_NAME); + let json = serde_json::to_string_pretty(&manifest)?; + fs::write(&path, json).with_context(|| format!("Failed to write {}", path.display())) +} + +/// The benchmarks a run recorded, or `None` when it left no manifest. +pub(crate) fn read(dir: &Path) -> Result>> { + let path = dir.join(FILE_NAME); + let Ok(text) = fs::read_to_string(&path) else { + return Ok(None); + }; + + let manifest: Manifest = serde_json::from_str(&text) + .with_context(|| format!("Failed to parse {}", path.display()))?; + + Ok(Some(manifest.benchmarks)) +} From 2cd9e19943b4f40bc30be666947156fb5947139f Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Wed, 23 Sep 2026 16:51:07 +0100 Subject: [PATCH 06/12] feat(ci): report the benchmarks of a CI run Comparing what CI measured meant downloading six artifacts by hand, unpacking each somewhere, and running the report once per configuration. The results are richer than the comment CI posts, holding every benchmark rather than the reported subset and the samples behind each estimate, so reaching for them is worth making cheap. `--candidate` and `--baseline` say where each side comes from: a criterion directory, `run:`, or `pr:`, which takes the most recent run that still has its artifacts, since the newest is often a label check or one whose benchmarks have not finished. A whole run renders as one section per hypervisor and cpu vendor, the way the pull request comment reads. Runs land under `target/ci-runs` and are reused, artifacts being immutable. A run is about 400MB unpacked, and the first report of one waits on the download. criterion-markdown 0.2.1 computes changes when it renders, so the report needs both datasets present. A run overwrites the baseline it compares against, so CI keeps the previous results in their own criterion root. Criterion keeps the last run of a directory in `new` and the one before it in `base`, so a directory on its own reports the last run against the previous one, and another directory is compared through its own last run. Results with no baseline to compare against are reported on their own. CI covers every hypervisor and cpu vendor, and comparing results measured on different machines says nothing. Each side is paired with the one that ran on the same kind of machine, read from the artifact name or from the operating system, cpu vendor and hypervisor a run records. What fits nothing, or several, is reported without a comparison. The ids come from the manifest, so they describe what the run measured rather than this checkout, which need not even be the same commit. Signed-off-by: Jorge Prendes --- .github/workflows/dep_benchmarks.yml | 8 +- Cargo.lock | 4 +- Justfile | 6 +- docs/benchmarking-hyperlight.md | 13 +- src/hyperlight_ci/Cargo.toml | 2 +- src/hyperlight_ci/src/bench_report.rs | 268 ++++++++++++++++++++++++-- src/hyperlight_ci/src/main.rs | 1 + src/hyperlight_ci/src/manifest.rs | 58 ++++-- src/hyperlight_ci/src/remote.rs | 151 +++++++++++++++ 9 files changed, 470 insertions(+), 41 deletions(-) create mode 100644 src/hyperlight_ci/src/remote.rs diff --git a/.github/workflows/dep_benchmarks.yml b/.github/workflows/dep_benchmarks.yml index fcd624af6b..0960c874a1 100644 --- a/.github/workflows/dep_benchmarks.yml +++ b/.github/workflows/dep_benchmarks.yml @@ -127,14 +127,14 @@ jobs: uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: benchmarks_${{ runner.os }}_${{ inputs.hypervisor }}_${{ inputs.cpu_vendor }} - path: ./target/criterion/ + path: ./target/criterion-baseline/ run-id: ${{ inputs.baseline_run_id }} github-token: ${{ secrets.GITHUB_TOKEN }} continue-on-error: true - name: Download baseline from release if: ${{ inputs.baseline_run_id == '' }} - run: just bench-download ${{ runner.os }} ${{ inputs.hypervisor }} ${{ inputs.cpu_vendor }} ${{ inputs.baseline_tag }} + run: just bench-download ${{ runner.os }} ${{ inputs.hypervisor }} ${{ inputs.cpu_vendor }} ${{ inputs.baseline_tag }} target/criterion-baseline env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} continue-on-error: true @@ -142,8 +142,10 @@ jobs: - name: Run benchmarks run: just bench-ci main + # The run overwrites its own `main` baseline, so changes are measured + # against the previous results kept aside. - name: Create benchmarks report - run: cargo ci bench-report --config-file bench_report.toml --collapsible '${{ inputs.hypervisor }} / ${{ inputs.cpu_vendor }} (${{ runner.os }})' > target/criterion/benchmark.md + run: cargo ci bench-report --config-file bench_report.toml --baseline target/criterion-baseline --collapsible '${{ inputs.hypervisor }} / ${{ inputs.cpu_vendor }} (${{ runner.os }})' > target/criterion/benchmark.md - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: diff --git a/Cargo.lock b/Cargo.lock index 7c45da51b2..1299050f54 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -673,9 +673,9 @@ dependencies = [ [[package]] name = "criterion-markdown" -version = "0.1.2" +version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "72741fa695d01bfab5c47a083ef370db8235c90aee6033136ce49aa7dcd2a413" +checksum = "0c723767825fca388c4d768dfbe983d7ba3f7e0b3104292f85c8ca8bd6880822" dependencies = [ "anyhow", "serde", diff --git a/Justfile b/Justfile index 75a9a8a764..7277c1abe7 100644 --- a/Justfile +++ b/Justfile @@ -430,10 +430,10 @@ tar-static-lib: (build-rust-capi "release") (build-rust-capi "debug") # Options for Linux hypervisor: "kvm", "mshv3" # Options for Windows hypervisor: "hyperv", "hyperv-ws2025" # Options for cpu_vendor: "amd", "intel" -bench-download os hypervisor cpu_vendor tag="": +bench-download os hypervisor cpu_vendor tag="" dest="target/criterion": gh release download {{ tag }} -D ./target/ -p benchmarks_{{ os }}_{{ hypervisor }}_{{ cpu_vendor }}.tar.gz - mkdir -p target/criterion {{ if os() == "windows" { "-Force" } else { "" } }} - tar -zxvf target/benchmarks_{{ os }}_{{ hypervisor }}_{{ cpu_vendor }}.tar.gz -C target/criterion/ --strip-components=1 + mkdir -p {{ dest }} {{ if os() == "windows" { "-Force" } else { "" } }} + tar -zxvf target/benchmarks_{{ os }}_{{ hypervisor }}_{{ cpu_vendor }}.tar.gz -C {{ dest }}/ --strip-components=1 # Warning: compares to and then OVERWRITES the given baseline bench-ci baseline features="": diff --git a/docs/benchmarking-hyperlight.md b/docs/benchmarking-hyperlight.md index 811ef18f06..246bc905c2 100644 --- a/docs/benchmarking-hyperlight.md +++ b/docs/benchmarking-hyperlight.md @@ -72,6 +72,17 @@ Found 1 outliers among 100 measurements (1.00%) ## Running benchmarks locally -Use `just bench` to run benchmarks with release builds (the only supported configuration). Comparing local benchmark results to GitHub-saved benchmarks doesn't make much sense, since you'd be using different hardware, but you can use `just bench-download os hypervisor cpu_vendor [tag] ` to download and extract the GitHub release benchmarks to the correct folder. You can then run `just bench-ci main` to compare to (and overwrite) the previous release benchmarks. Note that `main` is the name of the baselines stored in GitHub. +Use `just bench` to run benchmarks with release builds (the only supported configuration). Comparing local benchmark results to GitHub-saved benchmarks doesn't make much sense, since you'd be using different hardware, but you can use `just bench-download os hypervisor cpu_vendor [tag] [dest]` to download and extract the GitHub release benchmarks. Pass a `dest` outside `target/criterion` and point `cargo ci bench-report --baseline-root` at it, since `just bench-ci main` overwrites the `main` baseline it runs against. Note that `main` is the name of the baselines stored in GitHub. + +`cargo ci bench-report` renders the comparison. `--candidate` and `--baseline` say where each side comes from: a criterion directory, `run:` for a CI run, or `pr:` for the latest run of a pull request. Criterion keeps the last run of a directory in `new` and the one before it in `base`, so a directory on its own reports the last run against the previous one. + +CI results cover every hypervisor and cpu vendor, so results are paired with the ones measured on the same kind of machine. A run records its own operating system, cpu vendor and hypervisor, and CI artifacts are named after the configuration that produced them. Results that fit no counterpart, or several, are reported without a comparison. + +```sh +# a local run against results downloaded into their own directory +cargo ci bench-report --baseline target/criterion-baseline +# this machine against the configuration in CI that matches it +cargo ci bench-report --baseline pr:1529 +``` **Important**: The `just bench` command uses release builds by default to ensure meaningful performance measurements. For profiling purposes, you can compile benchmarks with debug symbols by running `cargo bench` directly. diff --git a/src/hyperlight_ci/Cargo.toml b/src/hyperlight_ci/Cargo.toml index 0f8138e317..087bcdd5f8 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.1.2" +criterion-markdown = "0.2.1" criterion-swarm = "0.2.2" regex = "1" serde = { version = "1.0", features = ["derive"] } diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs index e25f79f4c1..465a133028 100644 --- a/src/hyperlight_ci/src/bench_report.rs +++ b/src/hyperlight_ci/src/bench_report.rs @@ -3,14 +3,112 @@ //! The `bench-report` subcommand: generates a markdown table from existing //! criterion benchmark results in `target/criterion/`. -use std::path::PathBuf; +use std::fs; +use std::path::{Path, PathBuf}; +use std::str::FromStr; use anyhow::{Context, Result}; use clap::Args; use criterion_swarm::{CriterionSwarm, NoopReporter}; use crate::config::BenchConfig; -use crate::manifest; +use crate::{manifest, remote}; + +/// Where downloaded runs are kept. +const RUN_CACHE: &str = "target/ci-runs"; + +/// Where results come from, either a criterion directory or CI. +#[derive(Clone)] +pub enum Source { + Dir(PathBuf), + Run(u64), + PullRequest(u64), +} + +impl FromStr for Source { + type Err = String; + + fn from_str(value: &str) -> Result { + // Anything else is a path, so windows drive letters stay paths. + let Some((kind @ ("run" | "pr"), id)) = value.split_once(':') else { + return Ok(Self::Dir(value.into())); + }; + let id = id + .parse() + .map_err(|_| format!("`{id}` is not a {kind} number"))?; + Ok(match kind { + "run" => Self::Run(id), + _ => Self::PullRequest(id), + }) + } +} + +/// Results to report, identified by the host that produced them. +struct Input { + label: Option, + dir: PathBuf, + host: Option, +} + +/// What distinguishes one set of benchmark results from another. +#[derive(PartialEq)] +struct Identity { + os: String, + /// `amd` or `intel`. + vendor: String, + hypervisor: Option, +} + +impl Identity { + /// Read from the name CI gives an artifact, `Linux_kvm_amd` and so on. + fn from_label(label: &str) -> Option { + let (os, rest) = label.split_once('_')?; + let (hypervisor, vendor) = rest.rsplit_once('_')?; + let os = os.to_lowercase(); + Some(Self { + hypervisor: Some(canonical_hypervisor(&os, hypervisor)), + os, + vendor: vendor.to_lowercase(), + }) + } + + /// Read from what a run recorded about the machine it ran on. + fn from_host(host: &manifest::Host) -> Option { + let vendor = match host.cpu_vendor.as_deref()? { + vendor if vendor.contains("AMD") => "amd", + vendor if vendor.contains("Intel") => "intel", + _ => return None, + }; + Some(Self { + hypervisor: host + .hypervisor + .as_deref() + .map(|name| canonical_hypervisor(&host.os, name)), + os: host.os.clone(), + vendor: vendor.to_string(), + }) + } + + /// Whether both could be the same machine. What one of them does not say + /// cannot contradict the other. + fn matches(&self, other: &Self) -> bool { + self.os == other.os + && self.vendor == other.vendor + && match (&self.hypervisor, &other.hypervisor) { + (Some(ours), Some(theirs)) => ours == theirs, + _ => true, + } + } +} + +/// Windows runs on whp alone, so its artifacts are named after the runner +/// image instead. Elsewhere the name carries a version, `mshv3` for `mshv`. +fn canonical_hypervisor(os: &str, name: &str) -> String { + match os { + "windows" => "whp".to_string(), + _ => name.trim_end_matches(char::is_numeric).to_string(), + } +} /// Command-line arguments for the `bench-report` subcommand. #[derive(Args)] @@ -20,9 +118,22 @@ pub struct BenchReportArgs { #[arg(long)] pub binary: Vec, - /// Path to the criterion output directory - #[arg(long, default_value = "target/criterion")] - pub criterion_dir: PathBuf, + /// Results to report: a criterion directory, `run:` or `pr:` + #[arg(long, value_name = "SOURCE", default_value = "target/criterion")] + pub candidate: Source, + + /// Results to compare against: a criterion directory, `run:` or `pr:`. + /// Defaults to the previous run held in the reported directory. + #[arg(long, value_name = "SOURCE")] + pub baseline: Option, + + /// Repository holding the CI runs + #[arg( + long, + value_name = "OWNER/NAME", + default_value = "hyperlight-dev/hyperlight" + )] + pub repo: String, /// Wrap the output in a collapsible
tag with the given summary text. #[arg(long)] @@ -39,21 +150,146 @@ pub struct BenchReportArgs { /// Entry point for the bench-report subcommand. pub async fn run(args: BenchReportArgs) -> Result<()> { - let mut benchmarks = discover_benchmarks(&args).await?; + let candidates = resolve(&args.candidate, &args.repo)?; + let mut baselines = match &args.baseline { + Some(source) => resolve(source, &args.repo)?, + None => Vec::new(), + }; + + // The first run of a configuration has nothing to compare against, and CI + // carries on with the baseline it could not download. + baselines.retain(|baseline| has_results(&baseline.dir)); + if baselines.is_empty() && args.baseline.is_some() { + eprintln!("No baseline results found, reporting without a comparison"); + } + + // A CI run covers every hypervisor and cpu vendor, one section each. + for candidate in &candidates { + let label = candidate.label.as_deref(); + let markdown = report( + &args, + &candidate.dir, + baseline_for(&baselines, candidate), + title(args.collapsible.as_deref(), label), + ) + .await?; + print!("{markdown}"); + } + + Ok(()) +} + +/// Locate the results `source` points at. +fn resolve(source: &Source, repo: &str) -> Result> { + let run = match source { + Source::Dir(dir) => { + return Ok(vec![Input { + label: None, + host: host_of(dir)?, + dir: dir.clone(), + }]); + } + Source::Run(run) => *run, + Source::PullRequest(pull_request) => remote::latest_run_for(repo, *pull_request)?, + }; + + eprintln!("Fetching run {run} of {repo}"); + remote::fetch(repo, run, Path::new(RUN_CACHE))? + .into_iter() + .map(|results| { + Ok(Input { + // The artifact name says what produced it, so trust it over + // anything an older run left without a hypervisor recorded. + host: Identity::from_label(&results.label) + .map(Some) + .map_or_else(|| host_of(&results.dir), Ok)?, + label: Some(results.label), + dir: results.dir, + }) + }) + .collect() +} + +/// What the run in `dir` recorded about the machine it ran on. +fn host_of(dir: &Path) -> Result> { + Ok(manifest::read(dir)?.and_then(|manifest| Identity::from_host(&manifest.host))) +} + +/// Whether `dir` holds anything to report. Criterion writes nothing until a +/// benchmark runs, so an empty directory is one that never did. +fn has_results(dir: &Path) -> bool { + fs::read_dir(dir).is_ok_and(|mut entries| entries.next().is_some()) +} + +/// The baseline to compare `candidate` against. +fn baseline_for<'a>(baselines: &'a [Input], candidate: &Input) -> Option<&'a Path> { + match baselines { + [] => None, + // Nothing to tell apart, so a lone baseline stands in for whatever it + // is compared against. + [only] if only.host.is_none() || candidate.host.is_none() => Some(&only.dir), + _ => { + let host = candidate.host.as_ref()?; + let what = candidate.label.as_deref().unwrap_or("these results"); + let mut found = baselines.iter().filter(|baseline| { + baseline + .host + .as_ref() + .is_some_and(|other| host.matches(other)) + }); + + match (found.next(), found.next()) { + (Some(baseline), None) => Some(baseline.dir.as_path()), + // Results that do not say which hypervisor produced them can + // fit more than one configuration. + (Some(_), Some(_)) => { + eprintln!("Several baselines fit {what}, reporting them alone"); + None + } + _ => { + eprintln!("Nothing to compare {what} against, reporting them alone"); + None + } + } + } + } +} + +/// Name the report after the configuration it covers. +fn title(summary: Option<&str>, label: Option<&str>) -> Option { + match (summary, label) { + (Some(summary), Some(label)) => Some(format!("{summary} {label}")), + (summary, label) => summary.or(label).map(str::to_string), + } +} + +/// Render the results in `dir`. +async fn report( + args: &BenchReportArgs, + dir: &Path, + baseline_root: Option<&Path>, + title: Option, +) -> Result { + let mut benchmarks = discover_benchmarks(args, dir).await?; if let Some(path) = &args.config_file { benchmarks = BenchConfig::load(path)?.select(benchmarks)?; } - let options = criterion_markdown::RenderOptions { - collapsible: args.collapsible, - }; - let markdown = - criterion_markdown::render_with_options(&args.criterion_dir, &benchmarks, &options)?; + let mut renderer = criterion_markdown::Renderer::new(dir).benchmarks(benchmarks); + + // Criterion keeps the last run of a directory in `new` and the one before + // it in `base`, so another directory is compared through its own last run. + if let Some(root) = baseline_root { + renderer = renderer.baseline_root(root).baseline("new"); + } - print!("{markdown}"); + // The summary doubles as the title of a collapsed report. + if let Some(title) = title { + renderer = renderer.title(title).collapsible(true); + } - Ok(()) + renderer.render() } /// Benchmark ids for the results being reported. @@ -62,10 +298,10 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { /// them and describes the current checkout rather than the run in hand, which /// differ whenever results come from elsewhere. Explicit binaries or bench args /// ask for the binaries, and older results carry no manifest. -async fn discover_benchmarks(args: &BenchReportArgs) -> Result> { +async fn discover_benchmarks(args: &BenchReportArgs, dir: &Path) -> Result> { if args.binary.is_empty() && args.bench_args.is_empty() { - if let Some(benchmarks) = manifest::read(&args.criterion_dir)? { - return Ok(benchmarks); + if let Some(manifest) = manifest::read(dir)? { + return Ok(manifest.benchmarks); } } diff --git a/src/hyperlight_ci/src/main.rs b/src/hyperlight_ci/src/main.rs index 587e2e23ca..5c37698e83 100644 --- a/src/hyperlight_ci/src/main.rs +++ b/src/hyperlight_ci/src/main.rs @@ -5,6 +5,7 @@ mod bench; mod bench_report; mod config; mod manifest; +mod remote; use clap::{Parser, Subcommand}; diff --git a/src/hyperlight_ci/src/manifest.rs b/src/hyperlight_ci/src/manifest.rs index 3836aafde0..05e6d6af62 100644 --- a/src/hyperlight_ci/src/manifest.rs +++ b/src/hyperlight_ci/src/manifest.rs @@ -20,24 +20,27 @@ const FILE_NAME: &str = "benchmarks.json"; /// longer exist, indistinguishable from the ones just measured. This lists what /// the run actually covered. #[derive(Serialize, Deserialize)] -struct Manifest { +pub(crate) struct Manifest { /// Seconds since the Unix epoch. Criterion timestamps nothing, and archived /// results lose their file times. timestamp: u64, - host: Host, - benchmarks: Vec, + pub host: Host, + pub benchmarks: Vec, } #[derive(Serialize, Deserialize)] -struct Host { - os: String, - arch: String, +pub(crate) struct Host { + pub os: String, + pub arch: String, #[serde(default, skip_serializing_if = "Option::is_none")] - logical_cpus: Option, + pub logical_cpus: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - cpu_vendor: Option, + pub cpu_vendor: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - cpu_model: Option, + pub cpu_model: Option, + /// The hypervisor hyperlight would use here, `kvm` and so on. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub hypervisor: Option, } /// Where criterion keeps its results. @@ -76,6 +79,31 @@ fn cpu_vendor_and_model() -> (Option, Option) { (None, None) } +/// Kvm and mshv cannot both be present, so the device that exists names the +/// hypervisor hyperlight would pick. +#[cfg(target_os = "linux")] +fn hypervisor() -> Option { + [("/dev/kvm", "kvm"), ("/dev/mshv", "mshv")] + .into_iter() + .find(|(device, _)| Path::new(device).exists()) + .map(|(_, name)| name.to_string()) +} + +#[cfg(target_os = "windows")] +fn hypervisor() -> Option { + Some("whp".to_string()) +} + +#[cfg(target_os = "macos")] +fn hypervisor() -> Option { + Some("hvf".to_string()) +} + +#[cfg(not(any(target_os = "linux", target_os = "windows", target_os = "macos")))] +fn hypervisor() -> Option { + None +} + /// Record `benchmarks` as the contents of the run about to start. pub(crate) fn write(benchmarks: impl IntoIterator) -> Result<()> { let (cpu_vendor, cpu_model) = cpu_vendor_and_model(); @@ -93,6 +121,7 @@ pub(crate) fn write(benchmarks: impl IntoIterator) -> Result<()> logical_cpus: std::thread::available_parallelism().ok().map(Into::into), cpu_vendor, cpu_model, + hypervisor: hypervisor(), }, benchmarks, }; @@ -104,15 +133,14 @@ pub(crate) fn write(benchmarks: impl IntoIterator) -> Result<()> fs::write(&path, json).with_context(|| format!("Failed to write {}", path.display())) } -/// The benchmarks a run recorded, or `None` when it left no manifest. -pub(crate) fn read(dir: &Path) -> Result>> { +/// What a run recorded, or `None` when it left no manifest. +pub(crate) fn read(dir: &Path) -> Result> { let path = dir.join(FILE_NAME); let Ok(text) = fs::read_to_string(&path) else { return Ok(None); }; - let manifest: Manifest = serde_json::from_str(&text) - .with_context(|| format!("Failed to parse {}", path.display()))?; - - Ok(Some(manifest.benchmarks)) + serde_json::from_str(&text) + .map(Some) + .with_context(|| format!("Failed to parse {}", path.display())) } diff --git a/src/hyperlight_ci/src/remote.rs b/src/hyperlight_ci/src/remote.rs new file mode 100644 index 0000000000..272cd0e8bf --- /dev/null +++ b/src/hyperlight_ci/src/remote.rs @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2025 The Hyperlight Authors. +//! Benchmark results taken from a CI run rather than this machine. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +use anyhow::{Context, Result, bail}; +use serde::Deserialize; + +/// Artifacts holding a criterion directory are named after the configuration +/// that produced them, `benchmarks_Linux_kvm_amd` and so on. +const ARTIFACT_PREFIX: &str = "benchmarks_"; + +/// How far back to look for a run that still has its benchmark artifacts. +/// They outlive the workflow by days, but not forever. +const RUNS_SEARCHED: usize = 15; + +/// One configuration's results, and where they were unpacked. +pub(crate) struct Results { + /// The configuration that produced them, `Linux_kvm_amd` and so on. + pub label: String, + pub dir: PathBuf, +} + +#[derive(Deserialize)] +struct Artifact { + name: String, + expired: bool, +} + +#[derive(Deserialize)] +struct ArtifactList { + artifacts: Vec, +} + +#[derive(Deserialize)] +struct Run { + #[serde(rename = "databaseId")] + id: u64, +} + +/// Run `gh` and hand back its stdout. +fn gh(args: &[&str]) -> Result> { + let output = Command::new("gh") + .args(args) + .output() + .context("Failed to run gh. The GitHub CLI provides the run artifacts")?; + + if !output.status.success() { + bail!( + "gh {} failed: {}", + args.join(" "), + String::from_utf8_lossy(&output.stderr).trim() + ); + } + + Ok(output.stdout) +} + +/// Names of the benchmark artifacts a run still holds. +fn artifacts(repo: &str, run: u64) -> Result> { + let path = format!("repos/{repo}/actions/runs/{run}/artifacts"); + let list: ArtifactList = serde_json::from_slice(&gh(&["api", &path])?) + .with_context(|| format!("Failed to read the artifacts of run {run}"))?; + + let mut names: Vec = list + .artifacts + .into_iter() + .filter(|a| !a.expired && a.name.starts_with(ARTIFACT_PREFIX)) + .map(|a| a.name) + .collect(); + names.sort(); + names.dedup(); + Ok(names) +} + +/// The most recent run of `pull_request` that still has benchmark artifacts. +/// +/// The newest run is not always the one to report: a run can be cancelled by +/// the next push, or be recent enough that the benchmarks have not finished. +pub(crate) fn latest_run_for(repo: &str, pull_request: u64) -> Result { + let pr = pull_request.to_string(); + let branch = gh(&[ + "pr", + "view", + &pr, + "--repo", + repo, + "--json", + "headRefName", + "--jq", + ".headRefName", + ]) + .with_context(|| format!("Failed to find pull request {pull_request}"))?; + let branch = String::from_utf8_lossy(&branch).trim().to_string(); + + let limit = RUNS_SEARCHED.to_string(); + let runs: Vec = serde_json::from_slice(&gh(&[ + "run", + "list", + "--repo", + repo, + "--branch", + &branch, + "--limit", + &limit, + "--json", + "databaseId", + ])?) + .context("Failed to list the workflow runs of the branch")?; + + for run in &runs { + if !artifacts(repo, run.id)?.is_empty() { + return Ok(run.id); + } + } + + bail!("No run of {branch} still has benchmark artifacts") +} + +/// Fetch every configuration's results from `run`, reusing what is already on +/// disk. Artifacts are immutable, so a run downloads once. +pub(crate) fn fetch(repo: &str, run: u64, cache: &Path) -> Result> { + let names = artifacts(repo, run)?; + if names.is_empty() { + bail!("Run {run} has no benchmark artifacts. They may have expired"); + } + + let run_dir = cache.join(run.to_string()); + let mut results = Vec::new(); + + for name in names { + let label = name[ARTIFACT_PREFIX.len()..].to_string(); + let dir = run_dir.join(&label); + + if !dir.join("benchmarks.json").exists() { + std::fs::create_dir_all(&dir) + .with_context(|| format!("Failed to create {}", dir.display()))?; + let (id, out) = (run.to_string(), dir.display().to_string()); + gh(&[ + "run", "download", &id, "--repo", repo, "-n", &name, "-D", &out, + ]) + .with_context(|| format!("Failed to download {name}"))?; + } + + results.push(Results { label, dir }); + } + + Ok(results) +} From 1dbd4c9c2e51c5dea082b6630bcc0ce32231431c Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Fri, 25 Sep 2026 11:20:10 +0100 Subject: [PATCH 07/12] Compare a pull request with where it branched The default branch is benchmarked daily rather than per commit, so the run that measured a given commit rarely exists. `commit:` takes the closest run that carries no changes the commit never had, and `base-of:` the one where a pull request branched. Nothing within a pull request's own results says what they mean, so that is what they are measured against. A cancelled run leaves some configurations unmeasured, so only whole runs serve as a baseline. Sections are named the way the workflow that measured them is, so a report of a run reads like the comment CI posts. Artifacts hold nothing every run is bound to leave behind, so a download that finished says so itself. Reaching for a file the run might not have written re-downloaded results already on disk, onto the ones already there. Signed-off-by: Jorge Prendes --- docs/benchmarking-hyperlight.md | 6 +- src/hyperlight_ci/src/bench_report.rs | 102 ++++++++++++++++++++++--- src/hyperlight_ci/src/remote.rs | 105 +++++++++++++++++++++++++- 3 files changed, 196 insertions(+), 17 deletions(-) diff --git a/docs/benchmarking-hyperlight.md b/docs/benchmarking-hyperlight.md index 246bc905c2..79513c70c2 100644 --- a/docs/benchmarking-hyperlight.md +++ b/docs/benchmarking-hyperlight.md @@ -74,13 +74,15 @@ Found 1 outliers among 100 measurements (1.00%) Use `just bench` to run benchmarks with release builds (the only supported configuration). Comparing local benchmark results to GitHub-saved benchmarks doesn't make much sense, since you'd be using different hardware, but you can use `just bench-download os hypervisor cpu_vendor [tag] [dest]` to download and extract the GitHub release benchmarks. Pass a `dest` outside `target/criterion` and point `cargo ci bench-report --baseline-root` at it, since `just bench-ci main` overwrites the `main` baseline it runs against. Note that `main` is the name of the baselines stored in GitHub. -`cargo ci bench-report` renders the comparison. `--candidate` and `--baseline` say where each side comes from: a criterion directory, `run:` for a CI run, or `pr:` for the latest run of a pull request. Criterion keeps the last run of a directory in `new` and the one before it in `base`, so a directory on its own reports the last run against the previous one. +`cargo ci bench-report` renders the comparison. `--candidate` and `--baseline` say where each side comes from: a criterion directory, `run:` for a CI run, `pr:` for the latest run of a pull request, `commit:` for the benchmarks of the default branch taken at or before a commit, or `base-of:` for the ones taken where a pull request branched. Criterion keeps the last run of a directory in `new` and the one before it in `base`, so a directory on its own reports the last run against the previous one. -CI results cover every hypervisor and cpu vendor, so results are paired with the ones measured on the same kind of machine. A run records its own operating system, cpu vendor and hypervisor, and CI artifacts are named after the configuration that produced them. Results that fit no counterpart, or several, are reported without a comparison. +The default branch is benchmarked daily rather than per commit, so `commit:` and `base-of:` take the closest run that does not carry changes the commit never had. A pull request defaults to the branch point it was built from, since nothing within its own results says what they mean. CI results cover every hypervisor and cpu vendor, so results are paired with the ones measured on the same kind of machine. A run records its own operating system, cpu vendor and hypervisor, and CI artifacts are named after the configuration that produced them. Results that fit no counterpart, or several, are reported without a comparison. ```sh # a local run against results downloaded into their own directory cargo ci bench-report --baseline target/criterion-baseline +# a pull request against the branch point it was built from +cargo ci bench-report --candidate pr:1529 # this machine against the configuration in CI that matches it cargo ci bench-report --baseline pr:1529 ``` diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs index 465a133028..f253fcc3ea 100644 --- a/src/hyperlight_ci/src/bench_report.rs +++ b/src/hyperlight_ci/src/bench_report.rs @@ -23,6 +23,10 @@ pub enum Source { Dir(PathBuf), Run(u64), PullRequest(u64), + /// Benchmarks of the default branch taken at or before a commit. + Commit(String), + /// Benchmarks of the default branch taken where a pull request branched. + BaseOf(u64), } impl FromStr for Source { @@ -30,15 +34,22 @@ impl FromStr for Source { fn from_str(value: &str) -> Result { // Anything else is a path, so windows drive letters stay paths. - let Some((kind @ ("run" | "pr"), id)) = value.split_once(':') else { + let Some((kind @ ("run" | "pr" | "commit" | "base-of"), rest)) = value.split_once(':') + else { return Ok(Self::Dir(value.into())); }; - let id = id + + if kind == "commit" { + return Ok(Self::Commit(rest.to_string())); + } + + let id = rest .parse() - .map_err(|_| format!("`{id}` is not a {kind} number"))?; + .map_err(|_| format!("`{rest}` is not a {kind} number"))?; Ok(match kind { "run" => Self::Run(id), - _ => Self::PullRequest(id), + "pr" => Self::PullRequest(id), + _ => Self::BaseOf(id), }) } } @@ -118,12 +129,14 @@ pub struct BenchReportArgs { #[arg(long)] pub binary: Vec, - /// Results to report: a criterion directory, `run:` or `pr:` + /// Results to report: a criterion directory, `run:`, `pr:`, + /// `commit:` or `base-of:` #[arg(long, value_name = "SOURCE", default_value = "target/criterion")] pub candidate: Source, - /// Results to compare against: a criterion directory, `run:` or `pr:`. - /// Defaults to the previous run held in the reported directory. + /// 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. #[arg(long, value_name = "SOURCE")] pub baseline: Option, @@ -151,7 +164,15 @@ pub struct BenchReportArgs { /// Entry point for the bench-report subcommand. pub async fn run(args: BenchReportArgs) -> Result<()> { let candidates = resolve(&args.candidate, &args.repo)?; - let mut baselines = match &args.baseline { + + // 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, + }); + + let mut baselines = match &source { Some(source) => resolve(source, &args.repo)?, None => Vec::new(), }; @@ -159,7 +180,7 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { // The first run of a configuration has nothing to compare against, and CI // carries on with the baseline it could not download. baselines.retain(|baseline| has_results(&baseline.dir)); - if baselines.is_empty() && args.baseline.is_some() { + if baselines.is_empty() && source.is_some() { eprintln!("No baseline results found, reporting without a comparison"); } @@ -191,6 +212,12 @@ fn resolve(source: &Source, repo: &str) -> Result> { } Source::Run(run) => *run, Source::PullRequest(pull_request) => remote::latest_run_for(repo, *pull_request)?, + Source::Commit(commit) => remote::run_at(repo, commit)?, + Source::BaseOf(pull_request) => { + let commit = remote::merge_base_of(repo, *pull_request)?; + eprintln!("Pull request {pull_request} branched at {}", &commit[..12]); + remote::run_at(repo, &commit)? + } }; eprintln!("Fetching run {run} of {repo}"); @@ -230,7 +257,10 @@ fn baseline_for<'a>(baselines: &'a [Input], candidate: &Input) -> Option<&'a Pat [only] if only.host.is_none() || candidate.host.is_none() => Some(&only.dir), _ => { let host = candidate.host.as_ref()?; - let what = candidate.label.as_deref().unwrap_or("these results"); + let what = candidate + .label + .as_deref() + .map_or_else(|| "these results".to_string(), describe); let mut found = baselines.iter().filter(|baseline| { baseline .host @@ -257,9 +287,23 @@ fn baseline_for<'a>(baselines: &'a [Input], candidate: &Input) -> Option<&'a Pat /// Name the report after the configuration it covers. fn title(summary: Option<&str>, label: Option<&str>) -> Option { - match (summary, label) { + match (summary, label.map(describe)) { (Some(summary), Some(label)) => Some(format!("{summary} {label}")), - (summary, label) => summary.or(label).map(str::to_string), + (Some(summary), None) => Some(summary.to_string()), + (None, label) => label, + } +} + +/// Name a configuration the way the workflow that measured it does, turning +/// `Linux_kvm_amd` into `kvm / amd (Linux)`. +fn describe(label: &str) -> String { + let named = label + .split_once('_') + .and_then(|(os, rest)| Some((os, rest.rsplit_once('_')?))); + + match named { + Some((os, (hypervisor, vendor))) => format!("{hypervisor} / {vendor} ({os})"), + None => label.to_string(), } } @@ -327,3 +371,37 @@ async fn discover_benchmarks(args: &BenchReportArgs, dir: &Path) -> Result Result { "--limit", &limit, "--json", - "databaseId", + "databaseId,headSha", ])?) .context("Failed to list the workflow runs of the branch")?; @@ -119,6 +130,86 @@ pub(crate) fn latest_run_for(repo: &str, pull_request: u64) -> Result { bail!("No run of {branch} still has benchmark artifacts") } +/// Resolve a sha, tag or branch to the commit it names. +fn commit_sha(repo: &str, commit: &str) -> Result { + let path = format!("repos/{repo}/commits/{commit}"); + let sha = gh(&["api", &path, "--jq", ".sha"]) + .with_context(|| format!("Failed to find commit {commit}"))?; + Ok(String::from_utf8_lossy(&sha).trim().to_string()) +} + +/// Whether `commit` is `ancestor` or was built on top of it. +fn descends_from(repo: &str, commit: &str, ancestor: &str) -> Result { + let path = format!("repos/{repo}/compare/{ancestor}...{commit}"); + let status = gh(&["api", &path, "--jq", ".status"])?; + Ok(matches!( + String::from_utf8_lossy(&status).trim(), + "identical" | "ahead" + )) +} + +/// The most recent benchmarks of the default branch taken at or before +/// `commit`. +/// +/// The branch is benchmarked daily rather than per commit, so the run that +/// measured `commit` itself rarely exists. Anything measured after it carries +/// changes the commit never had. +pub(crate) fn run_at(repo: &str, commit: &str) -> Result { + let commit = commit_sha(repo, commit)?; + let limit = RUNS_SEARCHED.to_string(); + let runs: Vec = serde_json::from_slice(&gh(&[ + "run", + "list", + "--repo", + repo, + "--workflow", + BASELINE_WORKFLOW, + // A cancelled run leaves some configurations unmeasured. + "--status", + "success", + "--limit", + &limit, + "--json", + "databaseId,headSha", + ])?) + .context("Failed to list the benchmark runs of the default branch")?; + + for run in &runs { + if descends_from(repo, &commit, &run.head_sha)? && !artifacts(repo, run.id)?.is_empty() { + return Ok(run.id); + } + } + + bail!("No benchmarks taken at or before {commit} still have their artifacts") +} + +/// Where `pull_request` branched off the branch it targets. +pub(crate) fn merge_base_of(repo: &str, pull_request: u64) -> Result { + let pr = pull_request.to_string(); + let refs = gh(&[ + "pr", + "view", + &pr, + "--repo", + repo, + "--json", + "baseRefName,headRefOid", + "--jq", + ".baseRefName + \" \" + .headRefOid", + ]) + .with_context(|| format!("Failed to find pull request {pull_request}"))?; + + let refs = String::from_utf8_lossy(&refs); + let Some((base, head)) = refs.trim().split_once(' ') else { + bail!("Pull request {pull_request} has no branch to compare against"); + }; + + let path = format!("repos/{repo}/compare/{base}...{head}"); + let sha = gh(&["api", &path, "--jq", ".merge_base_commit.sha"]) + .with_context(|| format!("Failed to find where pull request {pull_request} branched"))?; + Ok(String::from_utf8_lossy(&sha).trim().to_string()) +} + /// Fetch every configuration's results from `run`, reusing what is already on /// disk. Artifacts are immutable, so a run downloads once. pub(crate) fn fetch(repo: &str, run: u64, cache: &Path) -> Result> { @@ -134,14 +225,22 @@ pub(crate) fn fetch(repo: &str, run: u64, cache: &Path) -> Result> let label = name[ARTIFACT_PREFIX.len()..].to_string(); let dir = run_dir.join(&label); - if !dir.join("benchmarks.json").exists() { - std::fs::create_dir_all(&dir) + if !dir.join(DOWNLOADED).exists() { + if dir.exists() { + fs::remove_dir_all(&dir) + .with_context(|| format!("Failed to clear {}", dir.display()))?; + } + fs::create_dir_all(&dir) .with_context(|| format!("Failed to create {}", dir.display()))?; + let (id, out) = (run.to_string(), dir.display().to_string()); gh(&[ "run", "download", &id, "--repo", repo, "-n", &name, "-D", &out, ]) .with_context(|| format!("Failed to download {name}"))?; + + fs::write(dir.join(DOWNLOADED), []) + .with_context(|| format!("Failed to mark {} downloaded", dir.display()))?; } results.push(Results { label, dir }); From ca78beea486cd0bc077103b479eebfbaf6c70429 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Fri, 25 Sep 2026 13:13:33 +0100 Subject: [PATCH 08/12] Report every configuration of a run at once Each benchmark job rendered its own section against a baseline it downloaded itself, which only the pull request comment ever read. The daily and release runs rendered sections nothing consumes, and a pull request was measured against the latest release rather than the branch it targets. The job that posts the comment now reports the whole run against where the pull request branched, so benchmarking a configuration is only that. What the results are worth on their own outlives the comparison, so a baseline out of reach costs the changes, not the report. Signed-off-by: Jorge Prendes --- .github/workflows/DailyBenchmarks.yml | 26 ++-------- .github/workflows/ValidatePullRequest.yml | 31 +++++++----- .github/workflows/dep_benchmarks.yml | 60 ++--------------------- docs/benchmarking-hyperlight.md | 7 ++- src/hyperlight_ci/src/bench_report.rs | 13 +++-- 5 files changed, 40 insertions(+), 97 deletions(-) diff --git a/.github/workflows/DailyBenchmarks.yml b/.github/workflows/DailyBenchmarks.yml index 714fe493c0..c01084adbe 100644 --- a/.github/workflows/DailyBenchmarks.yml +++ b/.github/workflows/DailyBenchmarks.yml @@ -12,24 +12,6 @@ permissions: actions: read jobs: - # Find the most recent successful run of this workflow so we can download - # its benchmark artifacts as a baseline for day-over-day comparison. - find-baseline: - runs-on: ubuntu-latest - outputs: - run-id: ${{ steps.find-run.outputs.run_id }} - steps: - - name: Find latest successful run - id: find-run - # gh run list returns runs sorted by creation date descending (implicit). - # On the first-ever run, this outputs empty and dep_benchmarks.yml - # will skip the baseline download (continue-on-error). - run: | - run_id=$(gh run list --repo "${{ github.repository }}" --workflow DailyBenchmarks.yml --status success --limit 1 --json databaseId --jq '.[0].databaseId // empty') - echo "run_id=$run_id" >> "$GITHUB_OUTPUT" - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - # Build release guest binaries needed by the benchmark suite. build-guests: uses: ./.github/workflows/dep_build_guests.yml @@ -38,10 +20,11 @@ jobs: arch: X64 config: release - # Run benchmarks across all hypervisor/cpu combos, comparing against - # the previous day's results. Artifacts are retained for 90 days. + # Run benchmarks across all hypervisor/cpu combos. Artifacts are retained + # for 90 days, long enough to serve as a baseline for the pull requests that + # branch from the commits they measure. benchmarks: - needs: [build-guests, find-baseline] + needs: [build-guests] strategy: fail-fast: true matrix: @@ -54,7 +37,6 @@ jobs: hypervisor: ${{ matrix.hypervisor }} cpu_vendor: ${{ matrix.cpu_vendor }} arch: ${{ matrix.arch }} - baseline_run_id: ${{ needs.find-baseline.outputs.run-id }} retention_days: 90 # File a GitHub issue if any job fails. diff --git a/.github/workflows/ValidatePullRequest.yml b/.github/workflows/ValidatePullRequest.yml index 41c6f77a04..a310761125 100644 --- a/.github/workflows/ValidatePullRequest.yml +++ b/.github/workflows/ValidatePullRequest.yml @@ -249,8 +249,9 @@ jobs: cpu_vendor: ${{ matrix.cpu_vendor }} arch: ${{ matrix.arch }} - # Combine benchmark reports into a single artifact 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 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. benchmark-comment: name: post-benchmark-comment needs: @@ -258,22 +259,28 @@ jobs: - benchmarks if: ${{ !cancelled() && !failure() && needs.docs-pr.outputs.docs-only == 'false' && github.event_name == 'pull_request' }} runs-on: ubuntu-latest + permissions: + contents: read + actions: read + pull-requests: read steps: - - name: Download benchmark reports - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - uses: hyperlight-dev/ci-setup-workflow@2f4142ba17cf573af44fc1e1f1ffc743daded5b3 # v1.10.0 with: - pattern: benchmark-report_* - path: reports/ + rust-toolchain: "1.94" + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - - name: Combine benchmark reports + - name: Create benchmarks report run: | echo '## Benchmark Results' > pr-comment.md echo '' >> pr-comment.md - for f in reports/benchmark-report_*/benchmark.md; do - [ -f "$f" ] || continue - cat "$f" >> pr-comment.md - echo '' >> pr-comment.md - done + cargo ci bench-report --config-file bench_report.toml \ + --candidate run:${{ github.run_id }} \ + --baseline base-of:${{ github.event.pull_request.number }} >> pr-comment.md + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - name: Upload PR comment artifact uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 diff --git a/.github/workflows/dep_benchmarks.yml b/.github/workflows/dep_benchmarks.yml index 0960c874a1..52764ac69c 100644 --- a/.github/workflows/dep_benchmarks.yml +++ b/.github/workflows/dep_benchmarks.yml @@ -2,26 +2,11 @@ # Reusable workflow to run benchmarks on a single hypervisor/cpu_vendor combination. # -# Baseline comparison: -# The workflow supports two mutually exclusive ways to load a baseline for -# Criterion to compare against: -# -# 1. baseline_run_id — Downloads benchmark artifacts from a previous workflow -# run (by run ID). Used by DailyBenchmarks.yml for day-over-day comparison. -# -# 2. baseline_tag — Downloads benchmark tarballs from a GitHub Release (by tag). -# If empty (the default), `gh release download` fetches from the latest -# stable release. Used by CreateRelease.yml. -# -# If baseline_run_id is set, baseline_tag is ignored. -# If neither is set, the latest stable release is used. -# Both downloads use continue-on-error so the first-ever run (no baseline -# available) succeeds without comparison. -# # Artifact upload: -# Benchmark results are always uploaded as workflow artifacts named +# Benchmark results are uploaded as workflow artifacts named # benchmarks___. The retention_days input controls -# how long they are kept (default: 5 days). +# how long they are kept (default: 5 days). Reading them is left to whoever +# wants a report, so that a comparison covers every configuration at once. name: Run Benchmarks @@ -45,16 +30,6 @@ on: description: CPU architecture for the build, X64 or arm64 (passed from caller matrix) required: true type: string - baseline_tag: - description: Release tag to download baseline benchmarks from (e.g. dev-latest). Ignored if baseline_run_id is set. If empty, downloads from the latest stable release. - required: false - type: string - default: "" - baseline_run_id: - description: Workflow run ID to download baseline benchmark artifacts from. Takes precedence over baseline_tag. - required: false - type: string - default: "" retention_days: description: Number of days to retain benchmark artifacts required: false @@ -122,38 +97,9 @@ jobs: - name: Build run: just build release - - name: Download baseline from previous run - if: ${{ inputs.baseline_run_id != '' }} - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 - with: - name: benchmarks_${{ runner.os }}_${{ inputs.hypervisor }}_${{ inputs.cpu_vendor }} - path: ./target/criterion-baseline/ - run-id: ${{ inputs.baseline_run_id }} - github-token: ${{ secrets.GITHUB_TOKEN }} - continue-on-error: true - - - name: Download baseline from release - if: ${{ inputs.baseline_run_id == '' }} - run: just bench-download ${{ runner.os }} ${{ inputs.hypervisor }} ${{ inputs.cpu_vendor }} ${{ inputs.baseline_tag }} target/criterion-baseline - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - continue-on-error: true - - name: Run benchmarks run: just bench-ci main - # The run overwrites its own `main` baseline, so changes are measured - # against the previous results kept aside. - - name: Create benchmarks report - run: cargo ci bench-report --config-file bench_report.toml --baseline target/criterion-baseline --collapsible '${{ inputs.hypervisor }} / ${{ inputs.cpu_vendor }} (${{ runner.os }})' > target/criterion/benchmark.md - - - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 - with: - name: benchmark-report_${{ runner.os }}_${{ inputs.hypervisor }}_${{ inputs.cpu_vendor }} - path: target/criterion/benchmark.md - if-no-files-found: error - retention-days: 1 - - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: benchmarks_${{ runner.os }}_${{ inputs.hypervisor }}_${{ inputs.cpu_vendor }} diff --git a/docs/benchmarking-hyperlight.md b/docs/benchmarking-hyperlight.md index 79513c70c2..1d1d1d67a1 100644 --- a/docs/benchmarking-hyperlight.md +++ b/docs/benchmarking-hyperlight.md @@ -14,7 +14,10 @@ Hyperlight uses the [Criterion](https://bheisler.github.io/criterion.rs/book/ind Change within noise threshold.* ``` -2. For each release +2. For each pull request + - Benchmarks run on every hypervisor and cpu vendor in `ValidatePullRequest.yml`, which invokes `dep_benchmarks.yml`. The results are reported against the daily benchmarks of the commit the pull request branched from, and posted as a comment. + +3. For each release - For each release, benchmarks are run as part of the release pipeline in `CreateRelease.yml`, which invokes `dep_benchmarks.yml`. These benchmark results are compared to the previous release, and are uploaded as part of the "Release assets" on the GitHub release page. Currently, benchmarks are run on windows, linux-kvm (ubuntu), and linux-hyperv (mariner). Only release builds are benchmarked, not debug. @@ -72,7 +75,7 @@ Found 1 outliers among 100 measurements (1.00%) ## Running benchmarks locally -Use `just bench` to run benchmarks with release builds (the only supported configuration). Comparing local benchmark results to GitHub-saved benchmarks doesn't make much sense, since you'd be using different hardware, but you can use `just bench-download os hypervisor cpu_vendor [tag] [dest]` to download and extract the GitHub release benchmarks. Pass a `dest` outside `target/criterion` and point `cargo ci bench-report --baseline-root` at it, since `just bench-ci main` overwrites the `main` baseline it runs against. Note that `main` is the name of the baselines stored in GitHub. +Use `just bench` to run benchmarks with release builds (the only supported configuration). Comparing local benchmark results to GitHub-saved benchmarks doesn't make much sense, since you'd be using different hardware, but you can use `just bench-download os hypervisor cpu_vendor [tag] [dest]` to download and extract the GitHub release benchmarks. Pass a `dest` outside `target/criterion` and point `cargo ci bench-report --baseline` at it, since `just bench-ci main` overwrites the `main` baseline it runs against. Note that `main` is the name of the baselines stored in GitHub. `cargo ci bench-report` renders the comparison. `--candidate` and `--baseline` say where each side comes from: a criterion directory, `run:` for a CI run, `pr:` for the latest run of a pull request, `commit:` for the benchmarks of the default branch taken at or before a commit, or `base-of:` for the ones taken where a pull request branched. Criterion keeps the last run of a directory in `new` and the one before it in `base`, so a directory on its own reports the last run against the previous one. diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs index f253fcc3ea..042f729272 100644 --- a/src/hyperlight_ci/src/bench_report.rs +++ b/src/hyperlight_ci/src/bench_report.rs @@ -172,10 +172,15 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { _ => None, }); - let mut baselines = match &source { - Some(source) => resolve(source, &args.repo)?, - None => Vec::new(), - }; + let mut baselines = Vec::new(); + if let Some(source) = &source { + match resolve(source, &args.repo) { + Ok(found) => baselines = found, + // What the results are worth on their own outlives the comparison, + // so a baseline out of reach costs the changes, not the report. + Err(error) => eprintln!("{error:#}"), + } + } // The first run of a configuration has nothing to compare against, and CI // carries on with the baseline it could not download. From 1cc9ab4d90575b342c6d809d8543b8498215f05b Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Fri, 25 Sep 2026 13:27:27 +0100 Subject: [PATCH 09/12] Report the benchmarks a release carries Workflow artifacts were swept away after five days, so comparing against anything older meant unpacking a release tarball by hand into a directory the report could be pointed at. They now last as long as GitHub keeps them, and `release:` reads what a release carries, which outlives artifacts altogether. Searching as far back as results are kept lets a commit that old still be found. Benchmark ids change over time, so a release far enough back has little left to compare against. What matches is reported and the rest stands alone. Signed-off-by: Jorge Prendes --- .github/workflows/DailyBenchmarks.yml | 7 +- .github/workflows/dep_benchmarks.yml | 16 ++--- Justfile | 12 ---- docs/benchmarking-hyperlight.md | 12 ++-- src/hyperlight_ci/src/bench_report.rs | 38 +++++++---- src/hyperlight_ci/src/remote.rs | 94 ++++++++++++++++++++++++++- 6 files changed, 135 insertions(+), 44 deletions(-) diff --git a/.github/workflows/DailyBenchmarks.yml b/.github/workflows/DailyBenchmarks.yml index c01084adbe..abf63cfd1b 100644 --- a/.github/workflows/DailyBenchmarks.yml +++ b/.github/workflows/DailyBenchmarks.yml @@ -20,9 +20,9 @@ jobs: arch: X64 config: release - # Run benchmarks across all hypervisor/cpu combos. Artifacts are retained - # for 90 days, long enough to serve as a baseline for the pull requests that - # branch from the commits they measure. + # Run benchmarks across all hypervisor/cpu combos. The artifacts they leave + # are the baseline for the pull requests that branch from the commits they + # measure. benchmarks: needs: [build-guests] strategy: @@ -37,7 +37,6 @@ jobs: hypervisor: ${{ matrix.hypervisor }} cpu_vendor: ${{ matrix.cpu_vendor }} arch: ${{ matrix.arch }} - retention_days: 90 # File a GitHub issue if any job fails. notify-failure: diff --git a/.github/workflows/dep_benchmarks.yml b/.github/workflows/dep_benchmarks.yml index 52764ac69c..5e54ecbb35 100644 --- a/.github/workflows/dep_benchmarks.yml +++ b/.github/workflows/dep_benchmarks.yml @@ -4,9 +4,10 @@ # # Artifact upload: # Benchmark results are uploaded as workflow artifacts named -# benchmarks___. The retention_days input controls -# how long they are kept (default: 5 days). Reading them is left to whoever -# wants a report, so that a comparison covers every configuration at once. +# benchmarks___, kept for as long as GitHub +# allows so that a later run can still be compared against them. Reading +# them is left to whoever wants a report, so that a comparison covers every +# configuration at once. name: Run Benchmarks @@ -30,11 +31,6 @@ on: description: CPU architecture for the build, X64 or arm64 (passed from caller matrix) required: true type: string - retention_days: - description: Number of days to retain benchmark artifacts - required: false - type: number - default: 5 env: CARGO_TERM_COLOR: always RUST_BACKTRACE: full @@ -105,4 +101,6 @@ jobs: name: benchmarks_${{ runner.os }}_${{ inputs.hypervisor }}_${{ inputs.cpu_vendor }} path: ./target/criterion/ if-no-files-found: error - retention-days: ${{ inputs.retention_days }} + # The longest GitHub keeps an artifact, so that a pull request can be + # compared against the branch point it was built from. + retention-days: 90 diff --git a/Justfile b/Justfile index 7277c1abe7..5859933302 100644 --- a/Justfile +++ b/Justfile @@ -423,18 +423,6 @@ tar-static-lib: (build-rust-capi "release") (build-rust-capi "debug") ### BENCHMARKING ### #################### -# Warning: can overwrite previous local benchmarks, so run this before running benchmarks -# Downloads the benchmarks result from the given release tag. -# If tag is not given, defaults to latest release -# Options for os: "Windows", or "Linux" -# Options for Linux hypervisor: "kvm", "mshv3" -# Options for Windows hypervisor: "hyperv", "hyperv-ws2025" -# Options for cpu_vendor: "amd", "intel" -bench-download os hypervisor cpu_vendor tag="" dest="target/criterion": - gh release download {{ tag }} -D ./target/ -p benchmarks_{{ os }}_{{ hypervisor }}_{{ cpu_vendor }}.tar.gz - mkdir -p {{ dest }} {{ if os() == "windows" { "-Force" } else { "" } }} - tar -zxvf target/benchmarks_{{ os }}_{{ hypervisor }}_{{ cpu_vendor }}.tar.gz -C {{ dest }}/ --strip-components=1 - # Warning: compares to and then OVERWRITES the given baseline bench-ci baseline features="": cargo ci bench {{ if features == "" {''} else { "--features " + features } }} --verbose --save-baseline {{ baseline }} diff --git a/docs/benchmarking-hyperlight.md b/docs/benchmarking-hyperlight.md index 1d1d1d67a1..dfa8c3012e 100644 --- a/docs/benchmarking-hyperlight.md +++ b/docs/benchmarking-hyperlight.md @@ -5,7 +5,7 @@ Hyperlight uses the [Criterion](https://bheisler.github.io/criterion.rs/book/ind ## When Benchmarks are run 1. Daily (scheduled) - - Benchmarks run daily via `DailyBenchmarks.yml`, comparing results against the previous day's run. Results are stored as workflow artifacts with 90-day retention. + - Benchmarks run daily via `DailyBenchmarks.yml`. Results are stored as workflow artifacts with 90-day retention, and are what pull requests are compared against. ``` sandboxes/create_sandbox @@ -75,19 +75,19 @@ Found 1 outliers among 100 measurements (1.00%) ## Running benchmarks locally -Use `just bench` to run benchmarks with release builds (the only supported configuration). Comparing local benchmark results to GitHub-saved benchmarks doesn't make much sense, since you'd be using different hardware, but you can use `just bench-download os hypervisor cpu_vendor [tag] [dest]` to download and extract the GitHub release benchmarks. Pass a `dest` outside `target/criterion` and point `cargo ci bench-report --baseline` at it, since `just bench-ci main` overwrites the `main` baseline it runs against. Note that `main` is the name of the baselines stored in GitHub. +Use `just bench` to run benchmarks with release builds (the only supported configuration). Comparing local benchmark results to the ones CI measures doesn't say much, since you'd be using different hardware, but `cargo ci bench-report` fetches them for you. -`cargo ci bench-report` renders the comparison. `--candidate` and `--baseline` say where each side comes from: a criterion directory, `run:` for a CI run, `pr:` for the latest run of a pull request, `commit:` for the benchmarks of the default branch taken at or before a commit, or `base-of:` for the ones taken where a pull request branched. Criterion keeps the last run of a directory in `new` and the one before it in `base`, so a directory on its own reports the last run against the previous one. +`cargo ci bench-report` renders the comparison. `--candidate` and `--baseline` say where each side comes from: a criterion directory, `run:` for a CI run, `pr:` for the latest run of a pull request, `commit:` for the benchmarks of the default branch taken at or before a commit, `base-of:` for the ones taken where a pull request branched, or `release:` for the ones a release carries. Criterion keeps the last run of a directory in `new` and the one before it in `base`, so a directory on its own reports the last run against the previous one. -The default branch is benchmarked daily rather than per commit, so `commit:` and `base-of:` take the closest run that does not carry changes the commit never had. A pull request defaults to the branch point it was built from, since nothing within its own results says what they mean. CI results cover every hypervisor and cpu vendor, so results are paired with the ones measured on the same kind of machine. A run records its own operating system, cpu vendor and hypervisor, and CI artifacts are named after the configuration that produced them. Results that fit no counterpart, or several, are reported without a comparison. +The default branch is benchmarked daily rather than per commit, so `commit:` and `base-of:` take the closest run that does not carry changes the commit never had. A pull request defaults to the branch point it was built from, since nothing within its own results says what they mean. Workflow artifacts are swept away after 90 days, so reaching further back means the results a release carries. CI results cover every hypervisor and cpu vendor, so results are paired with the ones measured on the same kind of machine. A run records its own operating system, cpu vendor and hypervisor, and CI artifacts are named after the configuration that produced them. Results that fit no counterpart, or several, are reported without a comparison. ```sh -# a local run against results downloaded into their own directory -cargo ci bench-report --baseline target/criterion-baseline # a pull request against the branch point it was built from cargo ci bench-report --candidate pr:1529 # this machine against the configuration in CI that matches it cargo ci bench-report --baseline pr:1529 +# this machine against what a release measured +cargo ci bench-report --baseline release:v0.17.0 ``` **Important**: The `just bench` command uses release builds by default to ensure meaningful performance measurements. For profiling purposes, you can compile benchmarks with debug symbols by running `cargo bench` directly. diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs index 042f729272..cba0691226 100644 --- a/src/hyperlight_ci/src/bench_report.rs +++ b/src/hyperlight_ci/src/bench_report.rs @@ -27,6 +27,8 @@ pub enum Source { Commit(String), /// Benchmarks of the default branch taken where a pull request branched. BaseOf(u64), + /// Benchmarks a release carries, which outlive the workflow artifacts. + Release(String), } impl FromStr for Source { @@ -34,13 +36,16 @@ impl FromStr for Source { fn from_str(value: &str) -> Result { // Anything else is a path, so windows drive letters stay paths. - let Some((kind @ ("run" | "pr" | "commit" | "base-of"), rest)) = value.split_once(':') + let Some((kind @ ("run" | "pr" | "commit" | "base-of" | "release"), rest)) = + value.split_once(':') else { return Ok(Self::Dir(value.into())); }; - if kind == "commit" { - return Ok(Self::Commit(rest.to_string())); + match kind { + "commit" => return Ok(Self::Commit(rest.to_string())), + "release" => return Ok(Self::Release(rest.to_string())), + _ => {} } let id = rest @@ -130,7 +135,7 @@ pub struct BenchReportArgs { pub binary: Vec, /// Results to report: a criterion directory, `run:`, `pr:`, - /// `commit:` or `base-of:` + /// `commit:`, `base-of:` or `release:` #[arg(long, value_name = "SOURCE", default_value = "target/criterion")] pub candidate: Source, @@ -207,7 +212,7 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { /// Locate the results `source` points at. fn resolve(source: &Source, repo: &str) -> Result> { - let run = match source { + let results = match source { Source::Dir(dir) => { return Ok(vec![Input { label: None, @@ -215,18 +220,23 @@ fn resolve(source: &Source, repo: &str) -> Result> { dir: dir.clone(), }]); } - Source::Run(run) => *run, - Source::PullRequest(pull_request) => remote::latest_run_for(repo, *pull_request)?, - Source::Commit(commit) => remote::run_at(repo, commit)?, + Source::Run(run) => fetch(repo, *run)?, + Source::PullRequest(pull_request) => { + fetch(repo, remote::latest_run_for(repo, *pull_request)?)? + } + Source::Commit(commit) => fetch(repo, remote::run_at(repo, commit)?)?, Source::BaseOf(pull_request) => { let commit = remote::merge_base_of(repo, *pull_request)?; eprintln!("Pull request {pull_request} branched at {}", &commit[..12]); - remote::run_at(repo, &commit)? + fetch(repo, remote::run_at(repo, &commit)?)? + } + Source::Release(tag) => { + eprintln!("Fetching release {tag} of {repo}"); + remote::fetch_release(repo, tag, Path::new(RUN_CACHE))? } }; - eprintln!("Fetching run {run} of {repo}"); - remote::fetch(repo, run, Path::new(RUN_CACHE))? + results .into_iter() .map(|results| { Ok(Input { @@ -242,6 +252,12 @@ fn resolve(source: &Source, repo: &str) -> Result> { .collect() } +/// Fetch every configuration a run measured. +fn fetch(repo: &str, run: u64) -> Result> { + eprintln!("Fetching run {run} of {repo}"); + remote::fetch(repo, run, Path::new(RUN_CACHE)) +} + /// What the run in `dir` recorded about the machine it ran on. fn host_of(dir: &Path) -> Result> { Ok(manifest::read(dir)?.and_then(|manifest| Identity::from_host(&manifest.host))) diff --git a/src/hyperlight_ci/src/remote.rs b/src/hyperlight_ci/src/remote.rs index f66093a7eb..c9df22b875 100644 --- a/src/hyperlight_ci/src/remote.rs +++ b/src/hyperlight_ci/src/remote.rs @@ -14,13 +14,16 @@ use serde::Deserialize; const ARTIFACT_PREFIX: &str = "benchmarks_"; /// How far back to look for a run that still has its benchmark artifacts. -/// They outlive the workflow by days, but not forever. -const RUNS_SEARCHED: usize = 15; +/// A day of the default branch is one run, so this spans how long they are kept. +const RUNS_SEARCHED: usize = 90; /// Where benchmarks of the default branch come from. Pull requests benchmark /// far more often, but never the branch they merge into. const BASELINE_WORKFLOW: &str = "DailyBenchmarks.yml"; +/// What a release calls the results it carries. +const ARCHIVE_SUFFIX: &str = ".tar.gz"; + /// Marks a download that finished. Artifacts hold nothing every run is bound /// to leave behind, and an interrupted one leaves the directory half written. const DOWNLOADED: &str = ".downloaded"; @@ -248,3 +251,90 @@ pub(crate) fn fetch(repo: &str, run: u64, cache: &Path) -> Result> Ok(results) } + +/// Names of the benchmark archives a release carries. +fn assets(repo: &str, tag: &str) -> Result> { + let names = gh(&[ + "release", + "view", + tag, + "--repo", + repo, + "--json", + "assets", + "--jq", + ".assets[].name", + ]) + .with_context(|| format!("Failed to find release {tag}"))?; + + Ok(String::from_utf8_lossy(&names) + .lines() + .filter(|name| name.starts_with(ARTIFACT_PREFIX) && name.ends_with(ARCHIVE_SUFFIX)) + .map(str::to_string) + .collect()) +} + +/// Fetch every configuration's results from the release tagged `tag`. +/// +/// A release carries what it measured for as long as it exists, which is past +/// the day the workflow artifacts of the same run are swept away. +pub(crate) fn fetch_release(repo: &str, tag: &str, cache: &Path) -> Result> { + let names = assets(repo, tag)?; + if names.is_empty() { + bail!("Release {tag} carries no benchmark results"); + } + + let release_dir = cache.join(format!("release-{tag}")); + let mut results = Vec::new(); + + for name in names { + let label = name[ARTIFACT_PREFIX.len()..name.len() - ARCHIVE_SUFFIX.len()].to_string(); + let dir = release_dir.join(&label); + + if !dir.join(DOWNLOADED).exists() { + if dir.exists() { + fs::remove_dir_all(&dir) + .with_context(|| format!("Failed to clear {}", dir.display()))?; + } + fs::create_dir_all(&dir) + .with_context(|| format!("Failed to create {}", dir.display()))?; + + let out = dir.display().to_string(); + gh(&[ + "release", "download", tag, "--repo", repo, "-p", &name, "-D", &out, + ]) + .with_context(|| format!("Failed to download {name}"))?; + + // The archive holds the criterion directory under a name of its own. + let archive = dir.join(&name); + unpack(&archive, &dir)?; + fs::remove_file(&archive) + .with_context(|| format!("Failed to remove {}", archive.display()))?; + + fs::write(dir.join(DOWNLOADED), []) + .with_context(|| format!("Failed to mark {} downloaded", dir.display()))?; + } + + results.push(Results { label, dir }); + } + + Ok(results) +} + +/// Unpack `archive` into `into`, dropping the directory it wraps everything in. +fn unpack(archive: &Path, into: &Path) -> Result<()> { + let status = Command::new("tar") + .arg("-xzf") + .arg(archive) + .arg("-C") + .arg(into) + .arg("--strip-components=1") + .status() + .context("Failed to run tar. It unpacks the results a release carries")?; + + if !status.success() { + bail!("Failed to unpack {}", archive.display()); + } + + Ok(()) +} From 9d6cedb32f493f453d820ef633799393f9a25080 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Fri, 25 Sep 2026 13:45:45 +0100 Subject: [PATCH 10/12] Say which commits a benchmark report covers A comment that only holds numbers leaves the reader to work out what was measured and what it was held against. The report names both commits, and `--reproduce` ends it with the command that asks for it again. What was asked for moves, since the last run of a pull request is whichever ran most recently and where it branched changes when it is rebased, so the command names the runs that answered instead. GitHub answers run listings out of an index that takes a moment to warm, leaving the most recent runs out of the first replies. Taking one at its word picked a baseline months older than the one asked for, silently, because an old run is still an ancestor of the commit. Listings are now asked for until two agree on the newest run. Signed-off-by: Jorge Prendes --- .github/workflows/ValidatePullRequest.yml | 2 +- src/hyperlight_ci/src/bench_report.rs | 136 +++++++++++++++++----- src/hyperlight_ci/src/remote.rs | 102 ++++++++++------ 3 files changed, 174 insertions(+), 66 deletions(-) diff --git a/.github/workflows/ValidatePullRequest.yml b/.github/workflows/ValidatePullRequest.yml index a310761125..571b5683f1 100644 --- a/.github/workflows/ValidatePullRequest.yml +++ b/.github/workflows/ValidatePullRequest.yml @@ -276,7 +276,7 @@ jobs: run: | echo '## Benchmark Results' > pr-comment.md echo '' >> pr-comment.md - cargo ci bench-report --config-file bench_report.toml \ + 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 env: diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs index cba0691226..a05951bc17 100644 --- a/src/hyperlight_ci/src/bench_report.rs +++ b/src/hyperlight_ci/src/bench_report.rs @@ -66,6 +66,14 @@ struct Input { host: Option, } +/// A set of results and the commit they were taken at. +struct Origin { + commit: Option, + /// How to ask for these same results again, whatever was asked for here. + pinned: String, + inputs: Vec, +} + /// What distinguishes one set of benchmark results from another. #[derive(PartialEq)] struct Identity { @@ -161,6 +169,10 @@ pub struct BenchReportArgs { #[arg(long, value_name = "PATH")] pub config_file: Option, + /// End the report with the command that asks for it again + #[arg(long)] + pub reproduce: bool, + /// Additional arguments to forward to criterion benchmarks (e.g. filter, --exact) #[arg(trailing_var_arg = true, allow_hyphen_values = true)] pub bench_args: Vec, @@ -168,7 +180,7 @@ pub struct BenchReportArgs { /// Entry point for the bench-report subcommand. pub async fn run(args: BenchReportArgs) -> Result<()> { - let candidates = resolve(&args.candidate, &args.repo)?; + let candidate = resolve(&args.candidate, &args.repo)?; // Nothing within a pull request's results says what they mean, so they are // measured against the branch point they were built from. @@ -177,10 +189,14 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { _ => None, }); - let mut baselines = Vec::new(); + let mut baseline = Origin { + commit: None, + pinned: String::new(), + inputs: Vec::new(), + }; if let Some(source) = &source { match resolve(source, &args.repo) { - Ok(found) => baselines = found, + Ok(found) => baseline = found, // What the results are worth on their own outlives the comparison, // so a baseline out of reach costs the changes, not the report. Err(error) => eprintln!("{error:#}"), @@ -189,53 +205,119 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { // The first run of a configuration has nothing to compare against, and CI // carries on with the baseline it could not download. - baselines.retain(|baseline| has_results(&baseline.dir)); - if baselines.is_empty() && source.is_some() { - eprintln!("No baseline results found, reporting without a comparison"); + baseline + .inputs + .retain(|baseline| has_results(&baseline.dir)); + if baseline.inputs.is_empty() { + baseline.commit = None; + if source.is_some() { + eprintln!("No baseline results found, reporting without a comparison"); + } + } + + if let Some(measured) = measured(&args.repo, &candidate, &baseline) { + print!("{measured}"); } // A CI run covers every hypervisor and cpu vendor, one section each. - for candidate in &candidates { + for candidate in &candidate.inputs { let label = candidate.label.as_deref(); let markdown = report( &args, &candidate.dir, - baseline_for(&baselines, candidate), + baseline_for(&baseline.inputs, candidate), title(args.collapsible.as_deref(), label), ) .await?; print!("{markdown}"); } + if args.reproduce { + print!("{}", reproduce(&args, &candidate, &baseline)); + } + Ok(()) } +/// The command that reports these same results again. +/// +/// What was asked for moves: the last run of a pull request is whichever ran +/// most recently, and where it branched changes when it is rebased. Naming the +/// runs that answered holds the report still. +fn reproduce(args: &BenchReportArgs, candidate: &Origin, baseline: &Origin) -> String { + let mut command = format!("cargo ci bench-report --candidate {}", candidate.pinned); + + if !baseline.inputs.is_empty() { + command.push_str(&format!(" --baseline {}", baseline.pinned)); + } + + if let Some(config) = &args.config_file { + command.push_str(&format!(" --config-file {}", config.display())); + } + + format!("\nReported by `{command}`.\n") +} + +/// Say which commits the report covers, so a reader can tell what they are +/// looking at without knowing how it was asked for. +fn measured(repo: &str, candidate: &Origin, baseline: &Origin) -> Option { + let link = |sha: &String| { + let short = sha.get(..12).unwrap_or(sha); + format!("[`{short}`](https://github.com/{repo}/commit/{sha})") + }; + + let mut lines = format!("Measured commit: {}", candidate.commit.as_ref().map(link)?); + if let Some(baseline) = baseline.commit.as_ref().map(link) { + lines.push_str(&format!("\nBaseline commit: {baseline}")); + } + lines.push_str("\n\n"); + + Some(lines) +} + /// Locate the results `source` points at. -fn resolve(source: &Source, repo: &str) -> Result> { - let results = match source { +fn resolve(source: &Source, repo: &str) -> Result { + let run = match source { Source::Dir(dir) => { - return Ok(vec![Input { - label: None, - host: host_of(dir)?, - dir: dir.clone(), - }]); + return Ok(Origin { + commit: None, + pinned: dir.display().to_string(), + inputs: vec![Input { + label: None, + host: host_of(dir)?, + dir: dir.clone(), + }], + }); } - Source::Run(run) => fetch(repo, *run)?, - Source::PullRequest(pull_request) => { - fetch(repo, remote::latest_run_for(repo, *pull_request)?)? + Source::Release(tag) => { + eprintln!("Fetching release {tag} of {repo}"); + return Ok(Origin { + commit: remote::commit_sha(repo, tag).ok(), + pinned: format!("release:{tag}"), + inputs: inputs(remote::fetch_release(repo, tag, Path::new(RUN_CACHE))?)?, + }); } - Source::Commit(commit) => fetch(repo, remote::run_at(repo, commit)?)?, + Source::Run(run) => *run, + Source::PullRequest(pull_request) => remote::latest_run_for(repo, *pull_request)?, + Source::Commit(commit) => remote::run_at(repo, commit)?, Source::BaseOf(pull_request) => { let commit = remote::merge_base_of(repo, *pull_request)?; eprintln!("Pull request {pull_request} branched at {}", &commit[..12]); - fetch(repo, remote::run_at(repo, &commit)?)? - } - Source::Release(tag) => { - eprintln!("Fetching release {tag} of {repo}"); - remote::fetch_release(repo, tag, Path::new(RUN_CACHE))? + remote::run_at(repo, &commit)? } }; + eprintln!("Fetching run {run} of {repo}"); + Ok(Origin { + // A report reads the same without it, so it is not worth failing over. + commit: remote::run_commit(repo, run).ok(), + pinned: format!("run:{run}"), + inputs: inputs(remote::fetch(repo, run, Path::new(RUN_CACHE))?)?, + }) +} + +/// Read what each set of results says about the machine that took them. +fn inputs(results: Vec) -> Result> { results .into_iter() .map(|results| { @@ -252,12 +334,6 @@ fn resolve(source: &Source, repo: &str) -> Result> { .collect() } -/// Fetch every configuration a run measured. -fn fetch(repo: &str, run: u64) -> Result> { - eprintln!("Fetching run {run} of {repo}"); - remote::fetch(repo, run, Path::new(RUN_CACHE)) -} - /// What the run in `dir` recorded about the machine it ran on. fn host_of(dir: &Path) -> Result> { Ok(manifest::read(dir)?.and_then(|manifest| Identity::from_host(&manifest.host))) diff --git a/src/hyperlight_ci/src/remote.rs b/src/hyperlight_ci/src/remote.rs index c9df22b875..8a2460c023 100644 --- a/src/hyperlight_ci/src/remote.rs +++ b/src/hyperlight_ci/src/remote.rs @@ -24,6 +24,9 @@ const BASELINE_WORKFLOW: &str = "DailyBenchmarks.yml"; /// What a release calls the results it carries. const ARCHIVE_SUFFIX: &str = ".tar.gz"; +/// How many times to ask for a listing before taking it at its word. +const LISTINGS: usize = 5; + /// Marks a download that finished. Artifacts hold nothing every run is bound /// to leave behind, and an interrupted one leaves the directory half written. const DOWNLOADED: &str = ".downloaded"; @@ -52,6 +55,8 @@ struct Run { id: u64, #[serde(rename = "headSha")] head_sha: String, + #[serde(rename = "createdAt")] + created_at: String, } /// Run `gh` and hand back its stdout. @@ -89,6 +94,47 @@ fn artifacts(repo: &str, run: u64) -> Result> { Ok(names) } +/// Runs matching `filter`, newest first. +/// +/// GitHub answers out of an index that takes a moment to warm, leaving the +/// most recent runs out of the first replies. Taking one at its word picks a +/// baseline months older than the one asked for, so ask until two replies +/// agree on the newest run and keep everything either of them saw. +fn runs(repo: &str, filter: &[&str]) -> Result> { + let limit = RUNS_SEARCHED.to_string(); + let mut seen: Vec = Vec::new(); + let mut newest = None; + + for _ in 0..LISTINGS { + let mut args = vec![ + "run", + "list", + "--repo", + repo, + "--limit", + &limit, + "--json", + "databaseId,headSha,createdAt", + ]; + args.extend_from_slice(filter); + + let listed: Vec = + serde_json::from_slice(&gh(&args)?).context("Failed to list the workflow runs")?; + + let latest = listed.first().map(|run| run.id); + seen.extend(listed); + + if latest.is_some() && latest == newest { + break; + } + newest = latest; + } + + seen.sort_by(|left, right| (&right.created_at, right.id).cmp(&(&left.created_at, left.id))); + seen.dedup_by_key(|run| run.id); + Ok(seen) +} + /// The most recent run of `pull_request` that still has benchmark artifacts. /// /// The newest run is not always the one to report: a run can be cancelled by @@ -109,22 +155,7 @@ pub(crate) fn latest_run_for(repo: &str, pull_request: u64) -> Result { .with_context(|| format!("Failed to find pull request {pull_request}"))?; let branch = String::from_utf8_lossy(&branch).trim().to_string(); - let limit = RUNS_SEARCHED.to_string(); - let runs: Vec = serde_json::from_slice(&gh(&[ - "run", - "list", - "--repo", - repo, - "--branch", - &branch, - "--limit", - &limit, - "--json", - "databaseId,headSha", - ])?) - .context("Failed to list the workflow runs of the branch")?; - - for run in &runs { + for run in runs(repo, &["--branch", &branch])? { if !artifacts(repo, run.id)?.is_empty() { return Ok(run.id); } @@ -134,13 +165,23 @@ pub(crate) fn latest_run_for(repo: &str, pull_request: u64) -> Result { } /// Resolve a sha, tag or branch to the commit it names. -fn commit_sha(repo: &str, commit: &str) -> Result { +pub(crate) fn commit_sha(repo: &str, commit: &str) -> Result { let path = format!("repos/{repo}/commits/{commit}"); let sha = gh(&["api", &path, "--jq", ".sha"]) .with_context(|| format!("Failed to find commit {commit}"))?; Ok(String::from_utf8_lossy(&sha).trim().to_string()) } +/// The commit a run measured. +pub(crate) fn run_commit(repo: &str, run: u64) -> Result { + let id = run.to_string(); + let sha = gh(&[ + "run", "view", &id, "--repo", repo, "--json", "headSha", "--jq", ".headSha", + ]) + .with_context(|| format!("Failed to find what run {run} measured"))?; + Ok(String::from_utf8_lossy(&sha).trim().to_string()) +} + /// Whether `commit` is `ancestor` or was built on top of it. fn descends_from(repo: &str, commit: &str, ancestor: &str) -> Result { let path = format!("repos/{repo}/compare/{ancestor}...{commit}"); @@ -159,23 +200,11 @@ fn descends_from(repo: &str, commit: &str, ancestor: &str) -> Result { /// changes the commit never had. pub(crate) fn run_at(repo: &str, commit: &str) -> Result { let commit = commit_sha(repo, commit)?; - let limit = RUNS_SEARCHED.to_string(); - let runs: Vec = serde_json::from_slice(&gh(&[ - "run", - "list", - "--repo", + // A cancelled run leaves some configurations unmeasured. + let runs = runs( repo, - "--workflow", - BASELINE_WORKFLOW, - // A cancelled run leaves some configurations unmeasured. - "--status", - "success", - "--limit", - &limit, - "--json", - "databaseId,headSha", - ])?) - .context("Failed to list the benchmark runs of the default branch")?; + &["--workflow", BASELINE_WORKFLOW, "--status", "success"], + )?; for run in &runs { if descends_from(repo, &commit, &run.head_sha)? && !artifacts(repo, run.id)?.is_empty() { @@ -267,11 +296,14 @@ fn assets(repo: &str, tag: &str) -> Result> { ]) .with_context(|| format!("Failed to find release {tag}"))?; - Ok(String::from_utf8_lossy(&names) + let mut names: Vec = String::from_utf8_lossy(&names) .lines() .filter(|name| name.starts_with(ARTIFACT_PREFIX) && name.ends_with(ARCHIVE_SUFFIX)) .map(str::to_string) - .collect()) + .collect(); + names.sort(); + names.dedup(); + Ok(names) } /// Fetch every configuration's results from the release tagged `tag`. From 9bed24c1d2f2a94d23574b81693bc36caac86dd4 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Fri, 25 Sep 2026 22:03:25 +0100 Subject: [PATCH 11/12] Choose the reported benchmarks from eleven CI runs The lists were measured on a quiet machine, which is not the machine that reports them, and then from four runs whose spread was taken as the range between the extremes. Whole runs land slow: two of eleven sat 10% to 20% above the rest across the entire suite, and a range reads that as every benchmark being unstable. The middle half of the runs says what a benchmark usually does. Listed here when no configuration spreads more than 10%. That is a fraction of what any one configuration could carry. `Linux_kvm_amd` holds 87 of 95 benchmarks within 5%, `guest_calls` among them at 0.3%, while `Windows_hyperv-ws2025_amd` holds 15 and stays there on uniform hardware. `Linux_mshv3_amd` is as quiet as kvm on one processor and four times worse across two, because its pool answers with both EPYC generations. Signed-off-by: Jorge Prendes --- bench_report.toml | 99 +++++++++++------------------------------------ 1 file changed, 22 insertions(+), 77 deletions(-) diff --git a/bench_report.toml b/bench_report.toml index 6a0627bfb8..b1fac85356 100644 --- a/bench_report.toml +++ b/bench_report.toml @@ -1,84 +1,29 @@ -# Benchmarks whose results appear in pull request comments. -# -# Entries are regular expressions matched against criterion benchmark ids such -# as `sandboxes/create_initialized/default`. CI runs every benchmark and reports -# only the ones selected here, so noisy benchmarks can be dropped from the -# comment without losing their results in the uploaded criterion artifact. -# -# A benchmark is reported when it matches `allowlist` and no `denylist` entry. -# An empty `allowlist` keeps every benchmark the denylist does not exclude, and -# leaving both lists empty disables filtering. -# -# An allowlist entry matching no benchmark fails the report, so a renamed or -# deleted benchmark surfaces instead of silently vanishing from the comment. A -# denylist entry matching nothing is accepted, because a benchmark may be absent -# on some platforms. -# -# cargo ci bench-report --config-file bench_report.toml -# cargo ci bench --config-file bench_report.toml -# -# The lists below keep the benchmarks whose median drifted by at most 5% across -# five back-to-back local runs on an idle 32 core machine. CI runners are -# smaller and noisier, so treat that as a lower bound on the drift CI sees. -# Regenerate by running the suite several times with `--save-baseline` and -# comparing `target/criterion///estimates.json`. -# -# `snapshot_files/cold_start_via_snapshot` and `snapshots/restore` stay out -# whatever they measure. Both families have members far outside the threshold, -# and `snapshots/restore/small` swung 1.43x on a later run after measuring 1.25% -# over the five sampled here. -# -# The `hyperlight_common` groups were restructured by the virtqueue transport -# work, which replaced the `alloc_*`, `free*`, `recycle_pool` and -# `segmented_payload` groups with `payload_allocation` and `slot_pool`, and -# shortened the `virtq_*_allocator_strategy` names. The entries below are the -# members of the new groups that hold to the threshold. -# -# The `snapshot_files`, `snapshots`, `sandboxes/sandbox_from_snapshot` and -# `guest_calls/call_with_restore` entries keep the drift measured before that -# work landed. Taking a snapshot fails on this machine now, so those families -# could not be re-measured. -# -# The commented entries widen the allowlist back to the full suite. Uncommenting -# all of them and emptying the denylist reports all 87 benchmarks. The trailing -# count is how many extra benchmarks each one pulls in. +# Benchmarks whose results appear in pull request comments, as regular +# expressions matched against criterion ids. A benchmark is reported when it +# matches `allowlist` and no `denylist` entry, and an allowlist entry matching +# nothing fails the report. allowlist = [ - # "^function_call_serialization/", # adds 2 - "^guest_calls/", - # "^guest_functions_with_large_parameters/", # adds 1 - "^payload_allocation/", - "^sample_workloads/", - # "^sandboxes/", # adds 12 - # "^shared_memory/", # adds 4 - "^shared_memory/copy_to_slice/1MB$", - "^shared_memory/fill/1MB$", - # "^slot_pool/", # adds 2 + "^function_call_codec/decode_vec_bytes_copy$", + "^function_call_codec/encode_control/byte_chunks$", + "^function_call_codec/encode_control/vec_bytes$", + "^payload_allocation/slot_pool_segmented/262144$", + "^payload_allocation/slot_pool_segmented/65536$", + "^sandboxes/create_initialized_and_drop/medium$", "^slot_pool/alloc_dealloc_128$", - # "^snapshot_files/", # adds 19 - "^snapshot_files/load_snapshot/large$", - "^snapshot_files/load_snapshot_unverified/large$", - "^snapshot_files/load_snapshot_unverified/medium$", + "^slot_pool/alloc_dealloc_1500$", + "^slot_pool/alloc_dealloc_4096$", "^snapshot_files/load_snapshot_unverified/small$", - "^snapshot_files/save_snapshot/large$", - # "^snapshots/", # adds 8 - "^virtq_readonly/", - # "^virtq_readwrite/", # adds 4 + "^virtq_readonly/slot_pool_segmented/262144$", + "^virtq_readonly/slot_pool_segmented/65536$", + "^virtq_readonly/slot_pool_segmented_fragmented/262144$", + "^virtq_readonly/slot_pool_segmented_fragmented/65536$", + "^virtq_readwrite/slot_pool_segmented/262144$", "^virtq_readwrite/slot_pool_segmented/65536$", "^virtq_readwrite/slot_pool_segmented/8192$", + "^virtq_readwrite/slot_pool_segmented_fragmented/65536$", + "^virtq_readwrite/slot_pool_segmented_fragmented/8192$", ] -# Noisy members of otherwise stable groups, with the measured drift. -# -# Every `sandboxes` benchmark drifted by more than 9%, reaching 218% on -# `create_initialized/small`, so that group is absent from the allowlist rather -# than listed here. `function_call_serialization` (9% to 15%) and -# `guest_functions_with_large_parameters` (8%) are absent for the same reason, -# as are the `slot_pool` and `virtq_readwrite` members left out above. -denylist = [ - "^guest_calls/call_with_restore/large$", # 13% - "^guest_calls/call_with_restore/medium$", # 16% - "^guest_calls/call_with_restore/small$", # 11% - "^guest_calls/interrupt_latency$", # 51% - "^virtq_readonly/slot_pool_segmented/8192$", # 19% - "^virtq_readonly/slot_pool_segmented_fragmented/8192$", # 11% -] +# Every entry names one benchmark, so nothing needs excluding. +denylist = [] + From 26fb31a73e6617105776d6611db5634e89a786bb Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Fri, 25 Sep 2026 22:03:25 +0100 Subject: [PATCH 12/12] Choose what counts as a change when reporting What the report calls a regression is a judgement about the machine it ran on, not about the benchmark. Two runs of one commit raise an alert in every report at the default ratios, and in none of them once a change has to reach 1.5x either way. `improvement`, `strong_improvement` and `regression` set where those lines fall, in the config file beside the benchmarks they apply to, or on the command line for a one-off reading. The renderer rejects ratios that cross or invert. `summary_limit` and `reproduce` settle beside them, being what a repository decides once rather than per report. `repo` reads `remote:` as whichever repository a git remote points at, so a fork reads its own runs instead of the one written into the tool. The config file is read once for a report rather than once for each configuration it covers. Signed-off-by: Jorge Prendes --- bench_report.toml | 17 +++++ src/hyperlight_ci/src/bench_report.rs | 105 ++++++++++++++++++++++---- src/hyperlight_ci/src/config.rs | 40 ++++++++++ src/hyperlight_ci/src/remote.rs | 66 ++++++++++++++++ 4 files changed, 212 insertions(+), 16 deletions(-) diff --git a/bench_report.toml b/bench_report.toml index b1fac85356..de30b510b5 100644 --- a/bench_report.toml +++ b/bench_report.toml @@ -27,3 +27,20 @@ allowlist = [ # Every entry names one benchmark, so nothing needs excluding. denylist = [] +# How far a result moves before a report calls it a change rather than noise. +# The improvements are how many times faster, `regression` the fraction of the +# baseline a result falls to. +improvement = 1.5 # 1.5x faster +strong_improvement = 1.8 # 1.8x faster +regression = 0.67 # 1.5x slower + +# Whose runs `run:`, `pr:` and the rest read, `/` or `remote:` +# for whichever repository a git remote points at. +repo = "hyperlight-dev/hyperlight" + +# How many changes are called out before the tables, none at 0. +summary_limit = 3 + +# End a report with the command that asks for it again, as CI does for the +# pull request comment. +reproduce = false diff --git a/src/hyperlight_ci/src/bench_report.rs b/src/hyperlight_ci/src/bench_report.rs index a05951bc17..653fe709a6 100644 --- a/src/hyperlight_ci/src/bench_report.rs +++ b/src/hyperlight_ci/src/bench_report.rs @@ -17,6 +17,9 @@ use crate::{manifest, remote}; /// Where downloaded runs are kept. const RUN_CACHE: &str = "target/ci-runs"; +/// Whose runs a report reads, when nothing else says. +const DEFAULT_REPO: &str = "hyperlight-dev/hyperlight"; + /// Where results come from, either a criterion directory or CI. #[derive(Clone)] pub enum Source { @@ -153,13 +156,10 @@ pub struct BenchReportArgs { #[arg(long, value_name = "SOURCE")] pub baseline: Option, - /// Repository holding the CI runs - #[arg( - long, - value_name = "OWNER/NAME", - default_value = "hyperlight-dev/hyperlight" - )] - pub repo: String, + /// Repository holding the CI runs, `/` or `remote:` for + /// whichever one a git remote points at [default: hyperlight-dev/hyperlight] + #[arg(long, value_name = "REPO")] + pub repo: Option, /// Wrap the output in a collapsible
tag with the given summary text. #[arg(long)] @@ -169,9 +169,25 @@ pub struct BenchReportArgs { #[arg(long, value_name = "PATH")] pub config_file: Option, + /// Call a result improved once it is this many times faster [default: 1.1] + #[arg(long, value_name = "RATIO")] + pub improvement: Option, + + /// Call an improvement strong once it is this many times faster [default: 1.8] + #[arg(long, value_name = "RATIO")] + pub strong_improvement: Option, + + /// Call a result regressed once it is this fraction of the baseline [default: 0.9] + #[arg(long, value_name = "RATIO")] + pub regression: Option, + + /// How many changes to call out before the tables, none at 0 [default: 3] + #[arg(long, value_name = "COUNT")] + pub summary_limit: Option, + /// End the report with the command that asks for it again - #[arg(long)] - pub reproduce: bool, + #[arg(long, value_name = "BOOL", num_args = 0..=1, default_missing_value = "true")] + pub reproduce: Option, /// Additional arguments to forward to criterion benchmarks (e.g. filter, --exact) #[arg(trailing_var_arg = true, allow_hyphen_values = true)] @@ -180,7 +196,27 @@ pub struct BenchReportArgs { /// Entry point for the bench-report subcommand. pub async fn run(args: BenchReportArgs) -> Result<()> { - let candidate = resolve(&args.candidate, &args.repo)?; + let config = args + .config_file + .as_deref() + .map(BenchConfig::load) + .transpose()?; + let thresholds = thresholds(&args, config.as_ref()); + let repo = args + .repo + .clone() + .or_else(|| config.as_ref().and_then(|c| c.repo.clone())) + .unwrap_or_else(|| DEFAULT_REPO.to_string()); + let repo = remote::repository(&repo)?; + let summary_limit = args + .summary_limit + .or_else(|| config.as_ref().and_then(|c| c.summary_limit)); + let reproduce_wanted = args + .reproduce + .or_else(|| config.as_ref().and_then(|c| c.reproduce)) + .unwrap_or(false); + + 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. @@ -195,7 +231,7 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { inputs: Vec::new(), }; if let Some(source) = &source { - match resolve(source, &args.repo) { + match resolve(source, &repo) { Ok(found) => baseline = found, // What the results are worth on their own outlives the comparison, // so a baseline out of reach costs the changes, not the report. @@ -215,7 +251,7 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { } } - if let Some(measured) = measured(&args.repo, &candidate, &baseline) { + if let Some(measured) = measured(&repo, &candidate, &baseline) { print!("{measured}"); } @@ -224,6 +260,9 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { let label = candidate.label.as_deref(); let markdown = report( &args, + config.as_ref(), + thresholds, + summary_limit, &candidate.dir, baseline_for(&baseline.inputs, candidate), title(args.collapsible.as_deref(), label), @@ -232,7 +271,7 @@ pub async fn run(args: BenchReportArgs) -> Result<()> { print!("{markdown}"); } - if args.reproduce { + if reproduce_wanted { print!("{}", reproduce(&args, &candidate, &baseline)); } @@ -258,6 +297,31 @@ fn reproduce(args: &BenchReportArgs, candidate: &Origin, baseline: &Origin) -> S format!("\nReported by `{command}`.\n") } +/// Where a change is worth reporting. The command line answers first, then the +/// config file, then the renderer. +fn thresholds( + args: &BenchReportArgs, + config: Option<&BenchConfig>, +) -> criterion_markdown::ChangeThresholds { + let from_config = |read: fn(&BenchConfig) -> Option| config.and_then(read); + let mut thresholds = criterion_markdown::ChangeThresholds::default(); + + if let Some(ratio) = args.improvement.or_else(|| from_config(|c| c.improvement)) { + thresholds = thresholds.improvement_ratio(ratio); + } + if let Some(ratio) = args + .strong_improvement + .or_else(|| from_config(|c| c.strong_improvement)) + { + thresholds = thresholds.strong_improvement_ratio(ratio); + } + if let Some(ratio) = args.regression.or_else(|| from_config(|c| c.regression)) { + thresholds = thresholds.regression_ratio(ratio); + } + + thresholds +} + /// Say which commits the report covers, so a reader can tell what they are /// looking at without knowing how it was asked for. fn measured(repo: &str, candidate: &Origin, baseline: &Origin) -> Option { @@ -407,17 +471,26 @@ fn describe(label: &str) -> String { /// Render the results in `dir`. async fn report( args: &BenchReportArgs, + config: Option<&BenchConfig>, + thresholds: criterion_markdown::ChangeThresholds, + summary_limit: Option, dir: &Path, baseline_root: Option<&Path>, title: Option, ) -> Result { let mut benchmarks = discover_benchmarks(args, dir).await?; - if let Some(path) = &args.config_file { - benchmarks = BenchConfig::load(path)?.select(benchmarks)?; + if let Some(config) = config { + benchmarks = config.select(benchmarks)?; } - let mut renderer = criterion_markdown::Renderer::new(dir).benchmarks(benchmarks); + let mut renderer = criterion_markdown::Renderer::new(dir) + .benchmarks(benchmarks) + .change_thresholds(thresholds); + + if let Some(limit) = summary_limit { + renderer = renderer.summary_limit(limit); + } // Criterion keeps the last run of a directory in `new` and the one before // it in `base`, so another directory is compared through its own last run. diff --git a/src/hyperlight_ci/src/config.rs b/src/hyperlight_ci/src/config.rs index 355de9a7a6..feb56971be 100644 --- a/src/hyperlight_ci/src/config.rs +++ b/src/hyperlight_ci/src/config.rs @@ -16,6 +16,12 @@ struct ConfigFile { allowlist: Vec, #[serde(default)] denylist: Vec, + improvement: Option, + strong_improvement: Option, + regression: Option, + repo: Option, + summary_limit: Option, + reproduce: Option, } /// Benchmark id patterns selecting which results are reported. @@ -23,6 +29,16 @@ struct ConfigFile { pub struct BenchConfig { allow: RegexSet, deny: RegexSet, + /// Where a change is worth reporting, when the file says. + pub improvement: Option, + pub strong_improvement: Option, + pub regression: Option, + /// Which repository the runs belong to. + pub repo: Option, + /// How many changes to call out before the tables. + pub summary_limit: Option, + /// Whether a report says how to ask for it again. + pub reproduce: Option, } impl BenchConfig { @@ -40,6 +56,12 @@ impl BenchConfig { Ok(Self { allow: RegexSet::new(&file.allowlist)?, deny: RegexSet::new(&file.denylist)?, + improvement: file.improvement, + strong_improvement: file.strong_improvement, + regression: file.regression, + repo: file.repo, + summary_limit: file.summary_limit, + reproduce: file.reproduce, }) } @@ -100,6 +122,24 @@ mod tests { names.iter().map(|name| name.to_string()).collect() } + #[test] + fn reads_the_thresholds_when_given() { + let config = BenchConfig::parse("improvement = 1.5\nregression = 0.5").unwrap(); + + assert_eq!(config.improvement, Some(1.5)); + assert_eq!(config.regression, Some(0.5)); + assert_eq!(config.strong_improvement, None); + } + + #[test] + fn leaves_the_thresholds_alone_when_absent() { + let config = BenchConfig::parse(r#"allowlist = ["^sandboxes/"]"#).unwrap(); + + assert_eq!(config.improvement, None); + assert_eq!(config.strong_improvement, None); + assert_eq!(config.regression, None); + } + #[test] fn selects_only_allowlisted_benchmarks() { let config = BenchConfig::parse(r#"allowlist = ["^sandboxes/", "^guest_calls/"]"#).unwrap(); diff --git a/src/hyperlight_ci/src/remote.rs b/src/hyperlight_ci/src/remote.rs index 8a2460c023..0606d3bb0b 100644 --- a/src/hyperlight_ci/src/remote.rs +++ b/src/hyperlight_ci/src/remote.rs @@ -77,6 +77,42 @@ fn gh(args: &[&str]) -> Result> { Ok(output.stdout) } +/// Which repository to read, either `owner/name` or whichever one a git +/// remote points at, `remote:origin`. +pub(crate) fn repository(value: &str) -> Result { + let Some(remote) = value.strip_prefix("remote:") else { + return Ok(value.to_string()); + }; + + let output = Command::new("git") + .args(["remote", "get-url", remote]) + .output() + .context("Failed to run git")?; + + if !output.status.success() { + bail!( + "Failed to read the url of remote {remote}: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + + let url = String::from_utf8_lossy(&output.stdout); + owner_and_name(&url) + .with_context(|| format!("Remote {remote} names no repository: {}", url.trim())) +} + +/// The owner and name a clone url ends with, however it spells the host. +fn owner_and_name(url: &str) -> Option { + let url = url.trim().trim_end_matches('/'); + let url = url.strip_suffix(".git").unwrap_or(url); + + let mut parts = url.rsplit(['/', ':']); + let name = parts.next()?; + let owner = parts.next()?; + + (!name.is_empty() && !owner.is_empty()).then(|| format!("{owner}/{name}")) +} + /// Names of the benchmark artifacts a run still holds. fn artifacts(repo: &str, run: u64) -> Result> { let path = format!("repos/{repo}/actions/runs/{run}/artifacts"); @@ -370,3 +406,33 @@ fn unpack(archive: &Path, into: &Path) -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn reads_the_repository_a_clone_url_ends_with() { + for url in [ + "git@github.com:hyperlight-dev/hyperlight.git", + "https://github.com/hyperlight-dev/hyperlight.git", + "https://github.com/hyperlight-dev/hyperlight", + "ssh://git@github.com/hyperlight-dev/hyperlight.git", + " git@github.com:hyperlight-dev/hyperlight.git\n", + ] { + assert_eq!( + owner_and_name(url).as_deref(), + Some("hyperlight-dev/hyperlight"), + "{url}" + ); + } + } + + #[test] + fn keeps_a_repository_named_outright() { + assert_eq!( + repository("hyperlight-dev/hyperlight").unwrap(), + "hyperlight-dev/hyperlight" + ); + } +}