From 37a61cc0cfe743d42352d46da9b23741beae6168 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Mon, 5 Oct 2026 18:05:09 +0100 Subject: [PATCH] Read a benchmark baseline from a listing that is current A pull request is measured against the daily run at the commit it branched from, found by walking the successful runs of that workflow. Asking GitHub for only the successful ones is what made it answer out of a cold index: that listing came back weeks stale in four of ten tries, stably enough that two replies agreed on it, while the same listing unnarrowed was current every time. The narrowing happens here now. PR #1881 branched at `66d5dd3e` on 2 October and was reported against `c3e1d311` from 4 September. Of 114 table cells, 102 held no comparison at all, because most of the reported benchmarks did not exist a month earlier, and two configurations announced a regression of 3.79x for a change that redirects fuzzer output. Reading the same run now finds `66d5dd3e` itself and fills every cell. A baseline that far back reads the same as one next door, so the run says how many commits separate them. Signed-off-by: Jorge Prendes --- src/hyperlight_ci/src/remote.rs | 142 ++++++++++++++++++++++++++++---- 1 file changed, 124 insertions(+), 18 deletions(-) diff --git a/src/hyperlight_ci/src/remote.rs b/src/hyperlight_ci/src/remote.rs index d5a44f7f6..4f8961331 100644 --- a/src/hyperlight_ci/src/remote.rs +++ b/src/hyperlight_ci/src/remote.rs @@ -58,6 +58,7 @@ struct Run { head_sha: String, #[serde(rename = "createdAt")] created_at: String, + conclusion: String, } /// Run `gh` and hand back its stdout. @@ -137,6 +138,11 @@ fn artifacts(repo: &str, run: u64) -> Result> { /// 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. +/// +/// Asking GitHub to filter is what leaves an answer cold. A listing narrowed +/// by status came back weeks stale in four of ten tries, and stably enough +/// that consecutive replies agreed on it, while the same listing unnarrowed +/// was current every time. Narrow the results here instead. fn runs(repo: &str, filter: &[&str]) -> Result> { let limit = RUNS_SEARCHED.to_string(); let mut seen: Vec = Vec::new(); @@ -151,7 +157,7 @@ fn runs(repo: &str, filter: &[&str]) -> Result> { "--limit", &limit, "--json", - "databaseId,headSha,createdAt", + "databaseId,headSha,createdAt,conclusion", ]; args.extend_from_slice(filter); @@ -227,14 +233,49 @@ pub(crate) fn run_commit(repo: &str, run: u64) -> Result { 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 { +/// How many commits `commit` sits past `ancestor`, or `None` when it was not +/// built on top of it. +fn commits_between(repo: &str, commit: &str, ancestor: &str) -> Result> { + #[derive(Deserialize)] + struct Comparison { + status: String, + ahead_by: u64, + } + 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" - )) + let comparison: Comparison = + serde_json::from_slice(&gh(&["api", &path])?).context("Failed to compare two commits")?; + + Ok(matches!(comparison.status.as_str(), "identical" | "ahead").then_some(comparison.ahead_by)) +} + +/// The newest run that measured something `commit` was built on top of, and +/// how many commits separate the two. +/// +/// `behind` says how far a run's commit sits before the one asked for, or +/// `None` when the run measured a different line of development. `kept` says +/// whether a run still has its artifacts. +fn baseline( + runs: &[Run], + behind: impl Fn(&str) -> Result>, + kept: impl Fn(u64) -> Result, +) -> Result> { + for run in runs { + // A cancelled run leaves some configurations unmeasured. + if run.conclusion != "success" { + continue; + } + + let Some(behind) = behind(&run.head_sha)? else { + continue; + }; + + if kept(run.id)? { + return Ok(Some((run, behind))); + } + } + + Ok(None) } /// The most recent benchmarks of the default branch taken at or before @@ -245,19 +286,25 @@ 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)?; - // A cancelled run leaves some configurations unmeasured. - let runs = runs( - repo, - &["--workflow", BASELINE_WORKFLOW, "--status", "success"], + let runs = runs(repo, &["--workflow", BASELINE_WORKFLOW])?; + + let found = baseline( + &runs, + |sha| commits_between(repo, &commit, sha), + |id| Ok(!artifacts(repo, id)?.is_empty()), )?; - for run in &runs { - if descends_from(repo, &commit, &run.head_sha)? && !artifacts(repo, run.id)?.is_empty() { - return Ok(run.id); - } - } + let Some((run, behind)) = found else { + bail!("No benchmarks taken at or before {commit} still have their artifacts") + }; - bail!("No benchmarks taken at or before {commit} still have their artifacts") + // How far back the branch point is says whether the baseline is the one + // meant, which a stale listing silently answers wrong. + eprintln!( + "Taking benchmarks of {}, {behind} commits back", + &run.head_sha[..12] + ); + Ok(run.id) } /// Where `pull_request` branched off the branch it targets. @@ -446,4 +493,63 @@ mod tests { "hyperlight-dev/hyperlight" ); } + + /// Newest first, as a listing arrives. + fn listing() -> Vec { + ["failure", "success", "success", "success", "success"] + .into_iter() + .enumerate() + .map(|(i, conclusion)| Run { + id: 50 - i as u64, + head_sha: format!("{:040x}", 50 - i), + created_at: format!("2026-10-{:02}T00:00:00Z", 10 - i), + conclusion: conclusion.to_string(), + }) + .collect() + } + + /// Every run measured the line of development asked about, one commit + /// further back each time, and kept its artifacts. + fn ancestral(sha: &str) -> Result> { + Ok(Some( + 50 - u64::from_str_radix(sha.trim_start_matches('0'), 16).unwrap(), + )) + } + + fn kept(_: u64) -> Result { + Ok(true) + } + + #[test] + fn passes_over_a_run_that_did_not_finish() { + let runs = listing(); + let (run, behind) = baseline(&runs, ancestral, kept).unwrap().unwrap(); + assert_eq!(run.id, 49); + assert_eq!(behind, 1); + } + + #[test] + fn passes_over_a_run_that_lost_its_artifacts() { + let runs = listing(); + let (run, _) = baseline(&runs, ancestral, |id| Ok(id < 48)) + .unwrap() + .unwrap(); + assert_eq!(run.id, 47); + } + + #[test] + fn passes_over_a_run_of_another_line_of_development() { + let runs = listing(); + let (run, behind) = baseline(&runs, |sha| Ok(ancestral(sha)?.filter(|&n| n > 2)), kept) + .unwrap() + .unwrap(); + assert_eq!(run.id, 47); + assert_eq!(behind, 3); + } + + #[test] + fn finds_nothing_to_compare_against() { + let runs = listing(); + assert!(baseline(&runs, |_| Ok(None), kept).unwrap().is_none()); + } }