From 485921258c4bef232cffcc8f1a917725080ad0aa Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Thu, 11 Dec 2025 09:26:26 +0000 Subject: [PATCH 1/4] Add `target` for GitHub commands --- database/schema.md | 1 + database/src/lib.rs | 53 ++++++++++++--- database/src/pool.rs | 4 +- database/src/pool/postgres.rs | 11 ++- database/src/tests/mod.rs | 2 +- site/src/job_queue/mod.rs | 7 +- site/src/request_handlers/github.rs | 102 ++++++++++++++++++---------- 7 files changed, 127 insertions(+), 53 deletions(-) diff --git a/database/schema.md b/database/schema.md index 3aaf5cfaf..f99191006 100644 --- a/database/schema.md +++ b/database/schema.md @@ -276,6 +276,7 @@ Columns: * `completed`: Completed request. * **backends** (`text NOT NULL`): Comma-separated list of codegen backends to benchmark. If empty, the default set of codegen backends will be benchmarked. * **profiles** (`text NOT NULL`): Comma-separated list of profiles to benchmark. If empty, the default set of profiles will be benchmarked. +* **targets** (`text NOT NULL`): Comma-separated list of targets to benchmark. If empty, the default `x86_64-unknown-linux-gnu` be benchmarked. ### collector_config diff --git a/database/src/lib.rs b/database/src/lib.rs index 121acf769..2d9dbc7d7 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -383,6 +383,14 @@ impl Target { pub fn all() -> Vec { vec![Self::X86_64UnknownLinuxGnu] } + + pub fn default_targets() -> Vec { + vec![Self::X86_64UnknownLinuxGnu] + } + + pub fn is_optional(self) -> bool { + self != Target::X86_64UnknownLinuxGnu + } } impl FromStr for Target { @@ -913,6 +921,7 @@ pub struct BenchmarkRequest { status: BenchmarkRequestStatus, backends: String, profiles: String, + targets: Option, } impl BenchmarkRequest { @@ -927,11 +936,17 @@ impl BenchmarkRequest { status: BenchmarkRequestStatus::ArtifactsReady, backends: String::new(), profiles: String::new(), + targets: None, } } /// Create a try request that is in the `WaitingForArtifacts` status. - pub fn create_try_without_artifacts(pr: u32, backends: &str, profiles: &str) -> Self { + pub fn create_try_without_artifacts( + pr: u32, + backends: &str, + profiles: &str, + targets: &str, + ) -> Self { Self { commit_type: BenchmarkRequestType::Try { pr, @@ -943,6 +958,7 @@ impl BenchmarkRequest { status: BenchmarkRequestStatus::WaitingForArtifacts, backends: backends.to_string(), profiles: profiles.to_string(), + targets: Some(targets.to_string()), } } @@ -959,6 +975,7 @@ impl BenchmarkRequest { status: BenchmarkRequestStatus::ArtifactsReady, backends: String::new(), profiles: String::new(), + targets: None, } } @@ -1037,6 +1054,18 @@ impl BenchmarkRequest { parse_profiles(&self.profiles).map_err(|e| anyhow::anyhow!("{e}")) } + /// Get the targets for the request + pub fn targets(&self) -> anyhow::Result> { + if let Some(targets) = &self.targets { + if targets.trim().is_empty() { + return Ok(Target::default_targets()); + } + parse_targets(targets).map_err(|e| anyhow::anyhow!("{e}")) + } else { + Ok(Target::default_targets()) + } + } + pub fn is_completed(&self) -> bool { matches!(self.status, BenchmarkRequestStatus::Completed { .. }) } @@ -1056,18 +1085,26 @@ pub enum BenchmarkRequestInsertResult { NothingInserted, } -pub fn parse_backends(backends: &str) -> Result, String> { - backends +fn parse_raw_string(raw_string: &str, name: &str) -> Result, String> +where + T: FromStr, +{ + raw_string .split(',') - .map(|s| CodegenBackend::from_str(s).map_err(|_| format!("Invalid backend: {s}"))) + .map(|s| T::from_str(s).map_err(|_| format!("Invalid {name}: {s}"))) .collect() } +pub fn parse_backends(backends: &str) -> Result, String> { + parse_raw_string(backends, "backend") +} + pub fn parse_profiles(profiles: &str) -> Result, String> { - profiles - .split(',') - .map(|s| Profile::from_str(s).map_err(|_| format!("Invalid profile: {s}"))) - .collect() + parse_raw_string(profiles, "profile") +} + +pub fn parse_targets(targets: &str) -> Result, String> { + parse_raw_string(targets, "target") } /// Cached information about benchmark requests in the DB diff --git a/database/src/pool.rs b/database/src/pool.rs index 4e433e20f..815b10cd8 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -576,7 +576,7 @@ mod tests { // But this should fail, as we can't have two queued requests at once let result = db .insert_benchmark_request(&BenchmarkRequest::create_try_without_artifacts( - 42, "", "", + 42, "", "", "", )) .await .unwrap(); @@ -675,7 +675,7 @@ mod tests { run_postgres_test(|ctx| async { let db = ctx.db(); - let req = BenchmarkRequest::create_try_without_artifacts(42, "", ""); + let req = BenchmarkRequest::create_try_without_artifacts(42, "", "", ""); db.insert_benchmark_request(&req).await.unwrap(); assert!(db diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 2b43425d0..0b2562a2d 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -441,6 +441,7 @@ static MIGRATIONS: &[&str] = &[ ALTER TABLE runtime_pstat_series ADD CONSTRAINT runtime_test_case UNIQUE(benchmark, target, metric); "#, r#"ALTER TABLE job_queue ADD COLUMN is_optional BOOLEAN NOT NULL DEFAULT FALSE"#, + r#"ALTER TABLE benchmark_request ADD COLUMN targets TEXT NOT NULL DEFAULT 'x86_64-unknown-linux-gnu'"#, ]; #[async_trait::async_trait] @@ -801,7 +802,7 @@ impl PostgresConnection { // `tag` should be kept as the first column const BENCHMARK_REQUEST_COLUMNS: &str = - "tag, parent_sha, pr, commit_type, status, created_at, completed_at, backends, profiles, commit_date, duration_ms"; + "tag, parent_sha, pr, commit_type, status, created_at, completed_at, backends, profiles, commit_date, duration_ms, targets"; #[async_trait::async_trait] impl

Connection for P @@ -2193,8 +2194,8 @@ where for row in rows { let tag = row.get::<_, &str>(0); - let error_benchmark = row.get::<_, Option>(11); - let error_content = row.get::<_, Option>(12); + let error_benchmark = row.get::<_, Option>(12); + let error_content = row.get::<_, Option>(13); // We already saw this request, just add errors if let Some(errors) = errors.get_mut(tag) { @@ -2350,6 +2351,7 @@ fn row_to_benchmark_request(row: &Row, row_offset: Option) -> BenchmarkRe let profiles = row.get::<_, String>(8 + row_offset); let commit_date = row.get::<_, Option>>(9 + row_offset); let duration_ms = row.get::<_, Option>(10 + row_offset); + let targets = row.get::<_, Option>(11 + row_offset); let pr = pr.map(|v| v as u32); @@ -2371,6 +2373,7 @@ fn row_to_benchmark_request(row: &Row, row_offset: Option) -> BenchmarkRe status, backends, profiles, + targets, }, BENCHMARK_REQUEST_MASTER_STR => BenchmarkRequest { commit_type: BenchmarkRequestType::Master { @@ -2383,6 +2386,7 @@ fn row_to_benchmark_request(row: &Row, row_offset: Option) -> BenchmarkRe status, backends, profiles, + targets, }, BENCHMARK_REQUEST_RELEASE_STR => BenchmarkRequest { commit_type: BenchmarkRequestType::Release { @@ -2393,6 +2397,7 @@ fn row_to_benchmark_request(row: &Row, row_offset: Option) -> BenchmarkRe status, backends, profiles, + targets, }, _ => panic!("Invalid `commit_type` for `BenchmarkRequest` {commit_type}",), } diff --git a/database/src/tests/mod.rs b/database/src/tests/mod.rs index 8e1b92c77..e394637ca 100644 --- a/database/src/tests/mod.rs +++ b/database/src/tests/mod.rs @@ -153,7 +153,7 @@ impl TestContext { /// Create a new try benchmark request without artifacts and add it to the DB. pub async fn insert_try_request(&self, pr: u32) -> BenchmarkRequest { - let req = BenchmarkRequest::create_try_without_artifacts(pr, "", ""); + let req = BenchmarkRequest::create_try_without_artifacts(pr, "", "", ""); self.db().insert_benchmark_request(&req).await.unwrap(); req } diff --git a/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index 6cd5294d2..bc323083d 100644 --- a/site/src/job_queue/mod.rs +++ b/site/src/job_queue/mod.rs @@ -268,6 +268,7 @@ pub async fn enqueue_benchmark_request( let backends = request.backends()?; let profiles = request.profiles()?; + let targets = request.targets()?; #[derive(PartialEq, Debug)] enum EnqueueMode { @@ -327,12 +328,12 @@ pub async fn enqueue_benchmark_request( }; // Target x benchmark_set x backend x profile -> BenchmarkJob - for target in Target::all() { + for &target in targets.iter() { // We only have X86_64 at the moment and when we add other targets do // not want to block the Benchmark request from completing to wait on // other targets. Essentially, for the time being, other targets will // run in the background - let is_optional = target != Target::X86_64UnknownLinuxGnu; + let is_optional = target.is_optional(); for benchmark_set in 0..get_benchmark_sets_for_target(target.into()).len() { for &backend in backends.iter() { @@ -629,7 +630,7 @@ mod tests { } fn create_try(pr: u32) -> BenchmarkRequest { - BenchmarkRequest::create_try_without_artifacts(pr, "", "") + BenchmarkRequest::create_try_without_artifacts(pr, "", "", "") } fn create_release(tag: &str) -> BenchmarkRequest { diff --git a/site/src/request_handlers/github.rs b/site/src/request_handlers/github.rs index c86d53ae2..5aa1feb7f 100644 --- a/site/src/request_handlers/github.rs +++ b/site/src/request_handlers/github.rs @@ -7,8 +7,8 @@ use crate::job_queue::should_use_job_queue; use crate::load::SiteCtxt; use database::{ - parse_backends, parse_profiles, BenchmarkRequest, BenchmarkRequestInsertResult, CodegenBackend, - Profile, + parse_backends, parse_profiles, parse_targets, BenchmarkRequest, BenchmarkRequestInsertResult, + CodegenBackend, Profile, Target, }; use hashbrown::HashMap; use std::sync::Arc; @@ -97,8 +97,10 @@ async fn record_try_benchmark_request_without_artifacts( pr: u32, backends: &str, profiles: &str, + targets: &str, ) -> String { - let try_request = BenchmarkRequest::create_try_without_artifacts(pr, backends, profiles); + let try_request = + BenchmarkRequest::create_try_without_artifacts(pr, backends, profiles, targets); log::info!("Inserting try benchmark request {try_request:?}"); match conn.insert_benchmark_request(&try_request).await { @@ -157,6 +159,7 @@ async fn handle_rust_timer( issue.number, cmd.params.backends.unwrap_or(""), cmd.params.profiles.unwrap_or(""), + cmd.params.targets.unwrap_or(""), ) .await } else { @@ -206,6 +209,7 @@ async fn handle_rust_timer( issue.number, command.params.backends.unwrap_or(""), command.params.profiles.unwrap_or(""), + command.params.targets.unwrap_or(""), ) .await; } else { @@ -284,6 +288,7 @@ fn parse_benchmark_parameters<'a>( runs: None, backends: args.remove("backends").filter(|s| !s.is_empty()), profiles: args.remove("profiles").filter(|s| !s.is_empty()), + targets: args.remove("targets").filter(|s| !s.is_empty()), }; if let Some(runs) = args.remove("runs").filter(|s| !s.is_empty()) { let Ok(runs) = runs.parse::() else { @@ -291,6 +296,7 @@ fn parse_benchmark_parameters<'a>( }; params.runs = Some(runs as i32); } + if let Some(backends) = ¶ms.backends { // Make sure that the backends are correct parse_backends(backends).map_err(|e| { @@ -304,6 +310,7 @@ fn parse_benchmark_parameters<'a>( ) })?; } + if let Some(profiles) = ¶ms.profiles { // Make sure that the profiles are correct parse_profiles(profiles).map_err(|e| { @@ -318,6 +325,20 @@ fn parse_benchmark_parameters<'a>( })?; } + if let Some(targets) = ¶ms.targets { + // Make sure that the targets are correct + parse_targets(targets).map_err(|e| { + format!( + "Cannot parse targets: {e}. Valid values are: {}", + Target::all() + .iter() + .map(|b| b.as_str()) + .collect::>() + .join(", ") + ) + })?; + } + if !args.is_empty() { Err(format!( "Unknown command argument(s) `{}`", @@ -367,6 +388,7 @@ struct BenchmarkParameters<'a> { runs: Option, backends: Option<&'a str>, profiles: Option<&'a str>, + targets: Option<&'a str>, } pub async fn get_authorized_users() -> Result, String> { @@ -408,7 +430,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, backends: None, profiles: None } })]"#); + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: None } })]"#); } #[test] @@ -417,7 +439,7 @@ mod tests { @rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af9952 @rust-timer build 23936af287657fa4148aeab40cc2a780810fae52 "#), - @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None } }), Ok(BuildCommand { sha: "23936af287657fa4148aeab40cc2a780810fae52", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None } })]"#); + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: None } }), Ok(BuildCommand { sha: "23936af287657fa4148aeab40cc2a780810fae52", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: None } })]"#); } #[test] @@ -429,14 +451,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), backends: None, profiles: None } })]"#); + @r#"[Ok(BuildCommand { sha: "sha123456", params: BenchmarkParameters { include: Some("foo,bar"), exclude: Some("baz"), runs: Some(4), backends: None, profiles: None, targets: 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, backends: None, profiles: None } })]"#); + @r#"[Ok(BuildCommand { sha: "323f521bc6d8f2b966ba7838a3f3ee364e760b7e", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: None } })]"#); } #[test] @@ -452,7 +474,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, backends: None, profiles: None } }))"); + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: None } }))"); } #[test] @@ -470,19 +492,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, backends: None, profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("abcd,feih"), exclude: None, runs: None, backends: None, profiles: None, targets: 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, backends: None, profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: Some("foo134,barzbaz41baf"), runs: None, backends: None, profiles: None, targets: 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), backends: None, profiles: None } }))"); + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: Some(5), backends: None, profiles: None, targets: None } }))"); } #[test] @@ -494,7 +516,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), backends: None, profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("acda,13asd"), exclude: Some("c13,DA"), runs: Some(5), backends: None, profiles: None, targets: None } }))"#); } #[test] @@ -506,19 +528,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, backends: None, profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("abcd,das"), exclude: None, runs: None, backends: None, profiles: None, targets: 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, backends: None, profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("foo,bar"), exclude: None, runs: None, backends: None, profiles: None, targets: 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), backends: None, profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("b"), exclude: Some("c,a"), runs: Some(3), backends: None, profiles: None, targets: None } }))"#); } #[test] @@ -529,7 +551,7 @@ 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, backends: None, profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("foo,bar"), exclude: None, runs: None, backends: None, profiles: None, targets: None } }))"#); } fn get_build_commands(body: &str) -> Vec, String>> { @@ -539,58 +561,66 @@ Otherwise LGTM."#), #[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, profiles: None } })]"#); + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af995G", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: 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"), profiles: None } })]"#); + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af995A", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Llvm"), profiles: None, targets: None } })]"#); 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"), profiles: None } })]"#); + @r#"[Ok(BuildCommand { sha: "23936af287657fa4148aeab40cc2a780810fae5B", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Cranelift"), profiles: None, targets: None } })]"#); 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"), profiles: None } })]"#); + @r#"[Ok(BuildCommand { sha: "23936af287657fa4148aeab40cc2a780810fae5C", params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Cranelift,Llvm"), profiles: None, targets: None } })]"#); 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"), profiles: None } })]"#); + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af995D", params: BenchmarkParameters { include: Some("hello"), exclude: None, runs: None, backends: Some("Llvm"), profiles: None, targets: None } })]"#); 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"), profiles: None } })]"#); + @r#"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af995E", params: BenchmarkParameters { include: None, exclude: None, runs: Some(10), backends: Some("Llvm"), profiles: None, targets: None } })]"#); } #[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"), profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Llvm"), profiles: None, targets: None } }))"#); 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"), profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Cranelift"), profiles: None, targets: None } }))"#); 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"), profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Cranelift,Llvm"), profiles: None, targets: None } }))"#); insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue"), - @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None } }))"); + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: 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"), profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("hello"), exclude: None, runs: None, backends: Some("Llvm"), profiles: None, targets: None } }))"#); 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"), profiles: None } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("hello"), exclude: Some("ripgrep"), runs: Some(3), backends: Some("Llvm"), profiles: None, targets: None } }))"#); } #[test] fn queue_command_with_profiles() { insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue profiles=Doc"), - @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: Some("Doc") } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: Some("Doc"), targets: None } }))"#); insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue profiles=Check,Clippy"), - @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: Some("Check,Clippy") } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: Some("Check,Clippy"), targets: None } }))"#); insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue profiles=Doc,Clippy,Opt backends=Cranelift,Llvm"), - @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Cranelift,Llvm"), profiles: Some("Doc,Clippy,Opt") } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: Some("Cranelift,Llvm"), profiles: Some("Doc,Clippy,Opt"), targets: None } }))"#); insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue profiles=Foo"), @r#"Some(Err("Cannot parse profiles: Invalid profile: Foo. Valid values are: check, debug, opt, doc, doc-json, clippy"))"#); insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue profiles=check"), - @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: Some("check") } }))"#); + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: Some("check"), targets: None } }))"#); + } + + #[test] + fn queue_command_with_targets() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue targets=x86_64-unknown-linux-gnu"), + @r#"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: Some("x86_64-unknown-linux-gnu") } }))"#); + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue targets=x86_64-unknown-linux-gnu,67-unknown-none"), + @r#"Some(Err("Cannot parse targets: Invalid target: 67-unknown-none. Valid values are: x86_64-unknown-linux-gnu"))"#); } #[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, profiles: None } }))"); + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: 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, profiles: None } }))"); + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: 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, profiles: None } }))"); + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: 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, profiles: None } }))"); + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None, backends: None, profiles: None, targets: None } }))"); } } From 5eb02b8db984f203a898265fffd2c19e7665029f Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Fri, 12 Dec 2025 08:42:51 +0000 Subject: [PATCH 2/4] PR Feedback; correct gramma & make targets not an `Option` --- database/schema.md | 2 +- database/src/lib.rs | 27 ++++++++++++--------------- database/src/pool/postgres.rs | 2 +- 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/database/schema.md b/database/schema.md index f99191006..8bfda5cb2 100644 --- a/database/schema.md +++ b/database/schema.md @@ -276,7 +276,7 @@ Columns: * `completed`: Completed request. * **backends** (`text NOT NULL`): Comma-separated list of codegen backends to benchmark. If empty, the default set of codegen backends will be benchmarked. * **profiles** (`text NOT NULL`): Comma-separated list of profiles to benchmark. If empty, the default set of profiles will be benchmarked. -* **targets** (`text NOT NULL`): Comma-separated list of targets to benchmark. If empty, the default `x86_64-unknown-linux-gnu` be benchmarked. +* **targets** (`text NOT NULL`): Comma-separated list of targets to benchmark. If empty, the default `x86_64-unknown-linux-gnu` will be benchmarked. ### collector_config diff --git a/database/src/lib.rs b/database/src/lib.rs index 2d9dbc7d7..1216fad14 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -921,7 +921,7 @@ pub struct BenchmarkRequest { status: BenchmarkRequestStatus, backends: String, profiles: String, - targets: Option, + targets: String, } impl BenchmarkRequest { @@ -936,7 +936,7 @@ impl BenchmarkRequest { status: BenchmarkRequestStatus::ArtifactsReady, backends: String::new(), profiles: String::new(), - targets: None, + targets: String::new(), } } @@ -958,7 +958,7 @@ impl BenchmarkRequest { status: BenchmarkRequestStatus::WaitingForArtifacts, backends: backends.to_string(), profiles: profiles.to_string(), - targets: Some(targets.to_string()), + targets: targets.to_string(), } } @@ -975,7 +975,7 @@ impl BenchmarkRequest { status: BenchmarkRequestStatus::ArtifactsReady, backends: String::new(), profiles: String::new(), - targets: None, + targets: String::new(), } } @@ -1056,14 +1056,11 @@ impl BenchmarkRequest { /// Get the targets for the request pub fn targets(&self) -> anyhow::Result> { - if let Some(targets) = &self.targets { - if targets.trim().is_empty() { - return Ok(Target::default_targets()); - } - parse_targets(targets).map_err(|e| anyhow::anyhow!("{e}")) - } else { - Ok(Target::default_targets()) + if self.targets.trim().is_empty() { + return Ok(Target::default_targets()); } + + parse_targets(&self.targets).map_err(|e| anyhow::anyhow!("{e}")) } pub fn is_completed(&self) -> bool { @@ -1085,7 +1082,7 @@ pub enum BenchmarkRequestInsertResult { NothingInserted, } -fn parse_raw_string(raw_string: &str, name: &str) -> Result, String> +fn parse_comma_separated(raw_string: &str, name: &str) -> Result, String> where T: FromStr, { @@ -1096,15 +1093,15 @@ where } pub fn parse_backends(backends: &str) -> Result, String> { - parse_raw_string(backends, "backend") + parse_comma_separated(backends, "backend") } pub fn parse_profiles(profiles: &str) -> Result, String> { - parse_raw_string(profiles, "profile") + parse_comma_separated(profiles, "profile") } pub fn parse_targets(targets: &str) -> Result, String> { - parse_raw_string(targets, "target") + parse_comma_separated(targets, "target") } /// Cached information about benchmark requests in the DB diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 0b2562a2d..f3ac3fbf6 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -2351,7 +2351,7 @@ fn row_to_benchmark_request(row: &Row, row_offset: Option) -> BenchmarkRe let profiles = row.get::<_, String>(8 + row_offset); let commit_date = row.get::<_, Option>>(9 + row_offset); let duration_ms = row.get::<_, Option>(10 + row_offset); - let targets = row.get::<_, Option>(11 + row_offset); + let targets = row.get::<_, String>(11 + row_offset); let pr = pr.map(|v| v as u32); From 38ee7b52b046d905bff926c2d4569bf9aa6afbc2 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Fri, 12 Dec 2025 09:50:25 +0000 Subject: [PATCH 3/4] update help page --- site/frontend/templates/pages/help.html | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/site/frontend/templates/pages/help.html b/site/frontend/templates/pages/help.html index 4d662917c..21dd163eb 100644 --- a/site/frontend/templates/pages/help.html +++ b/site/frontend/templates/pages/help.html @@ -53,6 +53,10 @@

@rust-timer commands

If you select a non-default profile, rustc-perf will also gather data for this profile for the parent/baseline commit, so that we have something to compare to. +
  • targets=<TARGETS> configures which targets should be benchmarked. + If no targets are provided x86_64-unknown-linux-gnu is the default. + Please note presently the only valid option is x86_64-unknown-linux-gnu. +

    • @rust-timer build $commit will queue a perf run for the given commit $commit. From d1393fae598f0e2c2ab690a47fb60ed78c36e63b Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Fri, 12 Dec 2025 11:22:18 +0000 Subject: [PATCH 4/4] PR Feedback; default to empty string in database --- database/src/pool/postgres.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index f3ac3fbf6..09b32b459 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -441,7 +441,7 @@ static MIGRATIONS: &[&str] = &[ ALTER TABLE runtime_pstat_series ADD CONSTRAINT runtime_test_case UNIQUE(benchmark, target, metric); "#, r#"ALTER TABLE job_queue ADD COLUMN is_optional BOOLEAN NOT NULL DEFAULT FALSE"#, - r#"ALTER TABLE benchmark_request ADD COLUMN targets TEXT NOT NULL DEFAULT 'x86_64-unknown-linux-gnu'"#, + r#"ALTER TABLE benchmark_request ADD COLUMN targets TEXT NOT NULL DEFAULT ''"#, ]; #[async_trait::async_trait]