From 1cb96c189becd5a95e456c0a965ff2d031af1a97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Fri, 11 Oct 2024 21:02:15 +0000 Subject: [PATCH 1/5] add optional `backends` to benchmark commands --- collector/src/api.rs | 1 + collector/src/bin/collector.rs | 32 ++++++++++++++++------ collector/src/toolchain.rs | 6 +--- database/src/bin/postgres-to-sqlite.rs | 5 ++-- database/src/bin/sqlite-to-postgres.rs | 6 ++-- database/src/lib.rs | 1 + database/src/pool.rs | 1 + database/src/pool/postgres.rs | 14 ++++++---- database/src/pool/sqlite.rs | 16 +++++++---- site/frontend/src/pages/status/data.ts | 1 + site/src/load.rs | 10 +++++++ site/src/request_handlers/github.rs | 4 +++ site/src/request_handlers/next_artifact.rs | 15 ++++++---- 13 files changed, 78 insertions(+), 34 deletions(-) diff --git a/collector/src/api.rs b/collector/src/api.rs index 41101e7bb..845bf6a50 100644 --- a/collector/src/api.rs +++ b/collector/src/api.rs @@ -9,6 +9,7 @@ pub mod next_artifact { include: Option, exclude: Option, runs: Option, + backends: Option, }, } diff --git a/collector/src/bin/collector.rs b/collector/src/bin/collector.rs index 9eb8526fb..dd1d151f8 100644 --- a/collector/src/bin/collector.rs +++ b/collector/src/bin/collector.rs @@ -961,7 +961,25 @@ fn main_result() -> anyhow::Result { include, exclude, runs, + backends: requested_backends, } => { + // Parse the requested backends, with LLVM as a fallback if none or no valid + // ones were explicitly specified, which will be the case for the vast + // majority of cases. + let mut backends = vec![]; + if let Some(requested_backends) = requested_backends { + if requested_backends.contains("Llvm") { + backends.push(CodegenBackend::Llvm); + } + if requested_backends.contains("Cranelift") { + backends.push(CodegenBackend::Cranelift); + } + } + + if backends.is_empty() { + backends.push(CodegenBackend::Llvm); + } + // FIXME: remove this when/if NextArtifact::Commit's include/exclude // changed from Option to Vec // to not to manually parse args @@ -973,12 +991,10 @@ fn main_result() -> anyhow::Result { } }; let sha = commit.sha.to_string(); - let sysroot = Sysroot::install( - sha.clone(), - &target_triple, - vec![CodegenBackend::Llvm], - ) - .with_context(|| format!("failed to install sysroot for {:?}", commit))?; + let sysroot = Sysroot::install(sha.clone(), &target_triple, &backends) + .with_context(|| { + format!("failed to install sysroot for {:?}", commit) + })?; let mut benchmarks = get_compile_benchmarks( &compile_benchmark_dir, @@ -1001,7 +1017,7 @@ fn main_result() -> anyhow::Result { Profile::Opt, ], scenarios: Scenario::all(), - backends: vec![CodegenBackend::Llvm], + backends, iterations: runs.map(|v| v as usize), is_self_profile: self_profile.self_profile, bench_rustc: bench_rustc.bench_rustc, @@ -1174,7 +1190,7 @@ fn main_result() -> anyhow::Result { let last_sha = String::from_utf8(last_sha.stdout).expect("utf8"); let last_sha = last_sha.split_whitespace().next().expect(&last_sha); let commit = get_commit_or_fake_it(last_sha).expect("success"); - let mut sysroot = Sysroot::install(commit.sha, &target_triple, codegen_backends.0)?; + let mut sysroot = Sysroot::install(commit.sha, &target_triple, &codegen_backends.0)?; sysroot.preserve(); // don't delete it // Print the directory containing the toolchain. diff --git a/collector/src/toolchain.rs b/collector/src/toolchain.rs index d7731afa3..d576fb05f 100644 --- a/collector/src/toolchain.rs +++ b/collector/src/toolchain.rs @@ -20,11 +20,7 @@ pub struct Sysroot { } impl Sysroot { - pub fn install( - sha: String, - triple: &str, - backends: Vec, - ) -> anyhow::Result { + pub fn install(sha: String, triple: &str, backends: &[CodegenBackend]) -> anyhow::Result { let unpack_into = "cache"; fs::create_dir_all(unpack_into)?; diff --git a/database/src/bin/postgres-to-sqlite.rs b/database/src/bin/postgres-to-sqlite.rs index 4f97d87f5..b45095fc8 100644 --- a/database/src/bin/postgres-to-sqlite.rs +++ b/database/src/bin/postgres-to-sqlite.rs @@ -275,13 +275,13 @@ impl Table for PullRequestBuild { } fn postgres_select_statement(&self, _since_weeks_ago: Option) -> String { - "select bors_sha, pr, parent_sha, complete, requested, include, exclude, runs from " + "select bors_sha, pr, parent_sha, complete, requested, include, exclude, runs, backends from " .to_string() + self.name() } fn sqlite_insert_statement(&self) -> &'static str { - "insert into pull_request_build (bors_sha, pr, parent_sha, complete, requested, include, exclude, runs) VALUES (?, ?, ?, ?, ?, ?, ?, ?)" + "insert into pull_request_build (bors_sha, pr, parent_sha, complete, requested, include, exclude, runs, backends) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)" } fn sqlite_execute_insert(&self, statement: &mut rusqlite::Statement, row: tokio_postgres::Row) { @@ -296,6 +296,7 @@ impl Table for PullRequestBuild { row.get::<_, Option<&str>>(5), row.get::<_, Option<&str>>(6), row.get::<_, Option>(7), + row.get::<_, Option<&str>>(8), ]) .unwrap(); } diff --git a/database/src/bin/sqlite-to-postgres.rs b/database/src/bin/sqlite-to-postgres.rs index 7092491b0..e2f01ac37 100644 --- a/database/src/bin/sqlite-to-postgres.rs +++ b/database/src/bin/sqlite-to-postgres.rs @@ -369,6 +369,7 @@ struct PullRequestBuildRow<'a> { exclude: Nullable<&'a str>, runs: Nullable, commit_date: Nullable>, + backends: Nullable<&'a str>, } impl Table for PullRequestBuild { @@ -377,11 +378,11 @@ impl Table for PullRequestBuild { } fn sqlite_attributes() -> &'static str { - "bors_sha, pr, parent_sha, complete, requested, include, exclude, runs, commit_date" + "bors_sha, pr, parent_sha, complete, requested, include, exclude, runs, commit_date, backends" } fn postgres_attributes() -> &'static str { - "bors_sha, pr, parent_sha, complete, requested, include, exclude, runs, commit_date" + "bors_sha, pr, parent_sha, complete, requested, include, exclude, runs, commit_date, backends" } fn postgres_generated_id_attribute() -> Option<&'static str> { @@ -407,6 +408,7 @@ impl Table for PullRequestBuild { commit_date: Nullable( commit_date.map(|seconds| Utc.timestamp_opt(seconds, 0).unwrap()), ), + backends: row.get_ref(9).unwrap().try_into().unwrap(), }) .unwrap(); } diff --git a/database/src/lib.rs b/database/src/lib.rs index 19b749243..aaa00ece8 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -28,6 +28,7 @@ pub struct QueuedCommit { pub exclude: Option, pub runs: Option, pub commit_date: Option, + pub backends: Option, } #[derive(Debug, Hash, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] diff --git a/database/src/pool.rs b/database/src/pool.rs index 38a6a659f..61439c927 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -121,6 +121,7 @@ pub trait Connection: Send + Sync { include: Option<&str>, exclude: Option<&str>, runs: Option, + backends: Option<&str>, ); /// Returns true if this PR was queued waiting for a commit async fn pr_attach_commit( diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 1dfc840c6..621f3e1bf 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -278,6 +278,7 @@ static MIGRATIONS: &[&str] = &[ alter table pstat_series drop constraint pstat_series_crate_profile_cache_statistic_key; alter table pstat_series add constraint test_case UNIQUE(crate, profile, scenario, backend, metric); "#, + r#"alter table pull_request_build add column backends text;"#, ]; #[async_trait::async_trait] @@ -736,14 +737,15 @@ where include: Option<&str>, exclude: Option<&str>, runs: Option, + backends: Option<&str>, ) { if let Err(e) = self.conn() .execute( - "insert into pull_request_build (pr, complete, requested, include, exclude, runs) VALUES ($1, false, CURRENT_TIMESTAMP, $2, $3, $4)", - &[&(pr as i32), &include, &exclude, &runs], + "insert into pull_request_build (pr, complete, requested, include, exclude, runs, backends) VALUES ($1, false, CURRENT_TIMESTAMP, $2, $3, $4, $5)", + &[&(pr as i32), &include, &exclude, &runs, &backends], ) .await { - log::error!("failed to queue_pr({}, {:?}, {:?}, {:?}): {:?}", pr, include, exclude, runs, e); + log::error!("failed to queue_pr({}, {:?}, {:?}, {:?}, {:?}): {:?}", pr, include, exclude, runs, backends, e); } } async fn pr_attach_commit( @@ -767,7 +769,7 @@ where let rows = self .conn() .query( - "select pr, bors_sha, parent_sha, include, exclude, runs, commit_date from pull_request_build + "select pr, bors_sha, parent_sha, include, exclude, runs, commit_date, backends from pull_request_build where complete is false and bors_sha is not null order by requested asc", &[], @@ -783,6 +785,7 @@ where exclude: row.get(4), runs: row.get(5), commit_date: row.get::<_, Option<_>>(6).map(Date), + backends: row.get(7), }) .collect() } @@ -792,7 +795,7 @@ where .query_opt( "update pull_request_build SET complete = true where bors_sha = $1 - returning pr, bors_sha, parent_sha, include, exclude, runs, commit_date", + returning pr, bors_sha, parent_sha, include, exclude, runs, commit_date, backends", &[&sha], ) .await @@ -805,6 +808,7 @@ where exclude: row.get(4), runs: row.get(5), commit_date: row.get::<_, Option<_>>(6).map(Date), + backends: row.get(7), }) } async fn collection_id(&self, version: &str) -> CollectionId { diff --git a/database/src/pool/sqlite.rs b/database/src/pool/sqlite.rs index 8a9fefcbf..d7a16a5a4 100644 --- a/database/src/pool/sqlite.rs +++ b/database/src/pool/sqlite.rs @@ -385,6 +385,7 @@ static MIGRATIONS: &[Migration] = &[ alter table pstat_series_new rename to pstat_series; "#, ), + Migration::new("alter table pull_request_build add column backends text"), ]; #[async_trait::async_trait] @@ -909,13 +910,14 @@ impl Connection for SqliteConnection { include: Option<&str>, exclude: Option<&str>, runs: Option, + backends: Option<&str>, ) { self.raw_ref() .prepare_cached( - "insert into pull_request_build (pr, complete, requested, include, exclude, runs) VALUES (?, 0, strftime('%s','now'), ?, ?, ?)", + "insert into pull_request_build (pr, complete, requested, include, exclude, runs, backends) VALUES (?, 0, strftime('%s','now'), ?, ?, ?, ?)", ) .unwrap() - .execute(params![pr, include, exclude, &runs]) + .execute(params![pr, include, exclude, &runs, backends]) .unwrap(); } async fn pr_attach_commit( @@ -939,7 +941,7 @@ impl Connection for SqliteConnection { async fn queued_commits(&self) -> Vec { self.raw_ref() .prepare_cached( - "select pr, bors_sha, parent_sha, include, exclude, runs, commit_date from pull_request_build + "select pr, bors_sha, parent_sha, include, exclude, runs, commit_date, backends from pull_request_build where complete is false and bors_sha is not null order by requested asc", ) @@ -954,7 +956,8 @@ impl Connection for SqliteConnection { include: row.get(3).unwrap(), exclude: row.get(4).unwrap(), runs: row.get(5).unwrap(), - commit_date: row.get::<_, Option>(6).unwrap().map(|timestamp| Date(DateTime::from_timestamp(timestamp, 0).unwrap())) + commit_date: row.get::<_, Option>(6).unwrap().map(|timestamp| Date(DateTime::from_timestamp(timestamp, 0).unwrap())), + backends: row.get(7).unwrap(), }) }) .collect::, _>>() @@ -974,7 +977,7 @@ impl Connection for SqliteConnection { assert_eq!(count, 1, "sha is unique column"); self.raw_ref() .query_row( - "select pr, sha, parent_sha, include, exclude, runs, commit_date from pull_request_build + "select pr, sha, parent_sha, include, exclude, runs, commit_date, backends from pull_request_build where sha = ?", params![sha], |row| { @@ -985,7 +988,8 @@ impl Connection for SqliteConnection { include: row.get(3).unwrap(), exclude: row.get(4).unwrap(), runs: row.get(5).unwrap(), - commit_date: row.get::<_, Option>(6).unwrap().map(|timestamp| Date(DateTime::from_timestamp(timestamp, 0).unwrap())) + commit_date: row.get::<_, Option>(6).unwrap().map(|timestamp| Date(DateTime::from_timestamp(timestamp, 0).unwrap())), + backends: row.get(7).unwrap(), }) }, ) diff --git a/site/frontend/src/pages/status/data.ts b/site/frontend/src/pages/status/data.ts index 6f86ffe4f..48371985e 100644 --- a/site/frontend/src/pages/status/data.ts +++ b/site/frontend/src/pages/status/data.ts @@ -39,6 +39,7 @@ export type MissingReason = include: string | null; exclude: string | null; runs: number | null; + backends: string | null; }; } | { diff --git a/site/src/load.rs b/site/src/load.rs index fe892b69e..1d1b8712e 100644 --- a/site/src/load.rs +++ b/site/src/load.rs @@ -33,6 +33,7 @@ pub enum MissingReason { include: Option, exclude: Option, runs: Option, + backends: Option, }, InProgress(Option>), } @@ -382,6 +383,7 @@ fn calculate_missing_from( exclude, runs, commit_date, + backends, } in queued_pr_commits .into_iter() // filter out any queued PR master commits (leaving only try commits) @@ -407,6 +409,7 @@ fn calculate_missing_from( include, exclude, runs, + backends, }, )); } @@ -579,6 +582,7 @@ mod tests { exclude: None, runs: None, commit_date: None, + backends: None, }, QueuedCommit { sha: "b".into(), @@ -588,6 +592,7 @@ mod tests { exclude: None, runs: None, commit_date: None, + backends: None, }, QueuedCommit { sha: "a".into(), @@ -597,6 +602,7 @@ mod tests { exclude: None, runs: None, commit_date: None, + backends: None, }, ]; let in_progress_artifacts = vec![]; @@ -640,6 +646,7 @@ mod tests { include: None, exclude: None, runs: None, + backends: None, }, ), ]; @@ -689,6 +696,7 @@ mod tests { exclude: None, runs: None, commit_date: None, + backends: None, }, // A try run QueuedCommit { @@ -699,6 +707,7 @@ mod tests { exclude: None, runs: None, commit_date: None, + backends: None, }, ]; let in_progress_artifacts = vec![]; @@ -746,6 +755,7 @@ mod tests { include: None, exclude: None, runs: None, + backends: None, }, ), ]; diff --git a/site/src/request_handlers/github.rs b/site/src/request_handlers/github.rs index 9403a05c2..3e7fc1b1f 100644 --- a/site/src/request_handlers/github.rs +++ b/site/src/request_handlers/github.rs @@ -119,6 +119,7 @@ async fn handle_rust_timer( cmd.params.include, cmd.params.exclude, cmd.params.runs, + cmd.params.backends, ) .await; format!( @@ -158,6 +159,7 @@ async fn handle_rust_timer( command.params.include, command.params.exclude, command.params.runs, + command.params.backends, ) .await; } @@ -225,6 +227,7 @@ fn parse_benchmark_parameters<'a>( include: args.remove("include"), exclude: args.remove("exclude"), runs: None, + backends: args.remove("backends"), }; if let Some(runs) = args.remove("runs") { let Ok(runs) = runs.parse::() else { @@ -280,6 +283,7 @@ struct BenchmarkParameters<'a> { include: Option<&'a str>, exclude: Option<&'a str>, runs: Option, + backends: Option<&'a str>, } pub async fn get_authorized_users() -> Result, String> { diff --git a/site/src/request_handlers/next_artifact.rs b/site/src/request_handlers/next_artifact.rs index 781d5b9a1..95253d119 100644 --- a/site/src/request_handlers/next_artifact.rs +++ b/site/src/request_handlers/next_artifact.rs @@ -30,7 +30,7 @@ pub async fn handle_next_artifact(ctxt: Arc) -> next_artifact::Respons let conn = ctxt.conn().await; // TODO: add capability of doing the following in one step // to avoid possibile illegal inbetween states. - conn.queue_pr(pr, None, None, None).await; + conn.queue_pr(pr, None, None, None, None).await; if !conn .pr_attach_commit(pr, &commit.sha, parent_sha, None) .await @@ -38,27 +38,29 @@ pub async fn handle_next_artifact(ctxt: Arc) -> next_artifact::Respons log::error!("failed to attach commit {} to PR queue", commit.sha); } } - let (include, exclude, runs) = match missing_reason { + let (include, exclude, runs, backends) = match missing_reason { crate::load::MissingReason::Try { include, exclude, runs, + backends, .. - } => (include, exclude, runs), + } => (include, exclude, runs, backends), crate::load::MissingReason::InProgress(Some(previous)) => { if let crate::load::MissingReason::Try { include, exclude, runs, + backends, .. } = *previous { - (include, exclude, runs) + (include, exclude, runs, backends) } else { - (None, None, None) + (None, None, None, None) } } - _ => (None, None, None), + _ => (None, None, None, None), }; log::debug!( "next_commit: {} (missing: {})", @@ -70,6 +72,7 @@ pub async fn handle_next_artifact(ctxt: Arc) -> next_artifact::Respons include, exclude, runs, + backends, }) } else { None From e129ae5de1b696cadbf273d0f8c4bf4efca91329 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Fri, 11 Oct 2024 21:02:55 +0000 Subject: [PATCH 2/5] update database schema documentation --- database/schema.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/database/schema.md b/database/schema.md index b553a3cf2..a51df4b60 100644 --- a/database/schema.md +++ b/database/schema.md @@ -238,11 +238,12 @@ is attached to the entry in this table, it can be benchmarked. * exclude: which benchmarks should be excluded (corresponds to the `--exclude` benchmark parameter) * runs: how many iterations should be used by default for the benchmark run * commit_date: when was the commit created +* backends: the codegen backends to use for the benchmarks (corresponds to the `--backends` benchmark parameter) ``` sqlite> select * from pull_request_build limit 1; -bors_sha pr parent_sha complete requested include exclude runs commit_date ----------- -- ---------- -------- --------- ------- ------- ---- ----------- +bors_sha pr parent_sha complete requested include exclude runs commit_date backends +---------- -- ---------- -------- --------- ------- ------- ---- ----------- -------- 1w0p83... 42 fq24xq... true 3 ``` From 65c90a158fc348153f224eee3d907f1ef23d48e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Fri, 11 Oct 2024 21:03:13 +0000 Subject: [PATCH 3/5] add test parsing the backends from GH commands --- site/src/request_handlers/github.rs | 58 ++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 13 deletions(-) diff --git a/site/src/request_handlers/github.rs b/site/src/request_handlers/github.rs index 3e7fc1b1f..175a41947 100644 --- a/site/src/request_handlers/github.rs +++ b/site/src/request_handlers/github.rs @@ -325,7 +325,7 @@ mod tests { #[test] fn build_command() { insta::assert_compact_debug_snapshot!(get_build_commands("@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af9952"), - @r###"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None } })]"###); + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } })]"#); } #[test] @@ -334,7 +334,7 @@ mod tests { @rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af9952 @rust-timer build 23936af287657fa4148aeab40cc2a780810fae52 "#), - @r###"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None } }), Ok(BuildCommand { sha: "23936af287657fa4148aeab40cc2a780810fae52", params: BenchmarkParameters { include: None, exclude: None, runs: None } })]"###); + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } }), Ok(BuildCommand { sha: "23936af287657fa4148aeab40cc2a780810fae52", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } })]"#); } #[test] @@ -346,14 +346,14 @@ mod tests { #[test] fn build_command_complex() { insta::assert_compact_debug_snapshot!(get_build_commands(" @rust-timer build sha123456 exclude=baz include=foo,bar runs=4"), - @r###"[Ok(BuildCommand { sha: "sha123456", params: BenchmarkParameters { include: Some("foo,bar"), exclude: Some("baz"), runs: Some(4) } })]"###); + @r#"[Ok(BuildCommand { sha: "sha123456", params: BenchmarkParameters { include: Some("foo,bar"), exclude: Some("baz"), runs: Some(4), backends: None } })]"#); } #[test] fn build_command_link() { insta::assert_compact_debug_snapshot!(get_build_commands(r#" @rust-timer build https://github.com/rust-lang/rust/commit/323f521bc6d8f2b966ba7838a3f3ee364e760b7e"#), - @r###"[Ok(BuildCommand { sha: "323f521bc6d8f2b966ba7838a3f3ee364e760b7e", params: BenchmarkParameters { include: None, exclude: None, runs: None } })]"###); + @r#"[Ok(BuildCommand { sha: "323f521bc6d8f2b966ba7838a3f3ee364e760b7e", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } })]"#); } #[test] @@ -369,7 +369,7 @@ mod tests { #[test] fn queue_command() { insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue"), - @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None } }))"); + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } }))"); } #[test] @@ -387,19 +387,19 @@ mod tests { #[test] fn queue_command_include() { insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=abcd,feih"), - @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("abcd,feih"), exclude: None, runs: None } }))"###); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("abcd,feih"), exclude: None, runs: None, backends: None } }))"#); } #[test] fn queue_command_exclude() { insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue exclude=foo134,barzbaz41baf"), - @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: Some("foo134,barzbaz41baf"), runs: None } }))"###); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: Some("foo134,barzbaz41baf"), runs: None, backends: None } }))"#); } #[test] fn queue_command_runs() { insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=5"), - @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: Some(5) } }))"); + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: Some(5), backends: None } }))"); } #[test] @@ -411,7 +411,7 @@ mod tests { #[test] fn queue_command_combination() { insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=acda,13asd exclude=c13,DA runs=5"), - @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("acda,13asd"), exclude: Some("c13,DA"), runs: Some(5) } }))"###); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("acda,13asd"), exclude: Some("c13,DA"), runs: Some(5), backends: None } }))"#); } #[test] @@ -423,19 +423,19 @@ mod tests { #[test] fn queue_command_spaces() { insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=abcd,das "), - @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("abcd,das"), exclude: None, runs: None } }))"###); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("abcd,das"), exclude: None, runs: None, backends: None } }))"#); } #[test] fn queue_command_with_bors() { insta::assert_compact_debug_snapshot!(parse_queue_command("@bors try @rust-timer queue include=foo,bar"), - @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("foo,bar"), exclude: None, runs: None } }))"###); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("foo,bar"), exclude: None, runs: None, backends: None } }))"#); } #[test] fn queue_command_parameter_order() { insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=3 exclude=c,a include=b"), - @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("b"), exclude: Some("c,a"), runs: Some(3) } }))"###); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("b"), exclude: Some("c,a"), runs: Some(3), backends: None } }))"#); } #[test] @@ -446,10 +446,42 @@ Let's do a perf run quickly and then we can merge it. @bors try @rust-timer queue include=foo,bar Otherwise LGTM."#), - @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("foo,bar"), exclude: None, runs: None } }))"###); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("foo,bar"), exclude: None, runs: None, backends: None } }))"#); } fn get_build_commands(body: &str) -> Vec> { parse_build_commands(body).collect() } + + #[test] + fn build_command_with_backends() { + insta::assert_compact_debug_snapshot!(get_build_commands(r#"@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af995G"#), + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af995G", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } })]"#); + insta::assert_compact_debug_snapshot!(get_build_commands(r#"@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af995A backends=Llvm"#), + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af995A", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Llvm") } })]"#); + insta::assert_compact_debug_snapshot!(get_build_commands(r#"@rust-timer build 23936af287657fa4148aeab40cc2a780810fae5B backends=Cranelift"#), + @r#"[Ok(BuildCommand { sha: "23936af287657fa4148aeab40cc2a780810fae5B", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Cranelift") } })]"#); + insta::assert_compact_debug_snapshot!(get_build_commands(r#"@rust-timer build 23936af287657fa4148aeab40cc2a780810fae5C backends=Cranelift,Llvm"#), + @r#"[Ok(BuildCommand { sha: "23936af287657fa4148aeab40cc2a780810fae5C", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Cranelift,Llvm") } })]"#); + insta::assert_compact_debug_snapshot!(get_build_commands(r#"@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af995D include=hello backends=Llvm"#), + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af995D", params: BenchmarkParameters { include: Some("hello"), exclude: None, runs: None, backends: Some("Llvm") } })]"#); + insta::assert_compact_debug_snapshot!(get_build_commands(r#"@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af995E runs=10 backends=Llvm"#), + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af995E", params: BenchmarkParameters { include: None, exclude: None, runs: Some(10), backends: Some("Llvm") } })]"#); + } + + #[test] + fn queue_command_with_backends() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue backends=Llvm"), + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Llvm") } }))"#); + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue backends=Cranelift"), + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Cranelift") } }))"#); + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue backends=Cranelift,Llvm"), + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Cranelift,Llvm") } }))"#); + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue"), + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } }))"); + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=hello backends=Llvm"), + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("hello"), exclude: None, runs: None, backends: Some("Llvm") } }))"#); + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=hello exclude=ripgrep runs=3 backends=Llvm"), + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("hello"), exclude: Some("ripgrep"), runs: Some(3), backends: Some("Llvm") } }))"#); + } } From 4a4c01b06b451cfdd30f70d212c331f0e3539f1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Tue, 5 Nov 2024 22:03:38 +0000 Subject: [PATCH 4/5] ignore empty arguments instead of `Some("")` --- site/src/request_handlers/github.rs | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/site/src/request_handlers/github.rs b/site/src/request_handlers/github.rs index 175a41947..f2781d0cf 100644 --- a/site/src/request_handlers/github.rs +++ b/site/src/request_handlers/github.rs @@ -224,12 +224,12 @@ fn parse_benchmark_parameters<'a>( mut args: HashMap<&'a str, &'a str>, ) -> Result, String> { let mut params = BenchmarkParameters { - include: args.remove("include"), - exclude: args.remove("exclude"), + include: args.remove("include").filter(|s| !s.is_empty()), + exclude: args.remove("exclude").filter(|s| !s.is_empty()), runs: None, - backends: args.remove("backends"), + backends: args.remove("backends").filter(|s| !s.is_empty()), }; - if let Some(runs) = args.remove("runs") { + if let Some(runs) = args.remove("runs").filter(|s| !s.is_empty()) { let Ok(runs) = runs.parse::() else { return Err(format!("Cannot parse runs {runs} as a number")); }; @@ -484,4 +484,16 @@ Otherwise LGTM."#), insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=hello exclude=ripgrep runs=3 backends=Llvm"), @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("hello"), exclude: Some("ripgrep"), runs: Some(3), backends: Some("Llvm") } }))"#); } + + #[test] + fn no_empty_arguments_thank_you() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include="), + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } }))"); + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue exclude="), + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } }))"); + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs="), + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } }))"); + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue backends="), + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None } }))"); + } } From 700be8cbc632063ae6d31a28005be7ee9874425a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Fri, 15 Nov 2024 18:19:23 +0000 Subject: [PATCH 5/5] make backend selection case-insensitive --- collector/src/bin/collector.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/collector/src/bin/collector.rs b/collector/src/bin/collector.rs index dd1d151f8..2f5e7fa9e 100644 --- a/collector/src/bin/collector.rs +++ b/collector/src/bin/collector.rs @@ -968,10 +968,11 @@ fn main_result() -> anyhow::Result { // majority of cases. let mut backends = vec![]; if let Some(requested_backends) = requested_backends { - if requested_backends.contains("Llvm") { + let requested_backends = requested_backends.to_lowercase(); + if requested_backends.contains("llvm") { backends.push(CodegenBackend::Llvm); } - if requested_backends.contains("Cranelift") { + if requested_backends.contains("cranelift") { backends.push(CodegenBackend::Cranelift); } }