Merge pull request #2342 from Jamesbarford/feat/optional-benchmark-jobs

Allow for optional jobs not interfering which benchmark completion

Author: Kobzol

database/src/lib.rs

@@ -1164,6 +1164,7 @@ pub struct BenchmarkJob {
     status: BenchmarkJobStatus,
     deque_counter: u32,
     kind: BenchmarkJobKind,
+    is_optional: bool,
 }
 
 impl BenchmarkJob {
@@ -1215,6 +1216,10 @@ impl BenchmarkJob {
     pub fn kind(&self) -> BenchmarkJobKind {
         self.kind
     }
+
+    pub fn is_optional(&self) -> bool {
+        self.is_optional
+    }
 }
 
 /// Describes the final state of a job

database/src/pool.rs

@@ -241,6 +241,7 @@ pub trait Connection: Send + Sync {
 
     /// Add a benchmark job to the job queue and returns its ID, if it was not
     /// already in the DB previously.
+    #[allow(clippy::too_many_arguments)]
     async fn enqueue_benchmark_job(
         &self,
         request_tag: &str,
@@ -249,6 +250,7 @@ pub trait Connection: Send + Sync {
         profile: Profile,
         benchmark_set: u32,
         kind: BenchmarkJobKind,
+        is_optional: bool,
     ) -> JobEnqueueResult;
 
     /// Returns a set of compile-time benchmark test cases that were already computed for the
@@ -747,6 +749,7 @@ mod tests {
                     Profile::Opt,
                     0u32,
                     BenchmarkJobKind::Runtime,
+                    false,
                 )
                 .await;
             match result {
@@ -906,6 +909,7 @@ mod tests {
                     Profile::Opt,
                     1u32,
                     BenchmarkJobKind::Runtime,
+                    false,
                 )
                 .await
             {
@@ -1006,6 +1010,7 @@ mod tests {
                 Profile::Opt,
                 benchmark_set.0,
                 BenchmarkJobKind::Runtime,
+                false,
             )
             .await
             .unwrap();
@@ -1260,6 +1265,7 @@ mod tests {
                 Profile::Check,
                 0,
                 BenchmarkJobKind::Compiletime,
+                false,
             )
             .await
             .unwrap();
@@ -1286,4 +1292,98 @@ mod tests {
         })
         .await;
     }
+
+    #[tokio::test]
+    async fn optional_jobs_should_not_block_benchmark_request_from_being_completed() {
+        run_postgres_test(|ctx| async {
+            let db = ctx.db();
+            let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap();
+            let benchmark_set = BenchmarkSet(0u32);
+            let tag = "sha-1";
+            let collector_name = "collector-1";
+            let target = Target::X86_64UnknownLinuxGnu;
+
+            let insert_result = db
+                .add_collector_config(collector_name, target, 1, true)
+                .await;
+            assert!(insert_result.is_ok());
+
+            /* Create the request */
+            let benchmark_request = BenchmarkRequest::create_release(tag, time);
+            db.insert_benchmark_request(&benchmark_request)
+                .await
+                .unwrap();
+
+            /* Create job for the request */
+            db.enqueue_benchmark_job(
+                benchmark_request.tag().unwrap(),
+                target,
+                CodegenBackend::Llvm,
+                Profile::Opt,
+                benchmark_set.0,
+                BenchmarkJobKind::Runtime,
+                false,
+            )
+            .await
+            .unwrap();
+
+            /* Create optional job for the request, this is somewhat unrealsitic
+             * as we're only going to make specific targets jobs optional */
+            db.enqueue_benchmark_job(
+                benchmark_request.tag().unwrap(),
+                target,
+                CodegenBackend::Llvm,
+                Profile::Doc,
+                benchmark_set.0,
+                BenchmarkJobKind::Runtime,
+                true,
+            )
+            .await
+            .unwrap();
+
+            let (job, _) = db
+                .dequeue_benchmark_job(collector_name, target, benchmark_set)
+                .await
+                .unwrap()
+                .unwrap();
+
+            assert_eq!(job.request_tag(), benchmark_request.tag().unwrap());
+
+            /* Make the job take some amount of time */
+            tokio::time::sleep(Duration::from_millis(1000)).await;
+
+            /* Mark the job as complete */
+            db.mark_benchmark_job_as_completed(job.id(), BenchmarkJobConclusion::Success)
+                .await
+                .unwrap();
+
+            db.maybe_mark_benchmark_request_as_completed(tag)
+                .await
+                .unwrap();
+
+            /* From the status page view we can see that the duration has been
+             * updated. Albeit that it will be a very short duration. */
+            let completed = db.get_last_n_completed_benchmark_requests(1).await.unwrap();
+            let req = &completed
+                .iter()
+                .find(|it| it.request.tag() == Some(tag))
+                .unwrap()
+                .request;
+
+            assert!(matches!(
+                req.status(),
+                BenchmarkRequestStatus::Completed { .. }
+            ));
+            let BenchmarkRequestStatus::Completed { duration, .. } = req.status() else {
+                unreachable!();
+            };
+            assert!(duration >= Duration::from_millis(1000));
+
+            let completed_index = db.load_benchmark_request_index().await.unwrap();
+            assert!(completed_index.contains_tag("sha-1"));
+
+            Ok(ctx)
+        })
+        .await;
+    }
 }

database/src/pool/postgres.rs

@@ -440,6 +440,7 @@ static MIGRATIONS: &[&str] = &[
     ALTER TABLE runtime_pstat_series DROP CONSTRAINT runtime_pstat_series_benchmark_metric_key;
     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"#,
 ];
 
 #[async_trait::async_trait]
@@ -1791,6 +1792,7 @@ where
         profile: Profile,
         benchmark_set: u32,
         kind: BenchmarkJobKind,
+        is_optional: bool,
     ) -> JobEnqueueResult {
         // This will return zero rows if the job already exists
         let result = self
@@ -1804,9 +1806,10 @@ where
                 profile,
                 benchmark_set,
                 status,
-                kind
+                kind,
+                is_optional
             )
-            VALUES ($1, $2, $3, $4, $5, $6, $7)
+            VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
             ON CONFLICT DO NOTHING
             RETURNING job_queue.id
                 "#,
@@ -1818,6 +1821,7 @@ where
                     &(benchmark_set as i32),
                     &BENCHMARK_JOB_STATUS_QUEUED_STR,
                     &kind,
+                    &is_optional,
                 ],
             )
             .await;
@@ -1985,8 +1989,8 @@ where
                         AND target = $2
                         AND benchmark_set = $3
                     ORDER BY
-                        -- Prefer in-progress jobs that have not been finished previously, so that
-                        -- we can finish them.
+                        -- Prefer in-progress jobs that have not been finished
+                        -- previously, so that we can finish them.
                         CASE
                             WHEN status = $5 THEN 0
                             WHEN status = $1 THEN 1
@@ -2019,6 +2023,7 @@ where
                     updated.started_at,
                     updated.retry,
                     updated.kind,
+                    updated.is_optional,
                     br.commit_type,
                     br.commit_date
                 FROM updated
@@ -2054,9 +2059,10 @@ where
                     deque_counter: row.get::<_, i32>(6) as u32,
                     kind: BenchmarkJobKind::from_str(row.get::<_, &str>(7))
                         .map_err(|e| anyhow::anyhow!(e))?,
+                    is_optional: row.get::<_, bool>(8),
                 };
-                let commit_type = row.get::<_, &str>(8);
-                let commit_date = row.get::<_, Option<DateTime<Utc>>>(9);
+                let commit_type = row.get::<_, &str>(9);
+                let commit_date = row.get::<_, Option<DateTime<Utc>>>(10);
 
                 let commit_date = Date(commit_date.ok_or_else(|| {
                     anyhow::anyhow!("Dequeuing job for a benchmark request without commit date")
@@ -2114,6 +2120,7 @@ where
                         WHERE
                             job_queue.request_tag = benchmark_request.tag
                             AND job_queue.status NOT IN ($3, $4)
+                            AND job_queue.is_optional = FALSE
                     )
                     AND (
                         benchmark_request.parent_sha IS NULL
@@ -2264,6 +2271,7 @@ where
                 deque_counter: row.get::<_, i32>(10) as u32,
                 kind: BenchmarkJobKind::from_str(row.get::<_, &str>(12))
                     .map_err(|e| anyhow::anyhow!(e))?,
+                is_optional: row.get::<_, bool>(13),
             };
             request_to_jobs
                 .entry(job.request_tag.clone())

database/src/pool/sqlite.rs

@@ -1359,6 +1359,7 @@ impl Connection for SqliteConnection {
         _profile: Profile,
         _benchmark_set: u32,
         _kind: BenchmarkJobKind,
+        _is_optional: bool,
     ) -> JobEnqueueResult {
         no_queue_implementation_abort!()
     }

database/src/tests/builder.rs

@@ -49,6 +49,7 @@ impl RequestBuilder {
                     job.profile,
                     job.benchmark_set,
                     job.kind,
+                    false,
                 )
                 .await
             {
@@ -116,13 +117,19 @@ pub struct JobBuilder {
     benchmark_set: u32,
     conclusion: BenchmarkJobConclusion,
     kind: BenchmarkJobKind,
+    is_optional: bool,
 }
 
 impl JobBuilder {
     pub fn profile(mut self, profile: Profile) -> Self {
         self.profile = profile;
         self
     }
+
+    pub fn make_optional(mut self, is_optional: bool) -> Self {
+        self.is_optional = is_optional;
+        self
+    }
 }
 
 impl Default for JobBuilder {
@@ -134,6 +141,7 @@ impl Default for JobBuilder {
             benchmark_set: 0,
             conclusion: BenchmarkJobConclusion::Success,
             kind: BenchmarkJobKind::Compiletime,
+            is_optional: false,
         }
     }
 }

site/src/job_queue/mod.rs

@@ -282,10 +282,19 @@ pub async fn enqueue_benchmark_request(
                              profile,
                              benchmark_set,
                              kind,
-                             mode: EnqueueMode| {
+                             mode: EnqueueMode,
+                             is_optional: bool| {
         let result = tx
             .conn()
-            .enqueue_benchmark_job(request_tag, target, backend, profile, benchmark_set, kind)
+            .enqueue_benchmark_job(
+                request_tag,
+                target,
+                backend,
+                profile,
+                benchmark_set,
+                kind,
+                is_optional,
+            )
             .await;
 
         let make_error = |msg| {
@@ -319,6 +328,12 @@ pub async fn enqueue_benchmark_request(
 
     // Target x benchmark_set x backend x profile -> BenchmarkJob
     for target in Target::all() {
+        // 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;
+
         for benchmark_set in 0..get_benchmark_sets_for_target(target.into()).len() {
             for &backend in backends.iter() {
                 for &profile in profiles.iter() {
@@ -331,6 +346,7 @@ pub async fn enqueue_benchmark_request(
                         benchmark_set as u32,
                         BenchmarkJobKind::Compiletime,
                         EnqueueMode::Commit,
+                        is_optional,
                     )
                     .await?;
 
@@ -354,6 +370,7 @@ pub async fn enqueue_benchmark_request(
                             benchmark_set as u32,
                             BenchmarkJobKind::Compiletime,
                             EnqueueMode::Parent,
+                            is_optional,
                         )
                         .await?;
                     }
@@ -372,6 +389,7 @@ pub async fn enqueue_benchmark_request(
             BENCHMARK_SET_RUNTIME_BENCHMARKS,
             BenchmarkJobKind::Runtime,
             EnqueueMode::Commit,
+            is_optional,
         )
         .await?;
     }
@@ -389,6 +407,7 @@ pub async fn enqueue_benchmark_request(
                 BENCHMARK_SET_RUSTC,
                 BenchmarkJobKind::Rustc,
                 EnqueueMode::Commit,
+                false,
             )
             .await?;
         }
@@ -642,6 +661,7 @@ mod tests {
                 Profile::Opt,
                 benchmark_set,
                 BenchmarkJobKind::Compiletime,
+                false,
             )
             .await
         {