From 084675fb7bef05a83acd3ab7e813a97fdf43e768 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Mon, 8 Dec 2025 11:04:17 +0000 Subject: [PATCH 1/3] Allow for optional jobs not interfering which benchmark completion --- database/src/lib.rs | 5 ++ database/src/pool.rs | 100 ++++++++++++++++++++++++++++++++++ database/src/pool/postgres.rs | 25 ++++++--- database/src/pool/sqlite.rs | 1 + database/src/tests/builder.rs | 8 +++ site/src/job_queue/mod.rs | 24 +++++++- 6 files changed, 153 insertions(+), 10 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index 12cc2f808..121acf769 100644 --- a/database/src/lib.rs +++ b/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 diff --git a/database/src/pool.rs b/database/src/pool.rs index a3b2d7b7e..b1eb23fb3 100644 --- a/database/src/pool.rs +++ b/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 */ + std::thread::sleep(Duration::from_millis(1000)); + + /* 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; + } } diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 173d763c7..1ca47c151 100644 --- a/database/src/pool/postgres.rs +++ b/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,11 +1989,12 @@ 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. Also prefer + -- mandatory jobs over optional ones. CASE - WHEN status = $5 THEN 0 - WHEN status = $1 THEN 1 + WHEN (status = $5 AND is_optional = FALSE) THEN 0 + WHEN (status = $1 AND is_optional = FALSE) THEN 1 ELSE 2 END, request_tag, @@ -2019,6 +2024,7 @@ where updated.started_at, updated.retry, updated.kind, + updated.is_optional, br.commit_type, br.commit_date FROM updated @@ -2054,9 +2060,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>>(9); + let commit_type = row.get::<_, &str>(9); + let commit_date = row.get::<_, Option>>(10); let commit_date = Date(commit_date.ok_or_else(|| { anyhow::anyhow!("Dequeuing job for a benchmark request without commit date") @@ -2114,6 +2121,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 +2272,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()) diff --git a/database/src/pool/sqlite.rs b/database/src/pool/sqlite.rs index f61079da2..028507146 100644 --- a/database/src/pool/sqlite.rs +++ b/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!() } diff --git a/database/src/tests/builder.rs b/database/src/tests/builder.rs index 5087fee5e..961c37d98 100644 --- a/database/src/tests/builder.rs +++ b/database/src/tests/builder.rs @@ -49,6 +49,7 @@ impl RequestBuilder { job.profile, job.benchmark_set, job.kind, + false, ) .await { @@ -116,6 +117,7 @@ pub struct JobBuilder { benchmark_set: u32, conclusion: BenchmarkJobConclusion, kind: BenchmarkJobKind, + is_optional: bool, } impl JobBuilder { @@ -123,6 +125,11 @@ impl JobBuilder { self.profile = profile; self } + + pub fn is_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, } } } diff --git a/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index 436d13a72..6cd5294d2 100644 --- a/site/src/job_queue/mod.rs +++ b/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 { From 35a3aebd028e738a01ad6e267d7af1ced556811d Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Tue, 9 Dec 2025 10:04:42 +0000 Subject: [PATCH 2/3] PR Feedback: rename builder method, revert query and change sleep function in test --- database/src/pool.rs | 2 +- database/src/pool/postgres.rs | 4 ++-- database/src/tests/builder.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/database/src/pool.rs b/database/src/pool.rs index b1eb23fb3..4e433e20f 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -1350,7 +1350,7 @@ mod tests { assert_eq!(job.request_tag(), benchmark_request.tag().unwrap()); /* Make the job take some amount of time */ - std::thread::sleep(Duration::from_millis(1000)); + tokio::time::sleep(Duration::from_millis(1000)).await; /* Mark the job as complete */ db.mark_benchmark_job_as_completed(job.id(), BenchmarkJobConclusion::Success) diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 1ca47c151..fac1fa9e0 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1993,8 +1993,8 @@ where -- previously, so that we can finish them. Also prefer -- mandatory jobs over optional ones. CASE - WHEN (status = $5 AND is_optional = FALSE) THEN 0 - WHEN (status = $1 AND is_optional = FALSE) THEN 1 + WHEN status = $5 THEN 0 + WHEN status = $1 THEN 1 ELSE 2 END, request_tag, diff --git a/database/src/tests/builder.rs b/database/src/tests/builder.rs index 961c37d98..52b2fc956 100644 --- a/database/src/tests/builder.rs +++ b/database/src/tests/builder.rs @@ -126,7 +126,7 @@ impl JobBuilder { self } - pub fn is_optional(mut self, is_optional: bool) -> Self { + pub fn make_optional(mut self, is_optional: bool) -> Self { self.is_optional = is_optional; self } From 2cd7234e783e7d62570d5dea80e2bd251eb27d17 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Tue, 9 Dec 2025 10:40:17 +0000 Subject: [PATCH 3/3] PR Feedback: revert comment --- database/src/pool/postgres.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index fac1fa9e0..2b43425d0 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1990,8 +1990,7 @@ where AND benchmark_set = $3 ORDER BY -- Prefer in-progress jobs that have not been finished - -- previously, so that we can finish them. Also prefer - -- mandatory jobs over optional ones. + -- previously, so that we can finish them. CASE WHEN status = $5 THEN 0 WHEN status = $1 THEN 1