diff --git a/controller/src/runner/bench_criterion.rs b/controller/src/runner/bench_criterion.rs index 95397e1..6105047 100644 --- a/controller/src/runner/bench_criterion.rs +++ b/controller/src/runner/bench_criterion.rs @@ -232,7 +232,7 @@ fn bench_command_args(bench_name: &str) -> Vec { } /// Copy criterion baselines from base to branch target directory. -async fn copy_criterion_baselines(base_dir: &Path, branch_dir: &Path) { +pub(crate) async fn copy_criterion_baselines(base_dir: &Path, branch_dir: &Path) { let src = base_dir.join("target/criterion"); let dst = branch_dir.join("target/criterion"); if src.exists() { diff --git a/controller/src/runner/bench_standard.rs b/controller/src/runner/bench_standard.rs index d95aafb..9571e71 100644 --- a/controller/src/runner/bench_standard.rs +++ b/controller/src/runner/bench_standard.rs @@ -6,12 +6,22 @@ use anyhow::{Context, Result}; use tracing::info; use crate::github; +use crate::runner::bench_criterion::copy_criterion_baselines; use crate::runner::config::RunnerConfig; use crate::runner::git; use crate::runner::monitor::{self, ResourceStats}; use crate::runner::poster::CommentPoster; use crate::runner::shell; +/// Benchmarks orchestrated by `bench.sh` but executed through the Criterion SQL +/// harness (`cargo bench --bench sql`). Unlike the dfbench-based suites, these +/// write timings to `target/criterion/` rather than `results/*.json`, so they +/// are compared with `critcmp` (against per-side `--save-baseline`s) instead of +/// `bench.sh compare_detail`. +fn is_criterion_harness(bench: &str) -> bool { + matches!(bench, "wide_schema" | "predicate_eval") +} + /// Run standard bench.sh benchmarks comparing a PR branch to its merge-base. pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> { let repo_url = config.repo_url(); @@ -187,14 +197,42 @@ pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> { branch_stats_list.push((bench, branch_stats)); } - // Compare and post results - let report = shell::run_command( - "./bench.sh", - &["compare_detail", &base_results_name, &bench_branch_name], - &bench_benchmarks, - ) - .await - .context("bench.sh compare")?; + // Compare and post results. dfbench-based suites are compared from their + // results/*.json via `bench.sh compare_detail`; Criterion-harness suites + // (which only produce target/criterion) are compared with `critcmp`. + let ran: Vec<&str> = benchmarks.split_whitespace().collect(); + let has_standard = ran.iter().any(|b| !is_criterion_harness(b)); + let has_criterion = ran.iter().any(|b| is_criterion_harness(b)); + + let mut report = String::new(); + + if has_standard { + let detail = shell::run_command( + "./bench.sh", + &["compare_detail", &base_results_name, &bench_branch_name], + &bench_benchmarks, + ) + .await + .context("bench.sh compare")?; + report.push_str(&detail); + } + + if has_criterion { + // Gather both sides' baselines into one target/criterion tree, then + // diff the base ("HEAD") and branch baselines. + copy_criterion_baselines(&base_dir, &branch_dir).await; + let critcmp = shell::run_command( + "critcmp", + &[base_results_name.as_str(), bench_branch_name.as_str()], + &branch_dir, + ) + .await + .context("critcmp")?; + if !report.is_empty() { + report.push('\n'); + } + report.push_str(&critcmp); + } let resource_section = format_resource_section(&base_stats_list, &branch_stats_list); let result_body = format_result_comment( @@ -249,6 +287,13 @@ async fn run_one_side( format!("RESULTS_NAME={results_name}"), format!("DATAFUSION_RUNTIME_TEMP_DIRECTORY={}", spill_dir.display()), ]; + // Criterion-harness suites don't emit results/*.json; have them save a + // named Criterion baseline per side so we can compare with critcmp. + if is_criterion_harness(bench) { + args.push(format!( + "SQL_CARGO_COMMAND=cargo bench --bench sql -- --save-baseline {results_name}" + )); + } args.extend(extra_env.iter().cloned()); args.extend([ "./bench.sh".to_string(), @@ -474,6 +519,18 @@ mod tests { assert!(comment.contains("lscpu output")); } + #[test] + fn criterion_harness_classification() { + // Criterion SQL-harness suites → compared via critcmp. + assert!(is_criterion_harness("wide_schema")); + assert!(is_criterion_harness("predicate_eval")); + // dfbench/json suites → compared via bench.sh compare_detail. + assert!(!is_criterion_harness("tpch")); + assert!(!is_criterion_harness("clickbench_1")); + assert!(!is_criterion_harness("imdb")); + assert!(!is_criterion_harness("h2o")); + } + #[test] fn tpch_direct_args_maps_variants() { let (args, results) = tpch_direct_args("tpch").unwrap();