Merge pull request #2352 from Jamesbarford/feat/allow-target-selection

Add `target` for GitHub commands

Author: Kobzol

database/schema.md

@@ -268,6 +268,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` will be benchmarked.
 
 ### collector_config
 

database/src/lib.rs

@@ -383,6 +383,14 @@ impl Target {
     pub fn all() -> Vec<Self> {
         vec![Self::X86_64UnknownLinuxGnu]
     }
+
+    pub fn default_targets() -> Vec<Self> {
+        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: String,
 }
 
 impl BenchmarkRequest {
@@ -927,11 +936,17 @@ impl BenchmarkRequest {
             status: BenchmarkRequestStatus::ArtifactsReady,
             backends: String::new(),
             profiles: String::new(),
+            targets: String::new(),
         }
     }
 
     /// 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: targets.to_string(),
         }
     }
 
@@ -959,6 +975,7 @@ impl BenchmarkRequest {
             status: BenchmarkRequestStatus::ArtifactsReady,
             backends: String::new(),
             profiles: String::new(),
+            targets: String::new(),
         }
     }
 
@@ -1037,6 +1054,15 @@ impl BenchmarkRequest {
         parse_profiles(&self.profiles).map_err(|e| anyhow::anyhow!("{e}"))
     }
 
+    /// Get the targets for the request
+    pub fn targets(&self) -> anyhow::Result<Vec<Target>> {
+        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 {
         matches!(self.status, BenchmarkRequestStatus::Completed { .. })
     }
@@ -1056,18 +1082,26 @@ pub enum BenchmarkRequestInsertResult {
     NothingInserted,
 }
 
-pub fn parse_backends(backends: &str) -> Result<Vec<CodegenBackend>, String> {
-    backends
+fn parse_comma_separated<T>(raw_string: &str, name: &str) -> Result<Vec<T>, 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<Vec<CodegenBackend>, String> {
+    parse_comma_separated(backends, "backend")
+}
+
 pub fn parse_profiles(profiles: &str) -> Result<Vec<Profile>, String> {
-    profiles
-        .split(',')
-        .map(|s| Profile::from_str(s).map_err(|_| format!("Invalid profile: {s}")))
-        .collect()
+    parse_comma_separated(profiles, "profile")
+}
+
+pub fn parse_targets(targets: &str) -> Result<Vec<Target>, String> {
+    parse_comma_separated(targets, "target")
 }
 
 /// Cached information about benchmark requests in the DB

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

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 ''"#,
 ];
 
 #[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<P> Connection for P
@@ -2193,8 +2194,8 @@ where
 
         for row in rows {
             let tag = row.get::<_, &str>(0);
-            let error_benchmark = row.get::<_, Option<String>>(11);
-            let error_content = row.get::<_, Option<String>>(12);
+            let error_benchmark = row.get::<_, Option<String>>(12);
+            let error_content = row.get::<_, Option<String>>(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<usize>) -> BenchmarkRe
     let profiles = row.get::<_, String>(8 + row_offset);
     let commit_date = row.get::<_, Option<DateTime<Utc>>>(9 + row_offset);
     let duration_ms = row.get::<_, Option<i32>>(10 + row_offset);
+    let targets = row.get::<_, String>(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<usize>) -> 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<usize>) -> 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<usize>) -> BenchmarkRe
             status,
             backends,
             profiles,
+            targets,
         },
         _ => panic!("Invalid `commit_type` for `BenchmarkRequest` {commit_type}",),
     }

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
     }

site/frontend/templates/pages/help.html

@@ -53,6 +53,10 @@ <h3><b><code>@rust-timer</code> commands</b></h3>
       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.
     </li>
+    <li><code>targets=&lt;TARGETS&gt;</code> configures which targets should be benchmarked.
+        If no targets are provided <code>x86_64-unknown-linux-gnu</code> is the default.
+        Please note presently the only valid option is <code>x86_64-unknown-linux-gnu</code>.
+    </li>
   </ul>
   <p><code>@rust-timer build $commit</code> will queue a perf run for the given commit
     <code>$commit</code>.

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 {