Skip to content

Commit 74185e7

Browse files
JamesbarfordJamesbarford
committed
Allow for optional jobs not interfering which benchmark completion
1 parent ab52ea0 commit 74185e7

File tree

6 files changed

+153
-10
lines changed

6 files changed

+153
-10
lines changed

‎database/src/lib.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1150,6 +1150,7 @@ pub struct BenchmarkJob {
11501150
status: BenchmarkJobStatus,
11511151
deque_counter: u32,
11521152
kind: BenchmarkJobKind,
1153+
is_optional: bool,
11531154
}
11541155

11551156
impl BenchmarkJob {
@@ -1201,6 +1202,10 @@ impl BenchmarkJob {
12011202
pub fn kind(&self) -> BenchmarkJobKind {
12021203
self.kind
12031204
}
1205+
1206+
pub fn is_optional(&self) -> bool {
1207+
self.is_optional
1208+
}
12041209
}
12051210

12061211
/// Describes the final state of a job

‎database/src/pool.rs‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,7 @@ pub trait Connection: Send + Sync {
241241

242242
/// Add a benchmark job to the job queue and returns its ID, if it was not
243243
/// already in the DB previously.
244+
#[allow(clippy::too_many_arguments)]
244245
async fn enqueue_benchmark_job(
245246
&self,
246247
request_tag: &str,
@@ -249,6 +250,7 @@ pub trait Connection: Send + Sync {
249250
profile: Profile,
250251
benchmark_set: u32,
251252
kind: BenchmarkJobKind,
253+
is_optional: bool,
252254
) -> JobEnqueueResult;
253255

254256
/// Returns a set of compile-time benchmark test cases that were already computed for the
@@ -747,6 +749,7 @@ mod tests {
747749
Profile::Opt,
748750
0u32,
749751
BenchmarkJobKind::Runtime,
752+
false,
750753
)
751754
.await;
752755
match result {
@@ -906,6 +909,7 @@ mod tests {
906909
Profile::Opt,
907910
1u32,
908911
BenchmarkJobKind::Runtime,
912+
false,
909913
)
910914
.await
911915
{
@@ -1006,6 +1010,7 @@ mod tests {
10061010
Profile::Opt,
10071011
benchmark_set.0,
10081012
BenchmarkJobKind::Runtime,
1013+
false,
10091014
)
10101015
.await
10111016
.unwrap();
@@ -1260,6 +1265,7 @@ mod tests {
12601265
Profile::Check,
12611266
0,
12621267
BenchmarkJobKind::Compiletime,
1268+
false,
12631269
)
12641270
.await
12651271
.unwrap();
@@ -1286,4 +1292,98 @@ mod tests {
12861292
})
12871293
.await;
12881294
}
1295+
1296+
#[tokio::test]
1297+
async fn optional_jobs_should_not_block_benchmark_request_from_being_completed() {
1298+
run_postgres_test(|ctx| async {
1299+
let db = ctx.db();
1300+
let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap();
1301+
let benchmark_set = BenchmarkSet(0u32);
1302+
let tag = "sha-1";
1303+
let collector_name = "collector-1";
1304+
let target = Target::X86_64UnknownLinuxGnu;
1305+
1306+
let insert_result = db
1307+
.add_collector_config(collector_name, target, 1, true)
1308+
.await;
1309+
assert!(insert_result.is_ok());
1310+
1311+
/* Create the request */
1312+
let benchmark_request = BenchmarkRequest::create_release(tag, time);
1313+
db.insert_benchmark_request(&benchmark_request)
1314+
.await
1315+
.unwrap();
1316+
1317+
/* Create job for the request */
1318+
db.enqueue_benchmark_job(
1319+
benchmark_request.tag().unwrap(),
1320+
target,
1321+
CodegenBackend::Llvm,
1322+
Profile::Opt,
1323+
benchmark_set.0,
1324+
BenchmarkJobKind::Runtime,
1325+
false,
1326+
)
1327+
.await
1328+
.unwrap();
1329+
1330+
/* Create optional job for the request, this is somewhat unrealsitic
1331+
* as we're only going to make specific targets jobs optional */
1332+
db.enqueue_benchmark_job(
1333+
benchmark_request.tag().unwrap(),
1334+
target,
1335+
CodegenBackend::Llvm,
1336+
Profile::Doc,
1337+
benchmark_set.0,
1338+
BenchmarkJobKind::Runtime,
1339+
true,
1340+
)
1341+
.await
1342+
.unwrap();
1343+
1344+
let (job, _) = db
1345+
.dequeue_benchmark_job(collector_name, target, benchmark_set)
1346+
.await
1347+
.unwrap()
1348+
.unwrap();
1349+
1350+
assert_eq!(job.request_tag(), benchmark_request.tag().unwrap());
1351+
1352+
/* Make the job take some amount of time */
1353+
std::thread::sleep(Duration::from_millis(1000));
1354+
1355+
/* Mark the job as complete */
1356+
db.mark_benchmark_job_as_completed(job.id(), BenchmarkJobConclusion::Success)
1357+
.await
1358+
.unwrap();
1359+
1360+
db.maybe_mark_benchmark_request_as_completed(tag)
1361+
.await
1362+
.unwrap();
1363+
1364+
/* From the status page view we can see that the duration has been
1365+
* updated. Albeit that it will be a very short duration. */
1366+
let completed = db.get_last_n_completed_benchmark_requests(1).await.unwrap();
1367+
let req = &completed
1368+
.iter()
1369+
.find(|it| it.request.tag() == Some(tag))
1370+
.unwrap()
1371+
.request;
1372+
1373+
assert!(matches!(
1374+
req.status(),
1375+
BenchmarkRequestStatus::Completed { .. }
1376+
));
1377+
let BenchmarkRequestStatus::Completed { duration, .. } = req.status() else {
1378+
unreachable!();
1379+
};
1380+
assert!(duration >= Duration::from_millis(1000));
1381+
1382+
let completed_index = db.load_benchmark_request_index().await.unwrap();
1383+
assert!(completed_index.contains_tag("sha-1"));
1384+
1385+
Ok(ctx)
1386+
})
1387+
.await;
1388+
}
12891389
}

‎database/src/pool/postgres.rs‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,7 @@ static MIGRATIONS: &[&str] = &[
440440
ALTER TABLE runtime_pstat_series DROP CONSTRAINT runtime_pstat_series_benchmark_metric_key;
441441
ALTER TABLE runtime_pstat_series ADD CONSTRAINT runtime_test_case UNIQUE(benchmark, target, metric);
442442
"#,
443+
r#"ALTER TABLE job_queue ADD COLUMN is_optional BOOLEAN NOT NULL DEFAULT FALSE"#,
443444
];
444445

445446
#[async_trait::async_trait]
@@ -1791,6 +1792,7 @@ where
17911792
profile: Profile,
17921793
benchmark_set: u32,
17931794
kind: BenchmarkJobKind,
1795+
is_optional: bool,
17941796
) -> JobEnqueueResult {
17951797
// This will return zero rows if the job already exists
17961798
let result = self
@@ -1804,9 +1806,10 @@ where
18041806
profile,
18051807
benchmark_set,
18061808
status,
1807-
kind
1809+
kind,
1810+
is_optional
18081811
)
1809-
VALUES ($1, $2, $3, $4, $5, $6, $7)
1812+
VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
18101813
ON CONFLICT DO NOTHING
18111814
RETURNING job_queue.id
18121815
"#,
@@ -1818,6 +1821,7 @@ where
18181821
&(benchmark_set as i32),
18191822
&BENCHMARK_JOB_STATUS_QUEUED_STR,
18201823
&kind,
1824+
&is_optional,
18211825
],
18221826
)
18231827
.await;
@@ -1985,11 +1989,12 @@ where
19851989
AND target = $2
19861990
AND benchmark_set = $3
19871991
ORDER BY
1988-
-- Prefer in-progress jobs that have not been finished previously, so that
1989-
-- we can finish them.
1992+
-- Prefer in-progress jobs that have not been finished
1993+
-- previously, so that we can finish them. Also prefer
1994+
-- mandatory jobs over optional ones.
19901995
CASE
1991-
WHEN status = $5 THEN 0
1992-
WHEN status = $1 THEN 1
1996+
WHEN (status = $5 AND is_optional = FALSE) THEN 0
1997+
WHEN (status = $1 AND is_optional = FALSE) THEN 1
19931998
ELSE 2
19941999
END,
19952000
request_tag,
@@ -2019,6 +2024,7 @@ where
20192024
updated.started_at,
20202025
updated.retry,
20212026
updated.kind,
2027+
updated.is_optional,
20222028
br.commit_type,
20232029
br.commit_date
20242030
FROM updated
@@ -2054,9 +2060,10 @@ where
20542060
deque_counter: row.get::<_, i32>(6) as u32,
20552061
kind: BenchmarkJobKind::from_str(row.get::<_, &str>(7))
20562062
.map_err(|e| anyhow::anyhow!(e))?,
2063+
is_optional: row.get::<_, bool>(8),
20572064
};
2058-
let commit_type = row.get::<_, &str>(8);
2059-
let commit_date = row.get::<_, Option<DateTime<Utc>>>(9);
2065+
let commit_type = row.get::<_, &str>(9);
2066+
let commit_date = row.get::<_, Option<DateTime<Utc>>>(10);
20602067

20612068
let commit_date = Date(commit_date.ok_or_else(|| {
20622069
anyhow::anyhow!("Dequeuing job for a benchmark request without commit date")
@@ -2114,6 +2121,7 @@ where
21142121
WHERE
21152122
job_queue.request_tag = benchmark_request.tag
21162123
AND job_queue.status NOT IN ($3, $4)
2124+
AND job_queue.is_optional = FALSE
21172125
)
21182126
AND (
21192127
benchmark_request.parent_sha IS NULL
@@ -2264,6 +2272,7 @@ where
22642272
deque_counter: row.get::<_, i32>(10) as u32,
22652273
kind: BenchmarkJobKind::from_str(row.get::<_, &str>(12))
22662274
.map_err(|e| anyhow::anyhow!(e))?,
2275+
is_optional: row.get::<_, bool>(13),
22672276
};
22682277
request_to_jobs
22692278
.entry(job.request_tag.clone())

‎database/src/pool/sqlite.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1359,6 +1359,7 @@ impl Connection for SqliteConnection {
13591359
_profile: Profile,
13601360
_benchmark_set: u32,
13611361
_kind: BenchmarkJobKind,
1362+
_is_optional: bool,
13621363
) -> JobEnqueueResult {
13631364
no_queue_implementation_abort!()
13641365
}

‎database/src/tests/builder.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ impl RequestBuilder {
4949
job.profile,
5050
job.benchmark_set,
5151
job.kind,
52+
false,
5253
)
5354
.await
5455
{
@@ -116,13 +117,19 @@ pub struct JobBuilder {
116117
benchmark_set: u32,
117118
conclusion: BenchmarkJobConclusion,
118119
kind: BenchmarkJobKind,
120+
is_optional: bool,
119121
}
120122

121123
impl JobBuilder {
122124
pub fn profile(mut self, profile: Profile) -> Self {
123125
self.profile = profile;
124126
self
125127
}
128+
129+
pub fn is_optional(mut self, is_optional: bool) -> Self {
130+
self.is_optional = is_optional;
131+
self
132+
}
126133
}
127134

128135
impl Default for JobBuilder {
@@ -134,6 +141,7 @@ impl Default for JobBuilder {
134141
benchmark_set: 0,
135142
conclusion: BenchmarkJobConclusion::Success,
136143
kind: BenchmarkJobKind::Compiletime,
144+
is_optional: false,
137145
}
138146
}
139147
}

‎site/src/job_queue/mod.rs‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,10 +267,19 @@ pub async fn enqueue_benchmark_request(
267267
profile,
268268
benchmark_set,
269269
kind,
270-
mode: EnqueueMode| {
270+
mode: EnqueueMode,
271+
is_optional: bool| {
271272
let result = tx
272273
.conn()
273-
.enqueue_benchmark_job(request_tag, target, backend, profile, benchmark_set, kind)
274+
.enqueue_benchmark_job(
275+
request_tag,
276+
target,
277+
backend,
278+
profile,
279+
benchmark_set,
280+
kind,
281+
is_optional,
282+
)
274283
.await;
275284

276285
let make_error = |msg| {
@@ -304,6 +313,12 @@ pub async fn enqueue_benchmark_request(
304313

305314
// Target x benchmark_set x backend x profile -> BenchmarkJob
306315
for target in Target::all() {
316+
// We only have X86_64 at the moment and when we add other targets do
317+
// not want to block the Benchmark request from completing to wait on
318+
// other targets. Essentially, for the time being, other targets will
319+
// run in the background
320+
let is_optional = target != Target::X86_64UnknownLinuxGnu;
321+
307322
for benchmark_set in 0..benchmark_set_count(target.into()) {
308323
for &backend in backends.iter() {
309324
for &profile in profiles.iter() {
@@ -316,6 +331,7 @@ pub async fn enqueue_benchmark_request(
316331
benchmark_set as u32,
317332
BenchmarkJobKind::Compiletime,
318333
EnqueueMode::Commit,
334+
is_optional,
319335
)
320336
.await?;
321337

@@ -339,6 +355,7 @@ pub async fn enqueue_benchmark_request(
339355
benchmark_set as u32,
340356
BenchmarkJobKind::Compiletime,
341357
EnqueueMode::Parent,
358+
is_optional,
342359
)
343360
.await?;
344361
}
@@ -357,6 +374,7 @@ pub async fn enqueue_benchmark_request(
357374
0u32,
358375
BenchmarkJobKind::Runtime,
359376
EnqueueMode::Commit,
377+
is_optional,
360378
)
361379
.await?;
362380
}
@@ -374,6 +392,7 @@ pub async fn enqueue_benchmark_request(
374392
0u32,
375393
BenchmarkJobKind::Rustc,
376394
EnqueueMode::Commit,
395+
false,
377396
)
378397
.await?;
379398

@@ -624,6 +643,7 @@ mod tests {
624643
Profile::Opt,
625644
benchmark_set,
626645
BenchmarkJobKind::Compiletime,
646+
false,
627647
)
628648
.await
629649
{

0 commit comments

Comments
 (0)