PR Feedback; remove create_queued

Author: Jamesbarford

database/src/lib.rs

@@ -1092,6 +1092,12 @@ impl fmt::Display for BenchmarkJobStatus {
 #[derive(Debug, Clone, PartialEq)]
 pub struct BenchmarkSet(u32);
 
+/// A single unit of work generated from a benchmark request. Split by profiles
+/// and backends
+///
+/// Each request is split into several `BenchmarkJob`s. Collectors poll the
+/// queue and claim a job only when its `benchmark_set` matches one of the sets
+/// they are responsible for.
 #[derive(Debug, Clone, PartialEq)]
 pub struct BenchmarkJob {
     target: Target,
@@ -1103,48 +1109,3 @@ pub struct BenchmarkJob {
     status: BenchmarkJobStatus,
     retry: u32,
 }
-
-impl BenchmarkJob {
-    pub fn create_queued(
-        target: Target,
-        backend: CodegenBackend,
-        profile: Profile,
-        request_tag: &str,
-        benchmark_set: u32,
-    ) -> Self {
-        BenchmarkJob {
-            target,
-            backend,
-            profile,
-            created_at: Utc::now(),
-            request_tag: request_tag.to_string(),
-            benchmark_set: BenchmarkSet(benchmark_set),
-            status: BenchmarkJobStatus::Queued,
-            retry: 0,
-        }
-    }
-
-    pub fn request_tag(&self) -> &str {
-        &self.request_tag
-    }
-
-    pub fn target(&self) -> Target {
-        self.target
-    }
-
-    pub fn backend(&self) -> CodegenBackend {
-        self.backend
-    }
-
-    pub fn profile(&self) -> Profile {
-        self.profile
-    }
-
-    pub fn benchmark_set(&self) -> u32 {
-        self.benchmark_set.0
-    }
-
-    pub fn status(&self) -> &BenchmarkJobStatus {
-        &self.status
-    }
-}

database/src/pool.rs

@@ -345,7 +345,7 @@ mod tests {
     use super::*;
     use crate::{
         tests::{run_db_test, run_postgres_test},
-        BenchmarkJob, BenchmarkRequestStatus, BenchmarkRequestType, Commit, CommitType, Date,
+        BenchmarkRequestStatus, BenchmarkRequestType, Commit, CommitType, Date,
     };
 
     /// Create a Commit
@@ -604,14 +604,6 @@ mod tests {
             let benchmark_request =
                 BenchmarkRequest::create_master("sha-1", "parent-sha-1", 42, time);
 
-            let benchmark_job = BenchmarkJob::create_queued(
-                Target::X86_64UnknownLinuxGnu,
-                CodegenBackend::Llvm,
-                Profile::Opt,
-                benchmark_request.tag().unwrap(),
-                2,
-            );
-
             // Insert the request so we don't violate the foreign key
             db.insert_benchmark_request(&benchmark_request)
                 .await
@@ -620,10 +612,10 @@ mod tests {
             // Now we can insert the job
             let result = db
                 .enqueue_benchmark_job(
-                    benchmark_job.request_tag(),
-                    &benchmark_job.target(),
-                    &benchmark_job.backend(),
-                    &benchmark_job.profile(),
+                    benchmark_request.tag().unwrap(),
+                    &Target::X86_64UnknownLinuxGnu,
+                    &CodegenBackend::Llvm,
+                    &Profile::Opt,
                     0u32,
                 )
                 .await;

site/src/job_queue/mod.rs

@@ -6,9 +6,7 @@ use crate::job_queue::utils::{parse_release_string, ExtractIf};
 use crate::load::{partition_in_place, SiteCtxt};
 use chrono::Utc;
 use collector::benchmark_set::benchmark_set_count;
-use database::{
-    BenchmarkJob, BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestStatus, Target,
-};
+use database::{BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestStatus, Target};
 use hashbrown::HashSet;
 use parking_lot::RwLock;
 use tokio::time::{self, Duration};
@@ -178,17 +176,19 @@ pub async fn build_queue(
 }
 
 /// From a benchmark_request create all the required jobs
-pub fn create_benchmark_jobs(
+pub async fn create_benchmark_jobs(
+    conn: &mut dyn database::pool::Connection,
     benchmark_request: &BenchmarkRequest,
-) -> anyhow::Result<Vec<BenchmarkJob>> {
+) -> anyhow::Result<()> {
+    let mut tx = conn.transaction().await;
     anyhow::ensure!(
         benchmark_request.tag().is_some(),
         "Benchmark request has no tag"
     );
 
     let backends = benchmark_request.backends()?;
     let profiles = benchmark_request.profiles()?;
-    let mut jobs = vec![];
+    let request_tag = benchmark_request.tag().unwrap();
 
     // Target x benchmark_set x backend x profile -> BenchmarkJob
     for target in Target::all() {
@@ -197,20 +197,40 @@ pub fn create_benchmark_jobs(
         ) {
             for backend in backends.iter() {
                 for profile in profiles.iter() {
-                    let job = BenchmarkJob::create_queued(
-                        target,
-                        *backend,
-                        *profile,
-                        benchmark_request.tag().unwrap(),
-                        benchmark_set as u32,
-                    );
-                    jobs.push(job);
+                    tx.conn()
+                        .enqueue_benchmark_job(
+                            request_tag,
+                            &target,
+                            backend,
+                            profile,
+                            benchmark_set as u32,
+                        )
+                        .await?;
+                    // If there is a parent, we create a job for it to. The
+                    // database will ignore it if there is already a job there.
+                    // If the parent job has been deleted from the database
+                    // but was already benchmarked then the collector will ignore
+                    // it as it will see it already has results.
+                    if let Some(parent_sha) = benchmark_request.parent_sha() {
+                        tx.conn()
+                            .enqueue_benchmark_job(
+                                parent_sha,
+                                &target,
+                                backend,
+                                profile,
+                                benchmark_set as u32,
+                            )
+                            .await?;
+                    }
                 }
             }
         }
     }
 
-    Ok(jobs)
+    tx.conn()
+        .update_benchmark_request_status(request_tag, BenchmarkRequestStatus::InProgress)
+        .await?;
+    Ok(())
 }
 
 /// Enqueue the job into the job_queue
@@ -219,30 +239,12 @@ async fn enqueue_next_job(
     index: &mut BenchmarkRequestIndex,
 ) -> anyhow::Result<()> {
     let queue = build_queue(conn, index).await?;
-    let mut tx = conn.transaction().await;
     for request in queue {
         if request.status() != BenchmarkRequestStatus::InProgress {
-            for benchmark_job in create_benchmark_jobs(&request)? {
-                tx.conn()
-                    .enqueue_benchmark_job(
-                        benchmark_job.request_tag(),
-                        &benchmark_job.target(),
-                        &benchmark_job.backend(),
-                        &benchmark_job.profile(),
-                        benchmark_job.benchmark_set(),
-                    )
-                    .await?;
-            }
-            tx.conn()
-                .update_benchmark_request_status(
-                    request.tag().unwrap(),
-                    BenchmarkRequestStatus::InProgress,
-                )
-                .await?;
+            create_benchmark_jobs(conn, &request).await?;
             break;
         }
     }
-    tx.commit().await?;
     Ok(())
 }
 
@@ -282,7 +284,7 @@ pub async fn cron_main(site_ctxt: Arc<RwLock<Option<Arc<SiteCtxt>>>>, seconds: u
 mod tests {
     use super::*;
     use chrono::{Datelike, Duration, TimeZone, Utc};
-    use database::{tests::run_postgres_test, CodegenBackend, Profile};
+    use database::tests::run_postgres_test;
 
     fn days_ago(day_str: &str) -> chrono::DateTime<Utc> {
         // Walk backwards until the first non-digit, then slice
@@ -346,18 +348,6 @@ mod tests {
         assert_eq!(queue_shas, expected)
     }
 
-    fn benchmark_jobs_match(jobs: &[BenchmarkJob], expected_jobs: &[BenchmarkJob]) {
-        assert_eq!(jobs.len(), expected_jobs.len());
-        for (job, expected) in std::iter::zip(jobs, expected_jobs) {
-            assert_eq!(job.target(), expected.target());
-            assert_eq!(job.backend(), expected.backend());
-            assert_eq!(job.profile(), expected.profile());
-            assert_eq!(job.benchmark_set(), expected.benchmark_set());
-            assert_eq!(job.request_tag(), expected.request_tag());
-            assert_eq!(job.status(), expected.status());
-        }
-    }
-
     #[tokio::test]
     async fn queue_ordering() {
         run_postgres_test(|ctx| async {
@@ -456,30 +446,4 @@ mod tests {
         })
         .await;
     }
-
-    #[test]
-    fn create_benchmark_jobs_default() {
-        let request = create_master("bar", "parent1", 10, "days2");
-        let jobs = create_benchmark_jobs(&request).unwrap();
-
-        let create_job = |profile: Profile| -> BenchmarkJob {
-            BenchmarkJob::create_queued(
-                Target::X86_64UnknownLinuxGnu,
-                CodegenBackend::Llvm,
-                profile,
-                request.tag().unwrap(),
-                0u32,
-            )
-        };
-
-        benchmark_jobs_match(
-            &vec![
-                create_job(Profile::Check),
-                create_job(Profile::Debug),
-                create_job(Profile::Doc),
-                create_job(Profile::Opt),
-            ],
-            &jobs,
-        );
-    }
 }