From e3247ac333560ecdfea9aec82ac79d40672b154c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 11:39:59 +0200 Subject: [PATCH 01/14] Use constants for benchmark request status strings --- database/src/lib.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index 1fa9b2350..84ec6c0b8 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -805,13 +805,18 @@ pub enum BenchmarkRequestStatus { Completed, } +const WAITING_FOR_ARTIFACTS_STR: &str = "waiting_for_artifacts"; +const ARTIFACTS_READY_STR: &str = "artifacts_ready"; +const IN_PROGRESS_STR: &str = "in_progress"; +const COMPLETED_STR: &str = "completed"; + impl BenchmarkRequestStatus { pub fn as_str(&self) -> &str { match self { - BenchmarkRequestStatus::WaitingForArtifacts => "waiting_for_artifacts", - BenchmarkRequestStatus::ArtifactsReady => "artifacts_ready", - BenchmarkRequestStatus::InProgress => "in_progress", - BenchmarkRequestStatus::Completed => "completed", + BenchmarkRequestStatus::WaitingForArtifacts => WAITING_FOR_ARTIFACTS_STR, + BenchmarkRequestStatus::ArtifactsReady => ARTIFACTS_READY_STR, + BenchmarkRequestStatus::InProgress => IN_PROGRESS_STR, + BenchmarkRequestStatus::Completed { .. } => COMPLETED_STR, } } } @@ -831,10 +836,10 @@ impl<'a> tokio_postgres::types::FromSql<'a> for BenchmarkRequestStatus { let s: &str = <&str as tokio_postgres::types::FromSql>::from_sql(ty, raw)?; match s { - x if x == Self::WaitingForArtifacts.as_str() => Ok(Self::WaitingForArtifacts), - x if x == Self::ArtifactsReady.as_str() => Ok(Self::ArtifactsReady), - x if x == Self::InProgress.as_str() => Ok(Self::InProgress), - x if x == Self::Completed.as_str() => Ok(Self::Completed), + WAITING_FOR_ARTIFACTS_STR => Ok(Self::WaitingForArtifacts), + ARTIFACTS_READY_STR => Ok(Self::ArtifactsReady), + IN_PROGRESS_STR => Ok(Self::InProgress), + COMPLETED_STR => Ok(Self::Completed), other => Err(format!("unknown benchmark_request_status '{other}'").into()), } } From f0d1077e096c832e0e1ed4bce2f0f26c47a708d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 11:59:32 +0200 Subject: [PATCH 02/14] Fix loading of `tag`, it may be `NULL` now --- database/src/pool/postgres.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 7db2291b0..60036d51d 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1452,7 +1452,7 @@ where let benchmark_requests = rows .iter() .map(|row| { - let tag = row.get::<_, &str>(0); + let tag = row.get::<_, Option<&str>>(0); let parent_sha = row.get::<_, Option<&str>>(1); let pr = row.get::<_, Option>(2); let commit_type = row.get::<_, &str>(3); @@ -1465,7 +1465,7 @@ where match commit_type { "try" => { let mut try_benchmark = BenchmarkRequest::create_try( - Some(tag), + tag, parent_sha, pr.unwrap() as u32, created_at, @@ -1478,7 +1478,7 @@ where } "master" => { let mut master_benchmark = BenchmarkRequest::create_master( - tag, + tag.expect("Master commit in DB without SHA"), parent_sha.unwrap(), pr.unwrap() as u32, created_at, @@ -1491,7 +1491,11 @@ where } "release" => { let mut release_benchmark = BenchmarkRequest::create_release( - tag, created_at, status, backends, profiles, + tag.expect("Release commit in DB witohut SHA"), + created_at, + status, + backends, + profiles, ); release_benchmark.completed_at = completed_at; release_benchmark From 723deb520bd3504d1fa67ddb6eb5803b49657539 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 12:03:11 +0200 Subject: [PATCH 03/14] Add constants for benchmark request type strings --- database/src/lib.rs | 10 +++++++--- database/src/pool/postgres.rs | 7 ++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index 84ec6c0b8..80737b6b3 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -849,6 +849,10 @@ impl<'a> tokio_postgres::types::FromSql<'a> for BenchmarkRequestStatus { } } +const BENCHMARK_REQUEST_TRY_STR: &str = "try"; +const BENCHMARK_REQUEST_MASTER_STR: &str = "master"; +const BENCHMARK_REQUEST_RELEASE_STR: &str = "release"; + #[derive(Debug, Clone, PartialEq)] pub enum BenchmarkRequestType { /// A Try commit @@ -870,9 +874,9 @@ pub enum BenchmarkRequestType { impl fmt::Display for BenchmarkRequestType { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - BenchmarkRequestType::Try { .. } => write!(f, "try"), - BenchmarkRequestType::Master { .. } => write!(f, "master"), - BenchmarkRequestType::Release { .. } => write!(f, "release"), + BenchmarkRequestType::Try { .. } => write!(f, "{BENCHMARK_REQUEST_TRY_STR}"), + BenchmarkRequestType::Master { .. } => write!(f, "{BENCHMARK_REQUEST_MASTER_STR}"), + BenchmarkRequestType::Release { .. } => write!(f, "{BENCHMARK_REQUEST_RELEASE_STR}"), } } } diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 60036d51d..ad99556a2 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -3,6 +3,7 @@ use crate::{ ArtifactCollection, ArtifactId, ArtifactIdNumber, Benchmark, BenchmarkRequest, BenchmarkRequestStatus, BenchmarkRequestType, CodegenBackend, CollectionId, Commit, CommitType, CompileBenchmark, Date, Index, Profile, QueuedCommit, Scenario, Target, + BENCHMARK_REQUEST_MASTER_STR, BENCHMARK_REQUEST_RELEASE_STR, BENCHMARK_REQUEST_TRY_STR, }; use anyhow::Context as _; use chrono::{DateTime, TimeZone, Utc}; @@ -1463,7 +1464,7 @@ where let profiles = row.get::<_, &str>(8); match commit_type { - "try" => { + BENCHMARK_REQUEST_TRY_STR => { let mut try_benchmark = BenchmarkRequest::create_try( tag, parent_sha, @@ -1476,7 +1477,7 @@ where try_benchmark.completed_at = completed_at; try_benchmark } - "master" => { + BENCHMARK_REQUEST_MASTER_STR => { let mut master_benchmark = BenchmarkRequest::create_master( tag.expect("Master commit in DB without SHA"), parent_sha.unwrap(), @@ -1489,7 +1490,7 @@ where master_benchmark.completed_at = completed_at; master_benchmark } - "release" => { + BENCHMARK_REQUEST_RELEASE_STR => { let mut release_benchmark = BenchmarkRequest::create_release( tag.expect("Release commit in DB witohut SHA"), created_at, From 8d404d4f6e25d944137467a1575de31acd396bc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 12:10:04 +0200 Subject: [PATCH 04/14] Make it impossible to not assign completed date for completed benchmark requests Also create benchmark requests with a struct literal in the Postgres code, to make it impossible to forget setting some fields. --- database/src/lib.rs | 67 ++++++++++--------- database/src/pool.rs | 4 -- database/src/pool/postgres.rs | 120 +++++++++++++++++----------------- site/src/job_queue.rs | 4 -- 4 files changed, 93 insertions(+), 102 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index 80737b6b3..5eb066378 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -309,6 +309,7 @@ impl Scenario { } } +use anyhow::anyhow; use std::cmp::Ordering; use std::str::FromStr; @@ -802,7 +803,7 @@ pub enum BenchmarkRequestStatus { WaitingForArtifacts, ArtifactsReady, InProgress, - Completed, + Completed { completed_at: DateTime }, } const WAITING_FOR_ARTIFACTS_STR: &str = "waiting_for_artifacts"; @@ -811,44 +812,48 @@ const IN_PROGRESS_STR: &str = "in_progress"; const COMPLETED_STR: &str = "completed"; impl BenchmarkRequestStatus { - pub fn as_str(&self) -> &str { + pub(crate) fn as_str(&self) -> &str { match self { - BenchmarkRequestStatus::WaitingForArtifacts => WAITING_FOR_ARTIFACTS_STR, - BenchmarkRequestStatus::ArtifactsReady => ARTIFACTS_READY_STR, - BenchmarkRequestStatus::InProgress => IN_PROGRESS_STR, - BenchmarkRequestStatus::Completed { .. } => COMPLETED_STR, + Self::WaitingForArtifacts => WAITING_FOR_ARTIFACTS_STR, + Self::ArtifactsReady => ARTIFACTS_READY_STR, + Self::InProgress => IN_PROGRESS_STR, + Self::Completed { .. } => COMPLETED_STR, } } -} - -impl fmt::Display for BenchmarkRequestStatus { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(self.as_str()) - } -} - -impl<'a> tokio_postgres::types::FromSql<'a> for BenchmarkRequestStatus { - fn from_sql( - ty: &tokio_postgres::types::Type, - raw: &'a [u8], - ) -> Result> { - // Decode raw bytes into &str with Postgres' own text codec - let s: &str = <&str as tokio_postgres::types::FromSql>::from_sql(ty, raw)?; - match s { + pub(crate) fn from_str_and_completion_date( + text: &str, + completion_date: Option>, + ) -> anyhow::Result { + match text { WAITING_FOR_ARTIFACTS_STR => Ok(Self::WaitingForArtifacts), ARTIFACTS_READY_STR => Ok(Self::ArtifactsReady), IN_PROGRESS_STR => Ok(Self::InProgress), - COMPLETED_STR => Ok(Self::Completed), - other => Err(format!("unknown benchmark_request_status '{other}'").into()), + COMPLETED_STR => Ok(Self::Completed { + completed_at: completion_date.ok_or_else(|| { + anyhow!("No completion date for a completed BenchmarkRequestStatus") + })?, + }), + _ => Err(anyhow!("Unknown BenchmarkRequestStatus `{text}`")), } } - fn accepts(ty: &tokio_postgres::types::Type) -> bool { - <&str as tokio_postgres::types::FromSql>::accepts(ty) + pub(crate) fn completed_at(&self) -> Option> { + match self { + Self::Completed { completed_at } => Some(*completed_at), + _ => None, + } } } +impl fmt::Display for BenchmarkRequestStatus { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.as_str()) + } +} + + + const BENCHMARK_REQUEST_TRY_STR: &str = "try"; const BENCHMARK_REQUEST_MASTER_STR: &str = "master"; const BENCHMARK_REQUEST_RELEASE_STR: &str = "release"; @@ -885,7 +890,6 @@ impl fmt::Display for BenchmarkRequestType { pub struct BenchmarkRequest { pub commit_type: BenchmarkRequestType, pub created_at: DateTime, - pub completed_at: Option>, pub status: BenchmarkRequestStatus, pub backends: String, pub profiles: String, @@ -896,18 +900,15 @@ impl BenchmarkRequest { tag: &str, created_at: DateTime, status: BenchmarkRequestStatus, - backends: &str, - profiles: &str, ) -> Self { Self { commit_type: BenchmarkRequestType::Release { tag: tag.to_string(), }, created_at, - completed_at: None, status, - backends: backends.to_string(), - profiles: profiles.to_string(), + backends: String::new(), + profiles: String::new(), } } @@ -927,7 +928,6 @@ impl BenchmarkRequest { parent_sha: parent_sha.map(|it| it.to_string()), }, created_at, - completed_at: None, status, backends: backends.to_string(), profiles: profiles.to_string(), @@ -950,7 +950,6 @@ impl BenchmarkRequest { parent_sha: parent_sha.to_string(), }, created_at, - completed_at: None, status, backends: backends.to_string(), profiles: profiles.to_string(), diff --git a/database/src/pool.rs b/database/src/pool.rs index 458f08af7..118878a7d 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -434,8 +434,6 @@ mod tests { "1.8.0", time, BenchmarkRequestStatus::ArtifactsReady, - "cranelift,llvm", - "", ); let db = db.connection().await; @@ -490,8 +488,6 @@ mod tests { "1.8.0", time, BenchmarkRequestStatus::ArtifactsReady, - "cranelift,llvm", - "", ); let db = db.connection().await; diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index ad99556a2..db0fe3258 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1453,58 +1453,56 @@ where let benchmark_requests = rows .iter() .map(|row| { - let tag = row.get::<_, Option<&str>>(0); - let parent_sha = row.get::<_, Option<&str>>(1); + let tag = row.get::<_, Option>(0); + let parent_sha = row.get::<_, Option>(1); let pr = row.get::<_, Option>(2); let commit_type = row.get::<_, &str>(3); - let status = row.get::<_, BenchmarkRequestStatus>(4); + let status = row.get::<_, &str>(4); let created_at = row.get::<_, DateTime>(5); let completed_at = row.get::<_, Option>>(6); - let backends = row.get::<_, &str>(7); - let profiles = row.get::<_, &str>(8); + let backends = row.get::<_, String>(7); + let profiles = row.get::<_, String>(8); + + let pr = pr.map(|v| v as u32); + + let status = + BenchmarkRequestStatus::from_str_and_completion_date(status, completed_at) + .expect("Invalid BenchmarkRequestStatus data in the database"); match commit_type { - BENCHMARK_REQUEST_TRY_STR => { - let mut try_benchmark = BenchmarkRequest::create_try( - tag, + BENCHMARK_REQUEST_TRY_STR => BenchmarkRequest { + commit_type: BenchmarkRequestType::Try { + sha: tag, parent_sha, - pr.unwrap() as u32, - created_at, - status, - backends, - profiles, - ); - try_benchmark.completed_at = completed_at; - try_benchmark - } - BENCHMARK_REQUEST_MASTER_STR => { - let mut master_benchmark = BenchmarkRequest::create_master( - tag.expect("Master commit in DB without SHA"), - parent_sha.unwrap(), - pr.unwrap() as u32, - created_at, - status, - backends, - profiles, - ); - master_benchmark.completed_at = completed_at; - master_benchmark - } - BENCHMARK_REQUEST_RELEASE_STR => { - let mut release_benchmark = BenchmarkRequest::create_release( - tag.expect("Release commit in DB witohut SHA"), - created_at, - status, - backends, - profiles, - ); - release_benchmark.completed_at = completed_at; - release_benchmark - } - _ => panic!( - "Invalid `commit_type` for `BenchmarkRequest` {}", - commit_type - ), + pr: pr.expect("Try commit in the DB without a PR"), + }, + created_at, + status, + backends, + profiles, + }, + BENCHMARK_REQUEST_MASTER_STR => BenchmarkRequest { + commit_type: BenchmarkRequestType::Master { + sha: tag.expect("Master commit in the DB without a SHA"), + parent_sha: parent_sha + .expect("Master commit in the DB without a parent SHA"), + pr: pr.expect("Master commit in the DB without a PR"), + }, + created_at, + status, + backends, + profiles, + }, + BENCHMARK_REQUEST_RELEASE_STR => BenchmarkRequest { + commit_type: BenchmarkRequestType::Release { + tag: tag.expect("Release commit in the DB without a SHA"), + }, + created_at, + status, + backends, + profiles, + }, + _ => panic!("Invalid `commit_type` for `BenchmarkRequest` {commit_type}",), } }) .collect(); @@ -1513,23 +1511,25 @@ where async fn update_benchmark_request_status( &mut self, - benchmark_request: &BenchmarkRequest, - benchmark_request_status: BenchmarkRequestStatus, + request: &BenchmarkRequest, + status: BenchmarkRequestStatus, ) -> anyhow::Result<()> { - let tx = self - .conn_mut() - .transaction() - .await - .context("failed to start transaction")?; - - tx.execute( - "UPDATE benchmark_request SET status = $1 WHERE tag = $2;", - &[&benchmark_request_status, &benchmark_request.tag()], - ) - .await - .context("failed to execute UPDATE benchmark_request")?; + let tag = request + .tag() + .expect("Cannot update status of a benchmark request without a tag. Use `attach_shas_to_try_benchmark_request` instead."); - tx.commit().await.context("failed to commit transaction")?; + let status_str = status.as_str(); + let completed_at = status.completed_at(); + self.conn() + .execute( + r#" + UPDATE benchmark_request + SET status = $1, completed_at = $2 + WHERE tag = $3;"#, + &[&status_str, &completed_at, &tag], + ) + .await + .context("failed to benchmark request status update")?; Ok(()) } diff --git a/site/src/job_queue.rs b/site/src/job_queue.rs index 1bde1ae06..d4779649d 100644 --- a/site/src/job_queue.rs +++ b/site/src/job_queue.rs @@ -129,8 +129,6 @@ async fn create_benchmark_request_releases( &name, date_time, BenchmarkRequestStatus::ArtifactsReady, - "", - "", ); if let Err(e) = conn.insert_benchmark_request(&release_request).await { log::error!("Failed to insert release benchmark request: {}", e); @@ -395,8 +393,6 @@ mod tests { tag, days_ago(age_days), BenchmarkRequestStatus::ArtifactsReady, - "", - "", ) } From 10904fcb4e2c47f8a4bd4195e01b164048c6fe15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 12:27:23 +0200 Subject: [PATCH 05/14] Create dedicated function for creating try requests without artifacts --- database/src/lib.rs | 14 +++++--------- site/src/request_handlers/github.rs | 25 +++++++++---------------- 2 files changed, 14 insertions(+), 25 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index 5eb066378..cffc0d325 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -852,8 +852,6 @@ impl fmt::Display for BenchmarkRequestStatus { } } - - const BENCHMARK_REQUEST_TRY_STR: &str = "try"; const BENCHMARK_REQUEST_MASTER_STR: &str = "master"; const BENCHMARK_REQUEST_RELEASE_STR: &str = "release"; @@ -912,23 +910,21 @@ impl BenchmarkRequest { } } - pub fn create_try( - sha: Option<&str>, - parent_sha: Option<&str>, + /// Create a try request that is waiting for artifacts + pub fn create_try_without_artifacts( pr: u32, created_at: DateTime, - status: BenchmarkRequestStatus, backends: &str, profiles: &str, ) -> Self { Self { commit_type: BenchmarkRequestType::Try { pr, - sha: sha.map(|it| it.to_string()), - parent_sha: parent_sha.map(|it| it.to_string()), + sha: None, + parent_sha: None, }, created_at, - status, + status: BenchmarkRequestStatus::WaitingForArtifacts, backends: backends.to_string(), profiles: profiles.to_string(), } diff --git a/site/src/request_handlers/github.rs b/site/src/request_handlers/github.rs index b3a0aedc2..becbd1b27 100644 --- a/site/src/request_handlers/github.rs +++ b/site/src/request_handlers/github.rs @@ -6,7 +6,7 @@ use crate::github::{ use crate::job_queue::run_new_queue; use crate::load::SiteCtxt; -use database::{BenchmarkRequest, BenchmarkRequestStatus}; +use database::BenchmarkRequest; use hashbrown::HashMap; use std::sync::Arc; @@ -74,25 +74,18 @@ async fn handle_issue( Ok(github::Response) } -/// The try does not have a `sha` or a `parent_sha` but we need to keep a record -/// of this commit existing. We make sure there can only be one `pr` with a -/// status of `WaitingForArtifacts` to ensure we don't have duplicates. -async fn queue_partial_try_benchmark_request( +/// The try request does not have a `sha` or a `parent_sha` but we need to keep a record +/// of this commit existing. The DB ensures that there is only one non-completed +/// try benchmark request per `pr`. +async fn record_try_benchmark_request_without_artifacts( conn: &dyn database::pool::Connection, pr: u32, backends: &str, ) { // We only want to run this if the new system is running if run_new_queue() { - let try_request = BenchmarkRequest::create_try( - None, - None, - pr, - chrono::Utc::now(), - BenchmarkRequestStatus::WaitingForArtifacts, - backends, - "", - ); + let try_request = + BenchmarkRequest::create_try_without_artifacts(pr, chrono::Utc::now(), backends, ""); if let Err(e) = conn.insert_benchmark_request(&try_request).await { log::error!("Failed to insert try benchmark request: {}", e); } @@ -125,7 +118,7 @@ async fn handle_rust_timer( Ok(cmd) => { let conn = ctxt.conn().await; - queue_partial_try_benchmark_request( + record_try_benchmark_request_without_artifacts( &*conn, issue.number, cmd.params.backends.unwrap_or(""), @@ -171,7 +164,7 @@ async fn handle_rust_timer( { let conn = ctxt.conn().await; for command in &valid_build_cmds { - queue_partial_try_benchmark_request( + record_try_benchmark_request_without_artifacts( &*conn, issue.number, command.params.backends.unwrap_or(""), From 8c99b0b6891abff6d39d432c5542c606d5e6a0fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 16:30:05 +0200 Subject: [PATCH 06/14] Add function for loading pending benchmark requests from the DB --- database/src/lib.rs | 63 +++++++----- database/src/pool.rs | 38 ++++---- database/src/pool/postgres.rs | 176 +++++++++++++++++++--------------- database/src/pool/sqlite.rs | 18 ++-- site/src/job_queue.rs | 79 +++++++-------- 5 files changed, 200 insertions(+), 174 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index cffc0d325..c352cb165 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -1,6 +1,6 @@ use chrono::offset::TimeZone; use chrono::{DateTime, Utc}; -use hashbrown::HashMap; +use hashbrown::{HashMap, HashSet}; use intern::intern; use serde::{Deserialize, Serialize}; use std::fmt; @@ -806,18 +806,18 @@ pub enum BenchmarkRequestStatus { Completed { completed_at: DateTime }, } -const WAITING_FOR_ARTIFACTS_STR: &str = "waiting_for_artifacts"; -const ARTIFACTS_READY_STR: &str = "artifacts_ready"; -const IN_PROGRESS_STR: &str = "in_progress"; -const COMPLETED_STR: &str = "completed"; +const BENCHMARK_REQUEST_STATUS_WAITING_FOR_ARTIFACTS_STR: &str = "waiting_for_artifacts"; +const BENCHMARK_REQUEST_STATUS_ARTIFACTS_READY_STR: &str = "artifacts_ready"; +const BENCHMARK_REQUEST_STATUS_IN_PROGRESS_STR: &str = "in_progress"; +const BENCHMARK_REQUEST_STATUS_COMPLETED_STR: &str = "completed"; impl BenchmarkRequestStatus { pub(crate) fn as_str(&self) -> &str { match self { - Self::WaitingForArtifacts => WAITING_FOR_ARTIFACTS_STR, - Self::ArtifactsReady => ARTIFACTS_READY_STR, - Self::InProgress => IN_PROGRESS_STR, - Self::Completed { .. } => COMPLETED_STR, + Self::WaitingForArtifacts => BENCHMARK_REQUEST_STATUS_WAITING_FOR_ARTIFACTS_STR, + Self::ArtifactsReady => BENCHMARK_REQUEST_STATUS_ARTIFACTS_READY_STR, + Self::InProgress => BENCHMARK_REQUEST_STATUS_IN_PROGRESS_STR, + Self::Completed { .. } => BENCHMARK_REQUEST_STATUS_COMPLETED_STR, } } @@ -826,10 +826,10 @@ impl BenchmarkRequestStatus { completion_date: Option>, ) -> anyhow::Result { match text { - WAITING_FOR_ARTIFACTS_STR => Ok(Self::WaitingForArtifacts), - ARTIFACTS_READY_STR => Ok(Self::ArtifactsReady), - IN_PROGRESS_STR => Ok(Self::InProgress), - COMPLETED_STR => Ok(Self::Completed { + BENCHMARK_REQUEST_STATUS_WAITING_FOR_ARTIFACTS_STR => Ok(Self::WaitingForArtifacts), + BENCHMARK_REQUEST_STATUS_ARTIFACTS_READY_STR => Ok(Self::ArtifactsReady), + BENCHMARK_REQUEST_STATUS_IN_PROGRESS_STR => Ok(Self::InProgress), + BENCHMARK_REQUEST_STATUS_COMPLETED_STR => Ok(Self::Completed { completed_at: completion_date.ok_or_else(|| { anyhow!("No completion date for a completed BenchmarkRequestStatus") })?, @@ -930,15 +930,7 @@ impl BenchmarkRequest { } } - pub fn create_master( - sha: &str, - parent_sha: &str, - pr: u32, - created_at: DateTime, - status: BenchmarkRequestStatus, - backends: &str, - profiles: &str, - ) -> Self { + pub fn create_master(sha: &str, parent_sha: &str, pr: u32, created_at: DateTime) -> Self { Self { commit_type: BenchmarkRequestType::Master { pr, @@ -946,9 +938,9 @@ impl BenchmarkRequest { parent_sha: parent_sha.to_string(), }, created_at, - status, - backends: backends.to_string(), - profiles: profiles.to_string(), + status: BenchmarkRequestStatus::ArtifactsReady, + backends: String::new(), + profiles: String::new(), } } @@ -979,3 +971,24 @@ impl BenchmarkRequest { } } } + +/// Cached information about benchmark requests in the DB +/// FIXME: only store non-try requests here +pub struct BenchmarkRequestIndex { + /// Tags (SHA or release name) of all known benchmark requests + all: HashSet, + /// Tags (SHA or release name) of all benchmark requests in the completed status + completed: HashSet, +} + +impl BenchmarkRequestIndex { + /// Do we already have a benchmark request for the passed `tag`? + pub fn contains_tag(&self, tag: &str) -> bool { + self.all.contains(tag) + } + + /// Return tags of already completed benchmark requests. + pub fn completed_requests(&self) -> &HashSet { + &self.completed + } +} diff --git a/database/src/pool.rs b/database/src/pool.rs index 118878a7d..5008edb62 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -1,6 +1,6 @@ use crate::{ - ArtifactCollection, ArtifactId, ArtifactIdNumber, BenchmarkRequest, BenchmarkRequestStatus, - CodegenBackend, CompileBenchmark, Target, + ArtifactCollection, ArtifactId, ArtifactIdNumber, BenchmarkRequest, BenchmarkRequestIndex, + BenchmarkRequestStatus, CodegenBackend, CompileBenchmark, Target, }; use crate::{CollectionId, Index, Profile, QueuedCommit, Scenario, Step}; use chrono::{DateTime, Utc}; @@ -187,18 +187,18 @@ pub trait Connection: Send + Sync { benchmark_request: &BenchmarkRequest, ) -> anyhow::Result<()>; - /// Gets the benchmark requests matching the status. Optionally provide the - /// number of days from whence to search from - async fn get_benchmark_requests_by_status( - &self, - statuses: &[BenchmarkRequestStatus], - ) -> anyhow::Result>; + /// Load all known benchmark request SHAs and all completed benchmark requests. + async fn load_benchmark_request_index(&self) -> anyhow::Result; + + /// Load all pending benchmark requests, i.e. those that have artifacts ready, but haven't + /// been completed yet. Pending statuses are `ArtifactsReady` and `InProgress`. + async fn load_pending_benchmark_requests(&self) -> anyhow::Result>; /// Update the status of a `benchmark_request` async fn update_benchmark_request_status( &mut self, - benchmark_request: &BenchmarkRequest, - benchmark_request_status: BenchmarkRequestStatus, + request: &BenchmarkRequest, + status: BenchmarkRequestStatus, ) -> anyhow::Result<()>; /// Update a Try commit to have a `sha` and `parent_sha`. Will update the @@ -420,7 +420,7 @@ mod tests { "", ); - let try_benchmark_request = BenchmarkRequest::create_try( + let try_benchmark_request = BenchmarkRequest::create_try_without_artifacts( Some("b-sha-2"), Some("parent-sha-2"), 32, @@ -474,7 +474,7 @@ mod tests { "", ); - let try_benchmark_request = BenchmarkRequest::create_try( + let try_benchmark_request = BenchmarkRequest::create_try_without_artifacts( Some("b-sha-2"), Some("parent-sha-2"), 32, @@ -502,7 +502,7 @@ mod tests { .unwrap(); let requests = db - .get_benchmark_requests_by_status(&[BenchmarkRequestStatus::ArtifactsReady]) + .load_benchmark_request_index(&[BenchmarkRequestStatus::ArtifactsReady]) .await .unwrap(); @@ -545,7 +545,7 @@ mod tests { .unwrap(); let requests = db - .get_benchmark_requests_by_status(&[BenchmarkRequestStatus::InProgress]) + .load_benchmark_request_index(&[BenchmarkRequestStatus::InProgress]) .await .unwrap(); @@ -566,7 +566,7 @@ mod tests { let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); let pr = 42; - let try_benchmark_request = BenchmarkRequest::create_try( + let try_benchmark_request = BenchmarkRequest::create_try_without_artifacts( None, None, pr, @@ -582,7 +582,7 @@ mod tests { .await .unwrap(); let requests = db - .get_benchmark_requests_by_status(&[BenchmarkRequestStatus::ArtifactsReady]) + .load_benchmark_request_index(&[BenchmarkRequestStatus::ArtifactsReady]) .await .unwrap(); @@ -604,7 +604,7 @@ mod tests { let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); let pr = 42; - let completed_try = BenchmarkRequest::create_try( + let completed_try = BenchmarkRequest::create_try_without_artifacts( Some("sha-2"), Some("p-sha-1"), pr, @@ -615,7 +615,7 @@ mod tests { ); db.insert_benchmark_request(&completed_try).await.unwrap(); - let try_benchmark_request = BenchmarkRequest::create_try( + let try_benchmark_request = BenchmarkRequest::create_try_without_artifacts( None, None, pr, @@ -638,7 +638,7 @@ mod tests { .unwrap(); let requests = db - .get_benchmark_requests_by_status(&[ + .load_benchmark_request_index(&[ BenchmarkRequestStatus::WaitingForArtifacts, BenchmarkRequestStatus::ArtifactsReady, BenchmarkRequestStatus::InProgress, diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index db0fe3258..dff72b8d7 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1,13 +1,15 @@ use crate::pool::{Connection, ConnectionManager, ManagedConnection, Transaction}; use crate::{ ArtifactCollection, ArtifactId, ArtifactIdNumber, Benchmark, BenchmarkRequest, - BenchmarkRequestStatus, BenchmarkRequestType, CodegenBackend, CollectionId, Commit, CommitType, - CompileBenchmark, Date, Index, Profile, QueuedCommit, Scenario, Target, - BENCHMARK_REQUEST_MASTER_STR, BENCHMARK_REQUEST_RELEASE_STR, BENCHMARK_REQUEST_TRY_STR, + BenchmarkRequestIndex, BenchmarkRequestStatus, BenchmarkRequestType, CodegenBackend, + CollectionId, Commit, CommitType, CompileBenchmark, Date, Index, Profile, QueuedCommit, + Scenario, Target, BENCHMARK_REQUEST_MASTER_STR, BENCHMARK_REQUEST_RELEASE_STR, + BENCHMARK_REQUEST_STATUS_ARTIFACTS_READY_STR, BENCHMARK_REQUEST_STATUS_COMPLETED_STR, + BENCHMARK_REQUEST_STATUS_IN_PROGRESS_STR, BENCHMARK_REQUEST_TRY_STR, }; use anyhow::Context as _; use chrono::{DateTime, TimeZone, Utc}; -use hashbrown::HashMap; +use hashbrown::{HashMap, HashSet}; use native_tls::{Certificate, TlsConnector}; use postgres_native_tls::MakeTlsConnector; use std::str::FromStr; @@ -388,6 +390,7 @@ pub struct CachedStatements { get_runtime_pstat: Statement, record_artifact_size: Statement, get_artifact_size: Statement, + load_benchmark_request_index: Statement, } pub struct PostgresTransaction<'a> { @@ -569,7 +572,11 @@ impl PostgresConnection { get_artifact_size: conn.prepare(" select component, size from artifact_size where aid = $1 - ").await.unwrap() + ").await.unwrap(), + load_benchmark_request_index: conn.prepare(" + select tag, status + from benchmark_request + ").await.unwrap(), }), conn, } @@ -1423,13 +1430,88 @@ where Ok(()) } - async fn get_benchmark_requests_by_status( + async fn load_benchmark_request_index(&self) -> anyhow::Result { + let requests = self + .conn() + .query(&self.statements().load_benchmark_request_index, &[]) + .await + .context("Cannot load benchmark request index")?; + + let mut all = HashSet::with_capacity(requests.len()); + let mut completed = HashSet::with_capacity(requests.len()); + for request in requests { + let tag = request.get::<_, String>(0); + let status = request.get::<_, &str>(1); + + if status == BENCHMARK_REQUEST_STATUS_COMPLETED_STR { + completed.insert(tag.clone()); + } + all.insert(tag); + } + Ok(BenchmarkRequestIndex { all, completed }) + } + + async fn update_benchmark_request_status( + &mut self, + request: &BenchmarkRequest, + status: BenchmarkRequestStatus, + ) -> anyhow::Result<()> { + let tag = request + .tag() + .expect("Cannot update status of a benchmark request without a tag. Use `attach_shas_to_try_benchmark_request` instead."); + + let status_str = status.as_str(); + let completed_at = status.completed_at(); + self.conn() + .execute( + r#" + UPDATE benchmark_request + SET status = $1, completed_at = $2 + WHERE tag = $3;"#, + &[&status_str, &completed_at, &tag], + ) + .await + .context("failed to benchmark request status update")?; + + Ok(()) + } + + async fn attach_shas_to_try_benchmark_request( &self, - statuses: &[BenchmarkRequestStatus], - ) -> anyhow::Result> { - // There is a small period of time where a try commit's parent_sha - // could be NULL, this query will filter that out. - let query = " + pr: u32, + sha: &str, + parent_sha: &str, + ) -> anyhow::Result<()> { + self.conn() + .execute( + "UPDATE + benchmark_request + SET + tag = $1, + parent_sha = $2, + status = $3 + WHERE + pr = $4 + AND commit_type = 'try' + AND tag IS NULL + AND status = $5;", + &[ + &sha, + &parent_sha, + &BenchmarkRequestStatus::ArtifactsReady, + &(pr as i32), + &BenchmarkRequestStatus::WaitingForArtifacts, + ], + ) + .await + .context("failed to execute UPDATE benchmark_request")?; + + Ok(()) + } + + async fn load_pending_benchmark_requests(&self) -> anyhow::Result> { + let query = format!( + r#" SELECT tag, parent_sha, @@ -1441,17 +1523,17 @@ where backends, profiles FROM benchmark_request - WHERE status = ANY($1)" - .to_string(); + WHERE status = ANY('{BENCHMARK_REQUEST_STATUS_ARTIFACTS_READY_STR}', '{BENCHMARK_REQUEST_STATUS_IN_PROGRESS_STR}')"# + ); let rows = self .conn() - .query(&query, &[&statuses]) + .query(&query, &[]) .await - .context("Failed to get benchmark requests")?; + .context("Failed to get pending benchmark requests")?; - let benchmark_requests = rows - .iter() + let requests = rows + .into_iter() .map(|row| { let tag = row.get::<_, Option>(0); let parent_sha = row.get::<_, Option>(1); @@ -1506,65 +1588,7 @@ where } }) .collect(); - Ok(benchmark_requests) - } - - async fn update_benchmark_request_status( - &mut self, - request: &BenchmarkRequest, - status: BenchmarkRequestStatus, - ) -> anyhow::Result<()> { - let tag = request - .tag() - .expect("Cannot update status of a benchmark request without a tag. Use `attach_shas_to_try_benchmark_request` instead."); - - let status_str = status.as_str(); - let completed_at = status.completed_at(); - self.conn() - .execute( - r#" - UPDATE benchmark_request - SET status = $1, completed_at = $2 - WHERE tag = $3;"#, - &[&status_str, &completed_at, &tag], - ) - .await - .context("failed to benchmark request status update")?; - - Ok(()) - } - - async fn attach_shas_to_try_benchmark_request( - &self, - pr: u32, - sha: &str, - parent_sha: &str, - ) -> anyhow::Result<()> { - self.conn() - .execute( - "UPDATE - benchmark_request - SET - tag = $1, - parent_sha = $2, - status = $3 - WHERE - pr = $4 - AND commit_type = 'try' - AND tag IS NULL - AND status = $5;", - &[ - &sha, - &parent_sha, - &BenchmarkRequestStatus::ArtifactsReady, - &(pr as i32), - &BenchmarkRequestStatus::WaitingForArtifacts, - ], - ) - .await - .context("failed to execute UPDATE benchmark_request")?; - - Ok(()) + Ok(requests) } } diff --git a/database/src/pool/sqlite.rs b/database/src/pool/sqlite.rs index 6dcc9a608..fa5c42c81 100644 --- a/database/src/pool/sqlite.rs +++ b/database/src/pool/sqlite.rs @@ -1,7 +1,8 @@ use crate::pool::{Connection, ConnectionManager, ManagedConnection, Transaction}; use crate::{ - ArtifactCollection, ArtifactId, Benchmark, BenchmarkRequest, BenchmarkRequestStatus, - CodegenBackend, CollectionId, Commit, CommitType, CompileBenchmark, Date, Profile, Target, + ArtifactCollection, ArtifactId, Benchmark, BenchmarkRequest, BenchmarkRequestIndex, + BenchmarkRequestStatus, CodegenBackend, CollectionId, Commit, CommitType, CompileBenchmark, + Date, Profile, Target, }; use crate::{ArtifactIdNumber, Index, QueuedCommit}; use chrono::{DateTime, TimeZone, Utc}; @@ -1271,17 +1272,18 @@ impl Connection for SqliteConnection { no_queue_implementation_abort!() } - async fn get_benchmark_requests_by_status( - &self, - _statuses: &[BenchmarkRequestStatus], - ) -> anyhow::Result> { + async fn load_benchmark_request_index(&self) -> anyhow::Result { + no_queue_implementation_abort!() + } + + async fn load_pending_benchmark_requests(&self) -> anyhow::Result> { no_queue_implementation_abort!() } async fn update_benchmark_request_status( &mut self, - _benchmark_request: &BenchmarkRequest, - _benchmark_request_status: BenchmarkRequestStatus, + _request: &BenchmarkRequest, + _status: BenchmarkRequestStatus, ) -> anyhow::Result<()> { no_queue_implementation_abort!() } diff --git a/site/src/job_queue.rs b/site/src/job_queue.rs index d4779649d..fa4196413 100644 --- a/site/src/job_queue.rs +++ b/site/src/job_queue.rs @@ -6,7 +6,9 @@ use std::{ use crate::load::{partition_in_place, SiteCtxt}; use chrono::{DateTime, NaiveDate, Utc}; -use database::{BenchmarkRequest, BenchmarkRequestStatus, BenchmarkRequestType}; +use database::{ + BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestStatus, BenchmarkRequestType, +}; use hashbrown::HashSet; use parking_lot::RwLock; use regex::Regex; @@ -24,6 +26,7 @@ pub fn run_new_queue() -> bool { async fn create_benchmark_request_master_commits( ctxt: &Arc, conn: &dyn database::pool::Connection, + index: &BenchmarkRequestIndex, ) -> anyhow::Result<()> { let master_commits = &ctxt.get_master_commits().commits; // TODO; delete at some point in the future @@ -31,19 +34,16 @@ async fn create_benchmark_request_master_commits( for master_commit in master_commits { // We don't want to add masses of obsolete data - if master_commit.time >= cutoff { + if master_commit.time >= cutoff && !index.contains_tag(&master_commit.sha) { let pr = master_commit.pr.unwrap_or(0); let benchmark = BenchmarkRequest::create_master( &master_commit.sha, &master_commit.parent_sha, pr, master_commit.time, - BenchmarkRequestStatus::ArtifactsReady, - "", - "", ); - if let Err(e) = conn.insert_benchmark_request(&benchmark).await { - log::error!("Failed to insert master benchmark request: {}", e); + if let Err(error) = conn.insert_benchmark_request(&benchmark).await { + log::error!("Failed to insert master benchmark request: {error:?}"); } } } @@ -108,6 +108,7 @@ fn parse_release_string(url: &str) -> Option<(String, DateTime)> { /// already in the database async fn create_benchmark_request_releases( conn: &dyn database::pool::Connection, + index: &BenchmarkRequestIndex, ) -> anyhow::Result<()> { let releases: String = reqwest::get("https://static.rust-lang.org/manifests.txt") .await? @@ -124,14 +125,14 @@ async fn create_benchmark_request_releases( .collect(); for (name, date_time) in releases { - if date_time >= cutoff { + if date_time >= cutoff && !index.contains_tag(&name) { let release_request = BenchmarkRequest::create_release( &name, date_time, BenchmarkRequestStatus::ArtifactsReady, ); - if let Err(e) = conn.insert_benchmark_request(&release_request).await { - log::error!("Failed to insert release benchmark request: {}", e); + if let Err(error) = conn.insert_benchmark_request(&release_request).await { + log::error!("Failed to insert release benchmark request: {error}"); } } } @@ -140,8 +141,8 @@ async fn create_benchmark_request_releases( /// Sorts try and master requests that are in the `ArtifactsReady` status. /// Doesn't consider in-progress requests or release artifacts. -fn sort_benchmark_requests(done: &HashSet, request_queue: &mut [BenchmarkRequest]) { - let mut done: HashSet = done.iter().cloned().collect(); +fn sort_benchmark_requests(index: &BenchmarkRequestIndex, request_queue: &mut [BenchmarkRequest]) { + let mut done: HashSet = index.completed_requests().clone(); // Ensure all the items are ready to be sorted, if they are not this is // undefined behaviour @@ -233,17 +234,18 @@ impl ExtractIf for Vec { } } -/// Assumes that master/release artifacts have been put into the DB. +/// Creates a benchmark request queue that determines in what order will +/// the requests be benchmarked. The ordering should be created in such a way that +/// after an in-progress request is finished, the ordering of the rest of the queue does not +/// change (unless some other request was added to the queue in the meantime). +/// +/// Does not consider requests that are waiting for artifacts or that are alredy completed. pub async fn build_queue( conn: &mut dyn database::pool::Connection, - completed_set: &HashSet, + index: &BenchmarkRequestIndex, ) -> anyhow::Result> { - let mut pending = conn - .get_benchmark_requests_by_status(&[ - BenchmarkRequestStatus::InProgress, - BenchmarkRequestStatus::ArtifactsReady, - ]) - .await?; + // Load ArtifactsReady and InProgress benchmark requests + let mut pending = conn.load_pending_benchmark_requests().await?; // The queue starts with in progress let mut queue: Vec = pending @@ -264,44 +266,29 @@ pub async fn build_queue( }); queue.append(&mut release_artifacts); - sort_benchmark_requests(completed_set, &mut pending); + sort_benchmark_requests(index, &mut pending); queue.append(&mut pending); Ok(queue) } /// Enqueue the job into the job_queue -async fn enqueue_next_job(conn: &mut dyn database::pool::Connection) -> anyhow::Result<()> { - // We draw back all completed requests - let completed: HashSet = conn - .get_benchmark_requests_by_status(&[BenchmarkRequestStatus::Completed]) - .await? - .into_iter() - .filter_map(|request| request.tag().map(|tag| tag.to_string())) - .collect(); - - let queue = build_queue(conn, &completed).await?; - - if let Some(request) = queue.into_iter().next() { - if request.status != BenchmarkRequestStatus::InProgress { - // TODO: - // - Uncomment - // - Actually enqueue the jobs - // conn.update_benchmark_request_status(&request, BenchmarkRequestStatus::InProgress) - // .await?; - } - } - +async fn enqueue_next_job( + conn: &mut dyn database::pool::Connection, + index: &mut BenchmarkRequestIndex, +) -> anyhow::Result<()> { + let _queue = build_queue(conn, index).await?; Ok(()) } /// For queueing jobs, add the jobs you want to queue to this function async fn cron_enqueue_jobs(site_ctxt: &Arc) -> anyhow::Result<()> { let mut conn = site_ctxt.conn().await; + let mut index = conn.load_benchmark_request_index().await?; // Put the master commits into the `benchmark_requests` queue - create_benchmark_request_master_commits(site_ctxt, &*conn).await?; + create_benchmark_request_master_commits(site_ctxt, &*conn, &index).await?; // Put the releases into the `benchmark_requests` queue - create_benchmark_request_releases(&*conn).await?; - enqueue_next_job(&mut *conn).await?; + create_benchmark_request_releases(&*conn, &index).await?; + enqueue_next_job(&mut *conn, &mut index).await?; Ok(()) } @@ -377,7 +364,7 @@ mod tests { } fn create_try(sha: &str, parent: &str, pr: u32, age_days: &str) -> BenchmarkRequest { - BenchmarkRequest::create_try( + BenchmarkRequest::create_try_without_artifacts( Some(sha), Some(parent), pr, From 5053b1d0f3dc6261de528c82fc5a70b24dc11415 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 17:06:11 +0200 Subject: [PATCH 07/14] Make `BenchmarkRequest` fields private To ensure that they can't be messed with from the outside. --- database/src/lib.rs | 44 ++++++++++++++++++++++++++++++------------- site/src/job_queue.rs | 40 +++++++++++++-------------------------- 2 files changed, 44 insertions(+), 40 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index c352cb165..b171f47ee 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -798,7 +798,7 @@ pub struct ArtifactCollection { pub end_time: DateTime, } -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Copy, Clone, PartialEq)] pub enum BenchmarkRequestStatus { WaitingForArtifacts, ArtifactsReady, @@ -886,31 +886,28 @@ impl fmt::Display for BenchmarkRequestType { #[derive(Debug, Clone, PartialEq)] pub struct BenchmarkRequest { - pub commit_type: BenchmarkRequestType, - pub created_at: DateTime, - pub status: BenchmarkRequestStatus, - pub backends: String, - pub profiles: String, + commit_type: BenchmarkRequestType, + created_at: DateTime, + status: BenchmarkRequestStatus, + backends: String, + profiles: String, } impl BenchmarkRequest { - pub fn create_release( - tag: &str, - created_at: DateTime, - status: BenchmarkRequestStatus, - ) -> Self { + /// Create a release benchmark request that is in the `ArtifactsReady` status. + pub fn create_release(tag: &str, created_at: DateTime) -> Self { Self { commit_type: BenchmarkRequestType::Release { tag: tag.to_string(), }, created_at, - status, + status: BenchmarkRequestStatus::ArtifactsReady, backends: String::new(), profiles: String::new(), } } - /// Create a try request that is waiting for artifacts + /// Create a try request that is in the `WaitingForArtifacts` status. pub fn create_try_without_artifacts( pr: u32, created_at: DateTime, @@ -930,6 +927,7 @@ impl BenchmarkRequest { } } + /// Create a master benchmark request that is in the `ArtifactsReady` status. pub fn create_master(sha: &str, parent_sha: &str, pr: u32, created_at: DateTime) -> Self { Self { commit_type: BenchmarkRequestType::Master { @@ -970,6 +968,26 @@ impl BenchmarkRequest { BenchmarkRequestType::Release { .. } => None, } } + + pub fn status(&self) -> BenchmarkRequestStatus { + self.status + } + + pub fn created_at(&self) -> DateTime { + self.created_at + } + + pub fn is_master(&self) -> bool { + matches!(self.commit_type, BenchmarkRequestType::Master { .. }) + } + + pub fn is_try(&self) -> bool { + matches!(self.commit_type, BenchmarkRequestType::Try { .. }) + } + + pub fn is_release(&self) -> bool { + matches!(self.commit_type, BenchmarkRequestType::Release { .. }) + } } /// Cached information about benchmark requests in the DB diff --git a/site/src/job_queue.rs b/site/src/job_queue.rs index fa4196413..cf3f2aaa6 100644 --- a/site/src/job_queue.rs +++ b/site/src/job_queue.rs @@ -6,9 +6,7 @@ use std::{ use crate::load::{partition_in_place, SiteCtxt}; use chrono::{DateTime, NaiveDate, Utc}; -use database::{ - BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestStatus, BenchmarkRequestType, -}; +use database::{BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestStatus}; use hashbrown::HashSet; use parking_lot::RwLock; use regex::Regex; @@ -126,11 +124,7 @@ async fn create_benchmark_request_releases( for (name, date_time) in releases { if date_time >= cutoff && !index.contains_tag(&name) { - let release_request = BenchmarkRequest::create_release( - &name, - date_time, - BenchmarkRequestStatus::ArtifactsReady, - ); + let release_request = BenchmarkRequest::create_release(&name, date_time); if let Err(error) = conn.insert_benchmark_request(&release_request).await { log::error!("Failed to insert release benchmark request: {error}"); } @@ -147,11 +141,7 @@ fn sort_benchmark_requests(index: &BenchmarkRequestIndex, request_queue: &mut [B // Ensure all the items are ready to be sorted, if they are not this is // undefined behaviour assert!(request_queue.iter().all(|bmr| { - bmr.status == BenchmarkRequestStatus::ArtifactsReady - && matches!( - bmr.commit_type, - BenchmarkRequestType::Master { .. } | BenchmarkRequestType::Try { .. } - ) + bmr.status() == BenchmarkRequestStatus::ArtifactsReady && (bmr.is_master() || bmr.is_try()) })); let mut finished = 0; @@ -180,15 +170,11 @@ fn sort_benchmark_requests(index: &BenchmarkRequestIndex, request_queue: &mut [B let level = &mut request_queue[finished..][..level_len]; level.sort_unstable_by_key(|bmr| { ( - // Pr number takes priority + // PR number takes priority *bmr.pr().unwrap_or(&0), // Order master commits before try commits - match bmr.commit_type { - BenchmarkRequestType::Try { .. } => 1, - BenchmarkRequestType::Master { .. } => 0, - BenchmarkRequestType::Release { .. } => unreachable!(), - }, - bmr.created_at, + if bmr.is_master() { 0 } else { 1 }, + bmr.created_at(), ) }); for c in level { @@ -248,21 +234,21 @@ pub async fn build_queue( let mut pending = conn.load_pending_benchmark_requests().await?; // The queue starts with in progress - let mut queue: Vec = pending - .extract_if_stable(|request| matches!(request.status, BenchmarkRequestStatus::InProgress)); + let mut queue: Vec = pending.extract_if_stable(|request| { + matches!(request.status(), BenchmarkRequestStatus::InProgress) + }); // We sort the in-progress ones based on the started date - queue.sort_unstable_by(|a, b| a.created_at.cmp(&b.created_at)); + queue.sort_unstable_by(|a, b| a.created_at().cmp(&b.created_at())); // Add release artifacts ordered by the release tag (1.87.0 before 1.88.0) and `created_at`. - let mut release_artifacts: Vec = pending.extract_if_stable(|request| { - matches!(request.commit_type, BenchmarkRequestType::Release { .. }) - }); + let mut release_artifacts: Vec = + pending.extract_if_stable(|request| request.is_release()); release_artifacts.sort_unstable_by(|a, b| { a.tag() .cmp(&b.tag()) - .then_with(|| a.created_at.cmp(&b.created_at)) + .then_with(|| a.created_at().cmp(&b.created_at())) }); queue.append(&mut release_artifacts); From 611d8e0a59c16710224065683629b05f837c3e8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 17:07:26 +0200 Subject: [PATCH 08/14] Change `job_queue.rs` into a directory module --- site/src/{job_queue.rs => job_queue/mod.rs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename site/src/{job_queue.rs => job_queue/mod.rs} (100%) diff --git a/site/src/job_queue.rs b/site/src/job_queue/mod.rs similarity index 100% rename from site/src/job_queue.rs rename to site/src/job_queue/mod.rs From 01379fc13068833fa93b733a64ee99626274c5de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 17:10:03 +0200 Subject: [PATCH 09/14] Move `ExtractIf` and version parsing to `utils` module --- site/src/job_queue/mod.rs | 158 ++---------------------------------- site/src/job_queue/utils.rs | 154 +++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 152 deletions(-) create mode 100644 site/src/job_queue/utils.rs diff --git a/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index cf3f2aaa6..d435eae06 100644 --- a/site/src/job_queue/mod.rs +++ b/site/src/job_queue/mod.rs @@ -1,15 +1,13 @@ -use std::{ - path::Path, - str::FromStr, - sync::{Arc, LazyLock}, -}; +mod utils; +use std::{str::FromStr, sync::Arc}; + +use crate::job_queue::utils::{parse_release_string, ExtractIf}; use crate::load::{partition_in_place, SiteCtxt}; -use chrono::{DateTime, NaiveDate, Utc}; +use chrono::Utc; use database::{BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestStatus}; use hashbrown::HashSet; use parking_lot::RwLock; -use regex::Regex; use tokio::time::{self, Duration}; pub fn run_new_queue() -> bool { @@ -48,60 +46,6 @@ async fn create_benchmark_request_master_commits( Ok(()) } -/// Parses strings in the following formats extracting the Date & release tag -/// `static.rust-lang.org/dist/2016-05-24/channel-rust-1.9.0.toml` -/// `static.rust-lang.org/dist/2016-05-31/channel-rust-nightly.toml` -/// `static.rust-lang.org/dist/2016-06-01/channel-rust-beta.toml` -/// `static.rust-lang.org/dist/2025-06-26/channel-rust-1.89-beta.toml` -/// `static.rust-lang.org/dist/2025-06-26/channel-rust-1.89.0-beta.toml` -/// `static.rust-lang.org/dist/2025-06-26/channel-rust-1.89.0-beta.2.toml` -fn parse_release_string(url: &str) -> Option<(String, DateTime)> { - static VERSION_RE: LazyLock = LazyLock::new(|| Regex::new(r"(\d+\.\d+\.\d+)").unwrap()); - - // Grab ".../YYYY-MM-DD/FILE.toml" components with Path helpers. - let file = Path::new(url).file_name().and_then(|n| n.to_str())?; - - let date_str = Path::new(url) - .parent() - .and_then(Path::file_name) - .and_then(|n| n.to_str())?; - - // No other beta releases are recognized as toolchains. - // - // We also have names like this: - // - // * channel-rust-1.75-beta.toml - // * channel-rust-1.75.0-beta.toml - // * channel-rust-1.75.0-beta.1.toml - // - // Which should get ignored for now, they're not consumable via rustup yet. - if file.contains("beta") && file != "channel-rust-beta.toml" { - return None; - } - - // Parse the YYYY-MM-DD segment and stamp it with *current* UTC time. - if let Ok(naive) = NaiveDate::parse_from_str(date_str, "%Y-%m-%d") { - let published = naive - .and_hms_opt(0, 0, 0) - .expect("valid HMS") - .and_local_timezone(Utc) - .single() - .unwrap(); - - // Special-case the rolling beta channel. - if file == "channel-rust-beta.toml" { - return Some((format!("beta-{date_str}"), published)); - } - - // Otherwise pull out a semver like "1.70.0" and return it. - if let Some(cap) = VERSION_RE.captures(file).and_then(|m| m.get(1)) { - return Some((cap.as_str().to_owned(), published)); - } - } - - None -} - /// Store the latest release commits or do nothing if all of them are /// already in the database async fn create_benchmark_request_releases( @@ -193,33 +137,6 @@ fn sort_benchmark_requests(index: &BenchmarkRequestIndex, request_queue: &mut [B } } -pub trait ExtractIf { - fn extract_if_stable(&mut self, predicate: F) -> Vec - where - F: FnMut(&T) -> bool; -} - -/// Vec method `extract_if` is unstable, this very simple implementation -/// can be deleted once it is stable -impl ExtractIf for Vec { - fn extract_if_stable(&mut self, mut predicate: F) -> Vec - where - F: FnMut(&T) -> bool, - { - let mut extracted = Vec::new(); - let mut i = 0; - - while i < self.len() { - if predicate(&self[i]) { - extracted.push(self.remove(i)); - } else { - i += 1; - } - } - extracted - } -} - /// Creates a benchmark request queue that determines in what order will /// the requests be benchmarked. The ordering should be created in such a way that /// after an in-progress request is finished, the ordering of the rest of the queue does not @@ -301,21 +218,9 @@ pub async fn cron_main(site_ctxt: Arc>>>, seconds: u #[cfg(test)] mod tests { use super::*; - use chrono::{Datelike, Duration, NaiveDate, TimeZone, Utc}; + use chrono::{Datelike, Duration, TimeZone, Utc}; use database::tests::run_postgres_test; - /// Helper: unwrap the Option, panic otherwise. - fn tag(url: &str) -> String { - parse_release_string(url) - .expect("Some") // Option<_> - .0 // take the tag - } - - /// Helper: unwrap the DateTime and keep only the YYYY-MM-DD part - fn day(url: &str) -> NaiveDate { - parse_release_string(url).expect("Some").1.date_naive() - } - fn days_ago(day_str: &str) -> chrono::DateTime { // Walk backwards until the first non-digit, then slice let days = day_str @@ -488,55 +393,4 @@ mod tests { }) .await; } - - #[test] - fn parses_stable_versions() { - assert_eq!( - tag("static.rust-lang.org/dist/2016-05-24/channel-rust-1.9.0.toml"), - "1.9.0" - ); - assert_eq!( - day("static.rust-lang.org/dist/2016-05-24/channel-rust-1.9.0.toml"), - NaiveDate::from_ymd_opt(2016, 5, 24).unwrap() - ); - - assert_eq!( - tag("static.rust-lang.org/dist/2025-06-26/channel-rust-1.88.0.toml"), - "1.88.0" - ); - assert_eq!( - day("static.rust-lang.org/dist/2025-06-26/channel-rust-1.88.0.toml"), - NaiveDate::from_ymd_opt(2025, 6, 26).unwrap() - ); - } - - #[test] - fn parses_plain_beta_channel() { - let want = "beta-2016-06-01"; - let url = "static.rust-lang.org/dist/2016-06-01/channel-rust-beta.toml"; - - assert_eq!(tag(url), want); - assert_eq!(day(url), NaiveDate::from_ymd_opt(2016, 6, 1).unwrap()); - } - - #[test] - fn skips_unconsumable_channels() { - // nightly never returns Anything - assert!(parse_release_string( - "static.rust-lang.org/dist/2016-05-31/channel-rust-nightly.toml" - ) - .is_none()); - - // versioned-beta artefacts are skipped too - for should_ignore in [ - "static.rust-lang.org/dist/2025-06-26/channel-rust-1.89-beta.toml", - "static.rust-lang.org/dist/2025-06-26/channel-rust-1.89.0-beta.toml", - "static.rust-lang.org/dist/2025-06-26/channel-rust-1.89.0-beta.2.toml", - ] { - assert!( - parse_release_string(should_ignore).is_none(), - "{should_ignore} should be ignored" - ); - } - } } diff --git a/site/src/job_queue/utils.rs b/site/src/job_queue/utils.rs new file mode 100644 index 000000000..3ade039c4 --- /dev/null +++ b/site/src/job_queue/utils.rs @@ -0,0 +1,154 @@ +use chrono::{DateTime, NaiveDate, Utc}; +use regex::Regex; +use std::path::Path; +use std::sync::LazyLock; + +pub trait ExtractIf { + fn extract_if_stable(&mut self, predicate: F) -> Vec + where + F: FnMut(&T) -> bool; +} + +/// Vec method `extract_if` is unstable, this very simple implementation +/// can be deleted once it is stable +impl ExtractIf for Vec { + fn extract_if_stable(&mut self, mut predicate: F) -> Vec + where + F: FnMut(&T) -> bool, + { + let mut extracted = Vec::new(); + let mut i = 0; + + while i < self.len() { + if predicate(&self[i]) { + extracted.push(self.remove(i)); + } else { + i += 1; + } + } + extracted + } +} + +/// Parses strings in the following formats extracting the Date & release tag +/// `static.rust-lang.org/dist/2016-05-24/channel-rust-1.9.0.toml` +/// `static.rust-lang.org/dist/2016-05-31/channel-rust-nightly.toml` +/// `static.rust-lang.org/dist/2016-06-01/channel-rust-beta.toml` +/// `static.rust-lang.org/dist/2025-06-26/channel-rust-1.89-beta.toml` +/// `static.rust-lang.org/dist/2025-06-26/channel-rust-1.89.0-beta.toml` +/// `static.rust-lang.org/dist/2025-06-26/channel-rust-1.89.0-beta.2.toml` +pub fn parse_release_string(url: &str) -> Option<(String, DateTime)> { + static VERSION_RE: LazyLock = LazyLock::new(|| Regex::new(r"(\d+\.\d+\.\d+)").unwrap()); + + // Grab ".../YYYY-MM-DD/FILE.toml" components with Path helpers. + let file = Path::new(url).file_name().and_then(|n| n.to_str())?; + + let date_str = Path::new(url) + .parent() + .and_then(Path::file_name) + .and_then(|n| n.to_str())?; + + // No other beta releases are recognized as toolchains. + // + // We also have names like this: + // + // * channel-rust-1.75-beta.toml + // * channel-rust-1.75.0-beta.toml + // * channel-rust-1.75.0-beta.1.toml + // + // Which should get ignored for now, they're not consumable via rustup yet. + if file.contains("beta") && file != "channel-rust-beta.toml" { + return None; + } + + // Parse the YYYY-MM-DD segment and stamp it with *current* UTC time. + if let Ok(naive) = NaiveDate::parse_from_str(date_str, "%Y-%m-%d") { + let published = naive + .and_hms_opt(0, 0, 0) + .expect("valid HMS") + .and_local_timezone(Utc) + .single() + .unwrap(); + + // Special-case the rolling beta channel. + if file == "channel-rust-beta.toml" { + return Some((format!("beta-{date_str}"), published)); + } + + // Otherwise pull out a semver like "1.70.0" and return it. + if let Some(cap) = VERSION_RE.captures(file).and_then(|m| m.get(1)) { + return Some((cap.as_str().to_owned(), published)); + } + } + + None +} + +#[cfg(test)] +mod tests { + use super::*; + use chrono::NaiveDate; + + /// Helper: unwrap the Option, panic otherwise. + fn tag(url: &str) -> String { + parse_release_string(url) + .expect("Some") // Option<_> + .0 // take the tag + } + + /// Helper: unwrap the DateTime and keep only the YYYY-MM-DD part + fn day(url: &str) -> NaiveDate { + parse_release_string(url).expect("Some").1.date_naive() + } + + #[test] + fn parses_stable_versions() { + assert_eq!( + tag("static.rust-lang.org/dist/2016-05-24/channel-rust-1.9.0.toml"), + "1.9.0" + ); + assert_eq!( + day("static.rust-lang.org/dist/2016-05-24/channel-rust-1.9.0.toml"), + NaiveDate::from_ymd_opt(2016, 5, 24).unwrap() + ); + + assert_eq!( + tag("static.rust-lang.org/dist/2025-06-26/channel-rust-1.88.0.toml"), + "1.88.0" + ); + assert_eq!( + day("static.rust-lang.org/dist/2025-06-26/channel-rust-1.88.0.toml"), + NaiveDate::from_ymd_opt(2025, 6, 26).unwrap() + ); + } + + #[test] + fn parses_plain_beta_channel() { + let want = "beta-2016-06-01"; + let url = "static.rust-lang.org/dist/2016-06-01/channel-rust-beta.toml"; + + assert_eq!(tag(url), want); + assert_eq!(day(url), NaiveDate::from_ymd_opt(2016, 6, 1).unwrap()); + } + + #[test] + fn skips_unconsumable_channels() { + // nightly never returns Anything + assert!(parse_release_string( + "static.rust-lang.org/dist/2016-05-31/channel-rust-nightly.toml" + ) + .is_none()); + + // versioned-beta artefacts are skipped too + for should_ignore in [ + "static.rust-lang.org/dist/2025-06-26/channel-rust-1.89-beta.toml", + "static.rust-lang.org/dist/2025-06-26/channel-rust-1.89.0-beta.toml", + "static.rust-lang.org/dist/2025-06-26/channel-rust-1.89.0-beta.2.toml", + ] { + assert!( + parse_release_string(should_ignore).is_none(), + "{should_ignore} should be ignored" + ); + } + } +} From f401ebe98d1a6152a22a9e29cc67413e03a192f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 17:15:54 +0200 Subject: [PATCH 10/14] Update benchmark requests by tag --- database/src/pool.rs | 7 ++++--- database/src/pool/postgres.rs | 22 ++++++++++++---------- database/src/pool/sqlite.rs | 4 ++-- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/database/src/pool.rs b/database/src/pool.rs index 5008edb62..9560e3666 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -194,10 +194,11 @@ pub trait Connection: Send + Sync { /// been completed yet. Pending statuses are `ArtifactsReady` and `InProgress`. async fn load_pending_benchmark_requests(&self) -> anyhow::Result>; - /// Update the status of a `benchmark_request` + /// Update the status of a `benchmark_request` with the given `tag`. + /// If no such request exists in the DB, returns an error. async fn update_benchmark_request_status( - &mut self, - request: &BenchmarkRequest, + &self, + tag: &str, status: BenchmarkRequestStatus, ) -> anyhow::Result<()>; diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index dff72b8d7..44f29ff02 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1452,17 +1452,14 @@ where } async fn update_benchmark_request_status( - &mut self, - request: &BenchmarkRequest, + &self, + tag: &str, status: BenchmarkRequestStatus, ) -> anyhow::Result<()> { - let tag = request - .tag() - .expect("Cannot update status of a benchmark request without a tag. Use `attach_shas_to_try_benchmark_request` instead."); - let status_str = status.as_str(); let completed_at = status.completed_at(); - self.conn() + let modified_rows = self + .conn() .execute( r#" UPDATE benchmark_request @@ -1471,9 +1468,14 @@ where &[&status_str, &completed_at, &tag], ) .await - .context("failed to benchmark request status update")?; - - Ok(()) + .context("failed to update benchmark request status")?; + if modified_rows == 0 { + Err(anyhow::anyhow!( + "Could not update status of benchmark request with tag `{tag}`, it was not found." + )) + } else { + Ok(()) + } } async fn attach_shas_to_try_benchmark_request( diff --git a/database/src/pool/sqlite.rs b/database/src/pool/sqlite.rs index fa5c42c81..ecc28484f 100644 --- a/database/src/pool/sqlite.rs +++ b/database/src/pool/sqlite.rs @@ -1281,8 +1281,8 @@ impl Connection for SqliteConnection { } async fn update_benchmark_request_status( - &mut self, - _request: &BenchmarkRequest, + &self, + _tag: &str, _status: BenchmarkRequestStatus, ) -> anyhow::Result<()> { no_queue_implementation_abort!() From d30174e19ae7e0f2f00ef415da2e53225fd2f4b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 17:27:45 +0200 Subject: [PATCH 11/14] Fix loading of benchmark request index --- database/src/pool/postgres.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 44f29ff02..b8079f5a7 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -574,8 +574,9 @@ impl PostgresConnection { where aid = $1 ").await.unwrap(), load_benchmark_request_index: conn.prepare(" - select tag, status - from benchmark_request + SELECT tag, status + FROM benchmark_request + WHERE tag IS NOT NULL ").await.unwrap(), }), conn, @@ -1506,7 +1507,7 @@ where ], ) .await - .context("failed to execute UPDATE benchmark_request")?; + .context("failed to attach SHAs to try benchmark request")?; Ok(()) } @@ -1525,7 +1526,7 @@ where backends, profiles FROM benchmark_request - WHERE status = ANY('{BENCHMARK_REQUEST_STATUS_ARTIFACTS_READY_STR}', '{BENCHMARK_REQUEST_STATUS_IN_PROGRESS_STR}')"# + WHERE status IN('{BENCHMARK_REQUEST_STATUS_ARTIFACTS_READY_STR}', '{BENCHMARK_REQUEST_STATUS_IN_PROGRESS_STR}')"# ); let rows = self From 57714993b14979b3b7598e1339a57c23a8b2d16c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 17:34:03 +0200 Subject: [PATCH 12/14] Update tests --- database/src/pool.rs | 301 ++++++++++++++------------------------ site/src/job_queue/mod.rs | 120 +++++++-------- 2 files changed, 162 insertions(+), 259 deletions(-) diff --git a/database/src/pool.rs b/database/src/pool.rs index 9560e3666..ac2fe31ac 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -335,7 +335,7 @@ mod tests { use super::*; use crate::{ tests::{run_db_test, run_postgres_test}, - BenchmarkRequestStatus, Commit, CommitType, Date, + BenchmarkRequestStatus, BenchmarkRequestType, Commit, CommitType, Date, }; /// Create a Commit @@ -406,153 +406,93 @@ mod tests { .await; } + // Check that we can't have multiple requests with the same SHA #[tokio::test] - async fn insert_benchmark_requests() { + async fn multiple_requests_same_sha() { run_postgres_test(|ctx| async { let db = ctx.db_client(); - let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); - let master_benchmark_request = BenchmarkRequest::create_master( + let db = db.connection().await; + db.insert_benchmark_request(&BenchmarkRequest::create_master( "a-sha-1", "parent-sha-1", 42, - time, - BenchmarkRequestStatus::ArtifactsReady, - "llvm", - "", - ); - - let try_benchmark_request = BenchmarkRequest::create_try_without_artifacts( - Some("b-sha-2"), - Some("parent-sha-2"), - 32, - time, - BenchmarkRequestStatus::ArtifactsReady, - "cranelift", - "", - ); - - let release_benchmark_request = BenchmarkRequest::create_release( - "1.8.0", - time, - BenchmarkRequestStatus::ArtifactsReady, - ); + Utc::now(), + )) + .await + .unwrap(); - let db = db.connection().await; - db.insert_benchmark_request(&master_benchmark_request) + db.insert_benchmark_request(&BenchmarkRequest::create_release("a-sha-1", Utc::now())) .await - .unwrap(); - db.insert_benchmark_request(&try_benchmark_request) - .await - .unwrap(); - db.insert_benchmark_request(&release_benchmark_request) - .await - .unwrap(); - // duplicate insert - assert!(db - .insert_benchmark_request(&master_benchmark_request) - .await - .is_err()); + .expect_err("it was possible to insert a second commit with the same SHA"); Ok(ctx) }) .await; } + // Check that we can't have multiple non-completed try requests on the same PR #[tokio::test] - async fn get_benchmark_requests_by_status() { - // Ensure we get back the requests matching the status with no date - // limit + async fn multiple_non_completed_try_requests() { run_postgres_test(|ctx| async { let db = ctx.db_client(); - let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); - let master_benchmark_request = BenchmarkRequest::create_master( - "a-sha-1", - "parent-sha-1", - 42, - time, - BenchmarkRequestStatus::ArtifactsReady, - "llvm", - "", - ); - - let try_benchmark_request = BenchmarkRequest::create_try_without_artifacts( - Some("b-sha-2"), - Some("parent-sha-2"), - 32, - time, - BenchmarkRequestStatus::Completed, - "cranelift", - "", - ); + let db = db.connection().await; - let release_benchmark_request = BenchmarkRequest::create_release( - "1.8.0", - time, - BenchmarkRequestStatus::ArtifactsReady, - ); + // Completed + let req_a = BenchmarkRequest::create_try_without_artifacts(42, Utc::now(), "", ""); + // WaitingForArtifacts + let req_b = BenchmarkRequest::create_try_without_artifacts(42, Utc::now(), "", ""); + let req_c = BenchmarkRequest::create_try_without_artifacts(42, Utc::now(), "", ""); - let db = db.connection().await; - db.insert_benchmark_request(&master_benchmark_request) - .await - .unwrap(); - db.insert_benchmark_request(&try_benchmark_request) - .await - .unwrap(); - db.insert_benchmark_request(&release_benchmark_request) + db.insert_benchmark_request(&req_a).await.unwrap(); + db.attach_shas_to_try_benchmark_request(42, "sha1", "sha-parent-1") .await .unwrap(); - let requests = db - .load_benchmark_request_index(&[BenchmarkRequestStatus::ArtifactsReady]) - .await - .unwrap(); + db.update_benchmark_request_status( + "sha1", + BenchmarkRequestStatus::Completed { + completed_at: Utc::now(), + }, + ) + .await + .unwrap(); - assert_eq!(requests.len(), 2); - assert_eq!(requests[0].status, BenchmarkRequestStatus::ArtifactsReady); - assert_eq!(requests[1].status, BenchmarkRequestStatus::ArtifactsReady); + // This should be fine, req_a was completed + db.insert_benchmark_request(&req_b).await.unwrap(); + // This should fail, we can't have two queued requests at once + db.insert_benchmark_request(&req_c).await.expect_err( + "It was possible to record two try benchmark requests without artifacts", + ); Ok(ctx) }) .await; } + // Check that we can't have multiple master requests on the same PR #[tokio::test] - async fn update_benchmark_request_status() { - // Insert one item into the database, change the status and then - // get the item back out again to ensure it has changed status + async fn multiple_master_requests_same_pr() { run_postgres_test(|ctx| async { let db = ctx.db_client(); - let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); - let master_benchmark_request = BenchmarkRequest::create_master( + let db = db.connection().await; + + db.insert_benchmark_request(&BenchmarkRequest::create_master( "a-sha-1", "parent-sha-1", 42, - time, - BenchmarkRequestStatus::ArtifactsReady, - "llvm", - "", - ); - - let mut db = db.connection().await; - db.insert_benchmark_request(&master_benchmark_request) - .await - .unwrap(); - - db.update_benchmark_request_status( - &master_benchmark_request, - BenchmarkRequestStatus::InProgress, - ) + Utc::now(), + )) .await .unwrap(); - let requests = db - .load_benchmark_request_index(&[BenchmarkRequestStatus::InProgress]) - .await - .unwrap(); - - assert_eq!(requests.len(), 1); - assert_eq!(requests[0].tag(), master_benchmark_request.tag()); - assert_eq!(requests[0].status, BenchmarkRequestStatus::InProgress); + db.insert_benchmark_request(&BenchmarkRequest::create_master( + "a-sha-2", + "parent-sha-2", + 42, + Utc::now(), + )) + .await + .expect_err("it was possible to insert a second master commit on the same PR"); Ok(ctx) }) @@ -560,37 +500,45 @@ mod tests { } #[tokio::test] - async fn updating_try_commits() { + async fn load_pending_benchmark_requests() { run_postgres_test(|ctx| async { let db = ctx.db_client(); - let db = db.connection().await; let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); - let pr = 42; - - let try_benchmark_request = BenchmarkRequest::create_try_without_artifacts( - None, - None, - pr, - time, - BenchmarkRequestStatus::WaitingForArtifacts, - "cranelift", - "", - ); - db.insert_benchmark_request(&try_benchmark_request) - .await - .unwrap(); - db.attach_shas_to_try_benchmark_request(pr, "foo", "bar") - .await - .unwrap(); - let requests = db - .load_benchmark_request_index(&[BenchmarkRequestStatus::ArtifactsReady]) + + // ArtifactsReady + let req_a = BenchmarkRequest::create_master("sha-1", "parent-sha-1", 42, time); + // ArtifactsReady + let req_b = BenchmarkRequest::create_release("1.80.0", time); + // WaitingForArtifacts + let req_c = BenchmarkRequest::create_try_without_artifacts(50, time, "", ""); + // InProgress + let req_d = BenchmarkRequest::create_master("sha-2", "parent-sha-2", 51, time); + // Completed + let req_e = BenchmarkRequest::create_master("sha-3", "parent-sha-3", 52, time); + + let db = db.connection().await; + for &req in &[&req_a, &req_b, &req_c, &req_d, &req_e] { + db.insert_benchmark_request(req).await.unwrap(); + } + + db.update_benchmark_request_status("sha-2", BenchmarkRequestStatus::InProgress) .await .unwrap(); + db.update_benchmark_request_status( + "sha-3", + BenchmarkRequestStatus::Completed { + completed_at: Utc::now(), + }, + ) + .await + .unwrap(); - assert_eq!(requests.len(), 1); - assert_eq!(requests[0].tag(), Some("foo")); - assert_eq!(requests[0].parent_sha(), Some("bar")); - assert_eq!(requests[0].status, BenchmarkRequestStatus::ArtifactsReady); + let requests = db.load_pending_benchmark_requests().await.unwrap(); + + assert_eq!(requests.len(), 3); + for req in &[req_a, req_b, req_d] { + assert!(requests.iter().any(|r| r.tag() == req.tag())); + } Ok(ctx) }) @@ -598,72 +546,39 @@ mod tests { } #[tokio::test] - async fn adding_try_commit_to_completed_request() { + async fn attach_shas_to_try_benchmark_request() { run_postgres_test(|ctx| async { let db = ctx.db_client(); let db = db.connection().await; - let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); - let pr = 42; - - let completed_try = BenchmarkRequest::create_try_without_artifacts( - Some("sha-2"), - Some("p-sha-1"), - pr, - time, - BenchmarkRequestStatus::Completed, - "cranelift", - "", - ); - db.insert_benchmark_request(&completed_try).await.unwrap(); - - let try_benchmark_request = BenchmarkRequest::create_try_without_artifacts( - None, - None, - pr, - time, - BenchmarkRequestStatus::WaitingForArtifacts, - "cranelift", - "", - ); - // deliberately insert twice - db.insert_benchmark_request(&try_benchmark_request) - .await - .unwrap(); - // this one should fail - assert!(db - .insert_benchmark_request(&try_benchmark_request) - .await - .is_err()); - db.attach_shas_to_try_benchmark_request(pr, "foo", "bar") + + let req = BenchmarkRequest::create_try_without_artifacts(42, Utc::now(), "", ""); + + db.insert_benchmark_request(&req).await.unwrap(); + db.attach_shas_to_try_benchmark_request(42, "sha1", "sha-parent-1") .await .unwrap(); - let requests = db - .load_benchmark_request_index(&[ - BenchmarkRequestStatus::WaitingForArtifacts, - BenchmarkRequestStatus::ArtifactsReady, - BenchmarkRequestStatus::InProgress, - BenchmarkRequestStatus::Completed, - ]) + let req_db = db + .load_pending_benchmark_requests() .await + .unwrap() + .into_iter() + .next() .unwrap(); - - assert_eq!(requests.len(), 2); - let completed_try = requests - .iter() - .find(|req| req.status == BenchmarkRequestStatus::Completed); - assert!(completed_try.is_some()); - assert_eq!(completed_try.unwrap().pr(), Some(&pr)); - assert_eq!(completed_try.unwrap().tag(), Some("sha-2")); - assert_eq!(completed_try.unwrap().parent_sha(), Some("p-sha-1")); - - let artifacts_ready_try = requests - .iter() - .find(|req| req.status == BenchmarkRequestStatus::ArtifactsReady); - assert!(artifacts_ready_try.is_some()); - assert_eq!(artifacts_ready_try.unwrap().pr(), Some(&pr)); - assert_eq!(artifacts_ready_try.unwrap().tag(), Some("foo")); - assert_eq!(artifacts_ready_try.unwrap().parent_sha(), Some("bar")); + assert_eq!(req.backends, req_db.backends); + assert_eq!(req.profiles, req_db.profiles); + assert!(matches!( + req_db.status, + BenchmarkRequestStatus::ArtifactsReady + )); + assert!(matches!( + req_db.commit_type, + BenchmarkRequestType::Try { .. } + )); + + assert_eq!(req_db.tag().as_deref(), Some("sha1")); + assert_eq!(req_db.parent_sha().as_deref(), Some("sha-parent-1")); + assert_eq!(req_db.pr(), Some(&42)); Ok(ctx) }) diff --git a/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index d435eae06..348caf118 100644 --- a/site/src/job_queue/mod.rs +++ b/site/src/job_queue/mod.rs @@ -144,7 +144,7 @@ fn sort_benchmark_requests(index: &BenchmarkRequestIndex, request_queue: &mut [B /// /// Does not consider requests that are waiting for artifacts or that are alredy completed. pub async fn build_queue( - conn: &mut dyn database::pool::Connection, + conn: &dyn database::pool::Connection, index: &BenchmarkRequestIndex, ) -> anyhow::Result> { // Load ArtifactsReady and InProgress benchmark requests @@ -176,7 +176,7 @@ pub async fn build_queue( /// Enqueue the job into the job_queue async fn enqueue_next_job( - conn: &mut dyn database::pool::Connection, + conn: &dyn database::pool::Connection, index: &mut BenchmarkRequestIndex, ) -> anyhow::Result<()> { let _queue = build_queue(conn, index).await?; @@ -185,13 +185,13 @@ async fn enqueue_next_job( /// For queueing jobs, add the jobs you want to queue to this function async fn cron_enqueue_jobs(site_ctxt: &Arc) -> anyhow::Result<()> { - let mut conn = site_ctxt.conn().await; + let conn = site_ctxt.conn().await; let mut index = conn.load_benchmark_request_index().await?; // Put the master commits into the `benchmark_requests` queue create_benchmark_request_master_commits(site_ctxt, &*conn, &index).await?; // Put the releases into the `benchmark_requests` queue create_benchmark_request_releases(&*conn, &index).await?; - enqueue_next_job(&mut *conn, &mut index).await?; + enqueue_next_job(&*conn, &mut index).await?; Ok(()) } @@ -243,35 +243,15 @@ mod tests { } fn create_master(sha: &str, parent: &str, pr: u32, age_days: &str) -> BenchmarkRequest { - BenchmarkRequest::create_master( - sha, - parent, - pr, - days_ago(age_days), - BenchmarkRequestStatus::ArtifactsReady, - "", - "", - ) + BenchmarkRequest::create_master(sha, parent, pr, days_ago(age_days)) } - fn create_try(sha: &str, parent: &str, pr: u32, age_days: &str) -> BenchmarkRequest { - BenchmarkRequest::create_try_without_artifacts( - Some(sha), - Some(parent), - pr, - days_ago(age_days), - BenchmarkRequestStatus::ArtifactsReady, - "", - "", - ) + fn create_try(pr: u32, age_days: &str) -> BenchmarkRequest { + BenchmarkRequest::create_try_without_artifacts(pr, days_ago(age_days), "", "") } fn create_release(tag: &str, age_days: &str) -> BenchmarkRequest { - BenchmarkRequest::create_release( - tag, - days_ago(age_days), - BenchmarkRequestStatus::ArtifactsReady, - ) + BenchmarkRequest::create_release(tag, days_ago(age_days)) } async fn db_insert_requests( @@ -283,6 +263,18 @@ mod tests { } } + async fn mark_as_completed(conn: &dyn database::pool::Connection, shas: &[&str]) { + let completed_at = Utc::now(); + for sha in shas { + conn.update_benchmark_request_status( + sha, + BenchmarkRequestStatus::Completed { completed_at }, + ) + .await + .unwrap(); + } + } + fn queue_order_matches(queue: &[BenchmarkRequest], expected: &[&str]) { let queue_shas: Vec<&str> = queue .iter() @@ -291,17 +283,6 @@ mod tests { assert_eq!(queue_shas, expected) } - trait BenchmarkRequestExt { - fn with_status(self, status: BenchmarkRequestStatus) -> Self; - } - - impl BenchmarkRequestExt for BenchmarkRequest { - fn with_status(mut self, status: BenchmarkRequestStatus) -> Self { - self.status = status; - self - } - } - #[tokio::test] async fn queue_ordering() { run_postgres_test(|ctx| async { @@ -324,9 +305,9 @@ mod tests { * * * 1: Currently `in_progress` - * +-----------+ +---------------+ - * | m "rrr" C | -----+--->| t "t1" IP pr1 | - * +-----------+ +---------------+ + * +-----------+ +-----------------+ + * | m "rrr" C | -----+--->| t "try1" IP pr6 | + * +-----------+ +-----------------+ * * * @@ -336,7 +317,7 @@ mod tests { * | * V * +----------------+ - * | m "mmm" R pr88 | 5: a master commit + * | m "mmm" R pr18 | 5: a master commit * +----------------+ * * +-----------+ @@ -345,7 +326,7 @@ mod tests { * | * V * +----------------+ - * | m "123" R pr11 | 3: a master commit, high pr number + * | m "123" R pr14 | 3: a master commit, high PR number * +----------------+ * * @@ -355,40 +336,47 @@ mod tests { * | * V * +----------------+ - * | m "foo" R pr77 | 4: a master commit + * | m "foo" R pr15 | 4: a master commit * +----------------+ * | * V - * +---------------+ - * | t "baz" R pr4 | 6: a try with a low pr, blocked by parent - * +---------------+ - * - * The master commits should take priority, then "yee" followed - * by "baz" + * +----------------+ + * | t "baz" R pr17 | 6: a try with a low PR, blocked by parent + * +----------------+ **/ - let mut db = ctx.db_client().connection().await; + let db = ctx.db_client().connection().await; let requests = vec![ - create_master("foo", "bar", 77, "days2"), - create_master("123", "345", 11, "days2"), - create_try("baz", "foo", 4, "days1"), - create_release("v.1.2.3", "days2"), - create_try("t1", "rrr", 1, "days1").with_status(BenchmarkRequestStatus::InProgress), - create_master("mmm", "aaa", 88, "days2"), + create_master("bar", "parent1", 10, "days2"), + create_master("345", "parent2", 11, "days2"), + create_master("aaa", "parent3", 12, "days2"), + create_master("rrr", "parent4", 13, "days2"), + create_master("123", "bar", 14, "days2"), + create_master("foo", "345", 15, "days2"), + create_try(16, "days1"), + create_release("v1.2.3", "days2"), + create_try(17, "days1"), + create_master("mmm", "aaa", 18, "days2"), ]; db_insert_requests(&*db, &requests).await; + db.attach_shas_to_try_benchmark_request(16, "try1", "rrr") + .await + .unwrap(); + db.update_benchmark_request_status("try1", BenchmarkRequestStatus::InProgress) + .await + .unwrap(); + db.attach_shas_to_try_benchmark_request(17, "baz", "foo") + .await + .unwrap(); + + mark_as_completed(&*db, &["bar", "345", "aaa", "rrr"]).await; - let completed: HashSet = HashSet::from([ - "bar".to_string(), - "345".to_string(), - "rrr".to_string(), - "aaa".to_string(), - ]); + let index = db.load_benchmark_request_index().await.unwrap(); - let sorted: Vec = build_queue(&mut *db, &completed).await.unwrap(); + let sorted: Vec = build_queue(&*db, &index).await.unwrap(); - queue_order_matches(&sorted, &["t1", "v.1.2.3", "123", "foo", "mmm", "baz"]); + queue_order_matches(&sorted, &["try1", "v1.2.3", "123", "foo", "mmm", "baz"]); Ok(ctx) }) .await; From ce323afb132e2073ca00b43668aaa0387f050bc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Jul 2025 17:55:50 +0200 Subject: [PATCH 13/14] Fix Clippy --- site/src/job_queue/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index 348caf118..43ac10986 100644 --- a/site/src/job_queue/mod.rs +++ b/site/src/job_queue/mod.rs @@ -156,7 +156,7 @@ pub async fn build_queue( }); // We sort the in-progress ones based on the started date - queue.sort_unstable_by(|a, b| a.created_at().cmp(&b.created_at())); + queue.sort_unstable_by_key(|req| req.created_at()); // Add release artifacts ordered by the release tag (1.87.0 before 1.88.0) and `created_at`. let mut release_artifacts: Vec = From 52a4efa140e730963640d19223be2a67a32f6e14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 12 Jul 2025 11:29:40 +0200 Subject: [PATCH 14/14] Document the schema of `benchmark_request` --- database/schema.md | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/database/schema.md b/database/schema.md index 8fb9d8657..6c8194d7d 100644 --- a/database/schema.md +++ b/database/schema.md @@ -258,3 +258,41 @@ aid benchmark error ---------- --- ----- 1 syn-1.0.89 Failed to compile... ``` + + +## New benchmarking design +We are currently implementing a new design for dispatching benchmarks to collector(s) and storing +them in the database. It will support new use-cases, like backfilling of new benchmarks into a parent +commit and primarily benchmarking with multiple collectors (and multiple hardware architectures) in +parallel. + +The tables below are a part of the new scheme. + +### benchmark_request + +Represents a single request for performing a benchmark collection. Each request can be one of three types: + +* Master: benchmark a merged master commit +* Release: benchmark a published stable or beta compiler toolchain +* Try: benchmark a try build on a PR + +Columns: + +* **id** (`int`): Unique ID. +* **tag** (`text`): Identifier of the compiler toolchain that should be benchmarked. + * Commit SHA for master/try requests or release name (e.g. `1.80.0`) for release requests. + * Can be `NULL` for try requests that were queued for a perf. run, but their compiler artifacts haven't been built yet. +* **parent_sha** (`text`): Parent SHA of the benchmarked commit. + * Can be `NULL` for try requests without compiler artifacts. +* **commit_type** (`text NOT NULL`): One of `master`, `try` or `release`. +* **pr** (`int`): Pull request number associated with the master/try commit. + * `NULL` for release requests. +* **created_at** (`timestamptz NOT NULL`): Datetime when the request was created. +* **completed_at** (`timestamptz`): Datetime when the request was completed. +* **status** (`text NOT NULL`): Current status of the benchmark request. + * `waiting-for-artifacts`: A try request waiting for compiler artifacts. + * `artifacts-ready`: Request that has compiler artifacts ready and can be benchmarked. + * `in-progress`: Request that is currently being benchmarked. + * `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.