From 5217c4713d4d992d6f89bee87d696a7a42f44653 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 6 Jul 2025 11:33:03 +0200 Subject: [PATCH] Revert "Merge pull request #2161 from Kobzol/check-test-cases-with-measurements" This reverts commit f521c0a619f1a8093be52799d2570dbae5202417, reversing changes made to b34ad292e43f4bf251eea32ce0db0f5dcb27cf89. --- Cargo.lock | 3 +- collector/Cargo.toml | 1 - collector/src/bin/collector.rs | 12 +- collector/src/compile/benchmark/mod.rs | 144 +++-------------------- collector/src/compile/execute/bencher.rs | 21 ++-- collector/src/lib.rs | 23 ++-- database/src/pool.rs | 84 ++----------- database/src/pool/postgres.rs | 50 ++------ database/src/pool/sqlite.rs | 41 ++----- 9 files changed, 64 insertions(+), 315 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 318433c1d..e8b5d4640 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 4 +version = 3 [[package]] name = "addr2line" @@ -397,7 +397,6 @@ dependencies = [ "env_logger", "flate2", "futures", - "hashbrown", "humansize", "inquire", "jobserver", diff --git a/collector/Cargo.toml b/collector/Cargo.toml index 6fe0e5635..75134a8a2 100644 --- a/collector/Cargo.toml +++ b/collector/Cargo.toml @@ -11,7 +11,6 @@ anyhow = { workspace = true } chrono = { workspace = true, features = ["serde"] } clap = { workspace = true, features = ["derive"] } env_logger = { workspace = true } -hashbrown = { workspace = true } log = { workspace = true } reqwest = { workspace = true, features = ["blocking", "json"] } serde = { workspace = true, features = ["derive"] } diff --git a/collector/src/bin/collector.rs b/collector/src/bin/collector.rs index fffa3ed47..3d53ce885 100644 --- a/collector/src/bin/collector.rs +++ b/collector/src/bin/collector.rs @@ -225,8 +225,6 @@ fn profile_compile( toolchain, Some(1), targets, - // We always want to profile everything - &hashbrown::HashSet::new(), )); eprintln!("Finished benchmark {benchmark_id}"); @@ -1806,8 +1804,11 @@ async fn bench_compile( print_intro: &dyn Fn(), measure: F, ) { - collector.start_compile_step(conn, benchmark_name).await; - + let is_fresh = collector.start_compile_step(conn, benchmark_name).await; + if !is_fresh { + eprintln!("skipping {} -- already benchmarked", benchmark_name); + return; + } let mut tx = conn.transaction().await; let (supports_stable, category) = category.db_representation(); tx.conn() @@ -1818,7 +1819,7 @@ async fn bench_compile( tx.conn(), benchmark_name, &shared.artifact_id, - collector, + collector.artifact_row_id, config.is_self_profile, ); let result = measure(&mut processor).await; @@ -1865,7 +1866,6 @@ async fn bench_compile( &shared.toolchain, config.iterations, &config.targets, - &collector.measured_compile_test_cases, )) .await .with_context(|| anyhow::anyhow!("Cannot compile {}", benchmark.name)) diff --git a/collector/src/compile/benchmark/mod.rs b/collector/src/compile/benchmark/mod.rs index 122af31eb..f8115ce10 100644 --- a/collector/src/compile/benchmark/mod.rs +++ b/collector/src/compile/benchmark/mod.rs @@ -8,7 +8,6 @@ use crate::compile::execute::{CargoProcess, Processor}; use crate::toolchain::Toolchain; use crate::utils::wait_for_future; use anyhow::{bail, Context}; -use database::selector::CompileTestCase; use log::debug; use std::collections::{HashMap, HashSet}; use std::fmt::{Display, Formatter}; @@ -244,7 +243,6 @@ impl Benchmark { toolchain: &Toolchain, iterations: Option, targets: &[Target], - already_computed: &hashbrown::HashSet, ) -> anyhow::Result<()> { if self.config.disabled { eprintln!("Skipping {}: disabled", self.name); @@ -275,65 +273,19 @@ impl Benchmark { return Ok(()); } - struct BenchmarkDir { - dir: TempDir, - scenarios: Vec, - profile: Profile, - backend: CodegenBackend, - target: Target, - } - - // Materialize the test cases that we want to benchmark - // We need to handle scenarios a bit specially, because they share the target directory - let mut benchmark_dirs: Vec = vec![]; - + eprintln!("Preparing {}", self.name); + let mut target_dirs: Vec<((CodegenBackend, Profile, Target), TempDir)> = vec![]; for backend in backends { for profile in &profiles { for target in targets { - // Do we have any scenarios left to compute? - let remaining_scenarios = scenarios - .iter() - .filter(|scenario| { - self.should_run_scenario( - scenario, - profile, - backend, - target, - already_computed, - ) - }) - .copied() - .collect::>(); - if remaining_scenarios.is_empty() { - continue; - } - - let temp_dir = self.make_temp_dir(&self.path)?; - benchmark_dirs.push(BenchmarkDir { - dir: temp_dir, - scenarios: remaining_scenarios, - profile: *profile, - backend: *backend, - target: *target, - }); + target_dirs.push(( + (*backend, *profile, *target), + self.make_temp_dir(&self.path)?, + )); } } } - if benchmark_dirs.is_empty() { - eprintln!( - "Skipping {}: all test cases were previously computed", - self.name - ); - return Ok(()); - } - - eprintln!( - "Preparing {} (test cases: {})", - self.name, - benchmark_dirs.len() - ); - // In parallel (but with a limit to the number of CPUs), prepare all // profiles. This is done in parallel vs. sequentially because: // * We don't record any measurements during this phase, so the @@ -367,18 +319,18 @@ impl Benchmark { .get(), ) .context("jobserver::new")?; - let mut threads = Vec::with_capacity(benchmark_dirs.len()); - for benchmark_dir in &benchmark_dirs { + let mut threads = Vec::with_capacity(target_dirs.len()); + for ((backend, profile, target), prep_dir) in &target_dirs { let server = server.clone(); let thread = s.spawn::<_, anyhow::Result<()>>(move || { wait_for_future(async move { let server = server.clone(); self.mk_cargo_process( toolchain, - benchmark_dir.dir.path(), - benchmark_dir.profile, - benchmark_dir.backend, - benchmark_dir.target, + prep_dir.path(), + *profile, + *backend, + *target, ) .jobserver(server) .run_rustc(false) @@ -413,11 +365,10 @@ impl Benchmark { let mut timing_dirs: Vec> = vec![]; let benchmark_start = std::time::Instant::now(); - for benchmark_dir in &benchmark_dirs { - let backend = benchmark_dir.backend; - let profile = benchmark_dir.profile; - let target = benchmark_dir.target; - let scenarios = &benchmark_dir.scenarios; + for ((backend, profile, target), prep_dir) in &target_dirs { + let backend = *backend; + let profile = *profile; + let target = *target; eprintln!( "Running {}: {:?} + {:?} + {:?} + {:?}", self.name, profile, scenarios, backend, target, @@ -437,7 +388,7 @@ impl Benchmark { } log::debug!("Benchmark iteration {}/{}", i + 1, iterations); // Don't delete the directory on error. - let timing_dir = ManuallyDrop::new(self.make_temp_dir(benchmark_dir.dir.path())?); + let timing_dir = ManuallyDrop::new(self.make_temp_dir(prep_dir.path())?); let cwd = timing_dir.path(); // A full non-incremental build. @@ -507,67 +458,6 @@ impl Benchmark { Ok(()) } - - /// Return true if the given `scenario` should be computed. - fn should_run_scenario( - &self, - scenario: &Scenario, - profile: &Profile, - backend: &CodegenBackend, - target: &Target, - already_computed: &hashbrown::HashSet, - ) -> bool { - let benchmark = database::Benchmark::from(self.name.0.as_str()); - let profile = match profile { - Profile::Check => database::Profile::Check, - Profile::Debug => database::Profile::Debug, - Profile::Doc => database::Profile::Doc, - Profile::DocJson => database::Profile::DocJson, - Profile::Opt => database::Profile::Opt, - Profile::Clippy => database::Profile::Clippy, - }; - let backend = match backend { - CodegenBackend::Llvm => database::CodegenBackend::Llvm, - CodegenBackend::Cranelift => database::CodegenBackend::Cranelift, - }; - let target = match target { - Target::X86_64UnknownLinuxGnu => database::Target::X86_64UnknownLinuxGnu, - }; - - match scenario { - // For these scenarios, we can simply check if they were benchmarked or not - Scenario::Full | Scenario::IncrFull | Scenario::IncrUnchanged => { - let test_case = CompileTestCase { - benchmark, - profile, - backend, - target, - scenario: match scenario { - Scenario::Full => database::Scenario::Empty, - Scenario::IncrFull => database::Scenario::IncrementalEmpty, - Scenario::IncrUnchanged => database::Scenario::IncrementalFresh, - Scenario::IncrPatched => unreachable!(), - }, - }; - !already_computed.contains(&test_case) - } - // For incr-patched, it is a bit more complicated. - // If there is at least a single uncomputed `IncrPatched`, we need to rerun - // all of them, because they stack on top of one another. - // Note that we don't need to explicitly include `IncrFull` if `IncrPatched` - // is selected, as the benchmark code will always run `IncrFull` before `IncrPatched`. - Scenario::IncrPatched => self.patches.iter().any(|patch| { - let test_case = CompileTestCase { - benchmark, - profile, - scenario: database::Scenario::IncrementalPatch(patch.name), - backend, - target, - }; - !already_computed.contains(&test_case) - }), - } - } } /// Directory containing compile-time benchmarks. diff --git a/collector/src/compile/execute/bencher.rs b/collector/src/compile/execute/bencher.rs index ac19a9ebc..694d78571 100644 --- a/collector/src/compile/execute/bencher.rs +++ b/collector/src/compile/execute/bencher.rs @@ -10,7 +10,6 @@ use crate::compile::execute::{ }; use crate::toolchain::Toolchain; use crate::utils::git::get_rustc_perf_commit; -use crate::CollectorCtx; use anyhow::Context; use database::CollectionId; use futures::stream::FuturesUnordered; @@ -43,7 +42,7 @@ pub struct BenchProcessor<'a> { benchmark: &'a BenchmarkName, conn: &'a mut dyn database::Connection, artifact: &'a database::ArtifactId, - collector_ctx: &'a CollectorCtx, + artifact_row_id: database::ArtifactIdNumber, is_first_collection: bool, is_self_profile: bool, tries: u8, @@ -55,7 +54,7 @@ impl<'a> BenchProcessor<'a> { conn: &'a mut dyn database::Connection, benchmark: &'a BenchmarkName, artifact: &'a database::ArtifactId, - collector_ctx: &'a CollectorCtx, + artifact_row_id: database::ArtifactIdNumber, is_self_profile: bool, ) -> Self { // Check we have `perf` or (`xperf.exe` and `tracelog.exe`) available. @@ -79,7 +78,7 @@ impl<'a> BenchProcessor<'a> { conn, benchmark, artifact, - collector_ctx, + artifact_row_id, is_first_collection: true, is_self_profile, tries: 0, @@ -109,7 +108,7 @@ impl<'a> BenchProcessor<'a> { for (stat, value) in stats.iter() { buf.push(self.conn.record_statistic( collection, - self.collector_ctx.artifact_row_id, + self.artifact_row_id, self.benchmark.0.as_str(), profile, scenario, @@ -124,13 +123,7 @@ impl<'a> BenchProcessor<'a> { } pub async fn measure_rustc(&mut self, toolchain: &Toolchain) -> anyhow::Result<()> { - rustc::measure( - self.conn, - toolchain, - self.artifact, - self.collector_ctx.artifact_row_id, - ) - .await + rustc::measure(self.conn, toolchain, self.artifact, self.artifact_row_id).await } } @@ -259,7 +252,7 @@ impl Processor for BenchProcessor<'_> { .map(|profile| { self.conn.record_raw_self_profile( profile.collection, - self.collector_ctx.artifact_row_id, + self.artifact_row_id, self.benchmark.0.as_str(), profile.profile, profile.scenario, @@ -277,7 +270,7 @@ impl Processor for BenchProcessor<'_> { // FIXME: Record codegen backend in the self profile name let prefix = PathBuf::from("self-profile") - .join(self.collector_ctx.artifact_row_id.0.to_string()) + .join(self.artifact_row_id.0.to_string()) .join(self.benchmark.0.as_str()) .join(profile.profile.to_string()) .join(profile.scenario.to_id()); diff --git a/collector/src/lib.rs b/collector/src/lib.rs index 91210dce6..94f1568da 100644 --- a/collector/src/lib.rs +++ b/collector/src/lib.rs @@ -17,9 +17,7 @@ pub mod utils; use crate::compile::benchmark::{Benchmark, BenchmarkName}; use crate::runtime::{BenchmarkGroup, BenchmarkSuite}; -use database::selector::CompileTestCase; use database::{ArtifactId, ArtifactIdNumber, Connection}; -use hashbrown::HashSet; use process::Stdio; use std::time::{Duration, Instant}; @@ -332,30 +330,23 @@ impl CollectorStepBuilder { tx.commit().await.unwrap(); artifact_row_id }; - // Find out which tests cases were already computed - let measured_compile_test_cases = conn - .get_compile_test_cases_with_measurements(&artifact_row_id) - .await - .expect("cannot fetch measured compile test cases from DB"); - - CollectorCtx { - artifact_row_id, - measured_compile_test_cases, - } + CollectorCtx { artifact_row_id } } } /// Represents an in-progress run for a given artifact. pub struct CollectorCtx { pub artifact_row_id: ArtifactIdNumber, - /// Which tests cases were already computed **before** this collection began? - pub measured_compile_test_cases: HashSet, } impl CollectorCtx { - pub async fn start_compile_step(&self, conn: &dyn Connection, benchmark_name: &BenchmarkName) { + pub async fn start_compile_step( + &self, + conn: &dyn Connection, + benchmark_name: &BenchmarkName, + ) -> bool { conn.collector_start_step(self.artifact_row_id, &benchmark_name.0) - .await; + .await } pub async fn end_compile_step(&self, conn: &dyn Connection, benchmark_name: &BenchmarkName) { diff --git a/database/src/pool.rs b/database/src/pool.rs index 40df7d65c..e71dfffe2 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -1,11 +1,10 @@ -use crate::selector::CompileTestCase; use crate::{ ArtifactCollection, ArtifactId, ArtifactIdNumber, BenchmarkRequest, BenchmarkRequestStatus, CodegenBackend, CompileBenchmark, Target, }; use crate::{CollectionId, Index, Profile, QueuedCommit, Scenario, Step}; use chrono::{DateTime, Utc}; -use hashbrown::{HashMap, HashSet}; +use hashbrown::HashMap; use std::sync::{Arc, Mutex}; use std::time::Duration; use tokio::sync::{OwnedSemaphorePermit, Semaphore}; @@ -185,17 +184,6 @@ pub trait Connection: Send + Sync { /// exists it will be ignored async fn insert_benchmark_request(&self, benchmark_request: &BenchmarkRequest); - /// Returns a set of compile-time benchmark test cases that were already computed for the - /// given artifact. - /// Note that for efficiency reasons, the function only checks if we have at least a single - /// result for a given test case. It does not check if *all* test results from all test - /// iterations were finished. - /// Therefore, the result is an over-approximation. - async fn get_compile_test_cases_with_measurements( - &self, - artifact_row_id: &ArtifactIdNumber, - ) -> 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( @@ -328,13 +316,15 @@ impl Pool { #[cfg(test)] mod tests { - use super::*; - use crate::metric::Metric; - use crate::tests::run_postgres_test; - use crate::{tests::run_db_test, BenchmarkRequestStatus, Commit, CommitType, Date}; use chrono::Utc; use std::str::FromStr; + use super::*; + use crate::{ + tests::{run_db_test, run_postgres_test}, + BenchmarkRequestStatus, Commit, CommitType, Date, + }; + /// Create a Commit fn create_commit(commit_sha: &str, time: chrono::DateTime, r#type: CommitType) -> Commit { Commit { @@ -449,66 +439,6 @@ mod tests { .await; } - #[tokio::test] - async fn get_compile_test_cases_with_data() { - run_db_test(|ctx| async { - let db = ctx.db_client().connection().await; - - let collection = db.collection_id("test").await; - let artifact = db - .artifact_id(&ArtifactId::Commit(create_commit( - "abcdef", - Utc::now(), - CommitType::Try, - ))) - .await; - db.record_compile_benchmark("benchmark", None, "primary".to_string()) - .await; - - db.record_statistic( - collection, - artifact, - "benchmark", - Profile::Check, - Scenario::IncrementalFresh, - CodegenBackend::Llvm, - Target::X86_64UnknownLinuxGnu, - Metric::CacheMisses.as_str(), - 1.0, - ) - .await; - - assert_eq!( - db.get_compile_test_cases_with_measurements(&artifact) - .await - .unwrap(), - HashSet::from([CompileTestCase { - benchmark: "benchmark".into(), - profile: Profile::Check, - scenario: Scenario::IncrementalFresh, - backend: CodegenBackend::Llvm, - target: Target::X86_64UnknownLinuxGnu, - }]) - ); - - let artifact2 = db - .artifact_id(&ArtifactId::Commit(create_commit( - "abcdef2", - Utc::now(), - CommitType::Try, - ))) - .await; - assert!(db - .get_compile_test_cases_with_measurements(&artifact2) - .await - .unwrap() - .is_empty()); - - Ok(ctx) - }) - .await; - } - #[tokio::test] async fn get_benchmark_requests_by_status() { // Ensure we get back the requests matching the status with no date diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index f5a6e1117..e0aa9324d 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1,5 +1,4 @@ use crate::pool::{Connection, ConnectionManager, ManagedConnection, Transaction}; -use crate::selector::CompileTestCase; use crate::{ ArtifactCollection, ArtifactId, ArtifactIdNumber, Benchmark, BenchmarkRequest, BenchmarkRequestStatus, BenchmarkRequestType, CodegenBackend, CollectionId, Commit, CommitType, @@ -7,7 +6,7 @@ use crate::{ }; use anyhow::Context as _; use chrono::{DateTime, TimeZone, Utc}; -use hashbrown::{HashMap, HashSet}; +use hashbrown::HashMap; use native_tls::{Certificate, TlsConnector}; use postgres_native_tls::MakeTlsConnector; use std::str::FromStr; @@ -383,7 +382,6 @@ pub struct CachedStatements { get_runtime_pstat: Statement, record_artifact_size: Statement, get_artifact_size: Statement, - get_compile_test_cases_with_measurements: Statement, } pub struct PostgresTransaction<'a> { @@ -565,16 +563,7 @@ impl PostgresConnection { get_artifact_size: conn.prepare(" select component, size from artifact_size where aid = $1 - ").await.unwrap(), - get_compile_test_cases_with_measurements: conn.prepare(" - SELECT DISTINCT crate, profile, scenario, backend, target - FROM pstat_series - WHERE id IN ( - SELECT DISTINCT series - FROM pstat - WHERE aid = $1 - ) - ").await.unwrap(), + ").await.unwrap() }), conn, } @@ -1111,14 +1100,19 @@ where == 1 } async fn collector_end_step(&self, aid: ArtifactIdNumber, step: &str) { - self.conn() + let did_modify = self + .conn() .execute( "update collector_progress set end_time = statement_timestamp() \ - where aid = $1 and step = $2 and start_time is not null;", + where aid = $1 and step = $2 and start_time is not null and end_time is null;", &[&(aid.0 as i32), &step], ) .await - .unwrap(); + .unwrap() + == 1; + if !did_modify { + log::error!("did not end {} for {:?}", step, aid); + } } async fn collector_remove_step(&self, aid: ArtifactIdNumber, step: &str) { self.conn() @@ -1420,30 +1414,6 @@ where .unwrap(); } - async fn get_compile_test_cases_with_measurements( - &self, - artifact_row_id: &ArtifactIdNumber, - ) -> anyhow::Result> { - let rows = self - .conn() - .query( - &self.statements().get_compile_test_cases_with_measurements, - &[&(artifact_row_id.0 as i32)], - ) - .await - .context("cannot query compile-time test cases with measurements")?; - Ok(rows - .into_iter() - .map(|row| CompileTestCase { - benchmark: Benchmark::from(row.get::<_, &str>(0)), - profile: Profile::from_str(row.get::<_, &str>(1)).unwrap(), - scenario: row.get::<_, &str>(2).parse().unwrap(), - backend: CodegenBackend::from_str(row.get::<_, &str>(3)).unwrap(), - target: Target::from_str(row.get::<_, &str>(4)).unwrap(), - }) - .collect()) - } - async fn get_benchmark_requests_by_status( &self, statuses: &[BenchmarkRequestStatus], diff --git a/database/src/pool/sqlite.rs b/database/src/pool/sqlite.rs index 0d018821c..c60477eb3 100644 --- a/database/src/pool/sqlite.rs +++ b/database/src/pool/sqlite.rs @@ -1,12 +1,11 @@ use crate::pool::{Connection, ConnectionManager, ManagedConnection, Transaction}; -use crate::selector::CompileTestCase; use crate::{ ArtifactCollection, ArtifactId, Benchmark, BenchmarkRequest, BenchmarkRequestStatus, CodegenBackend, CollectionId, Commit, CommitType, CompileBenchmark, Date, Profile, Target, }; use crate::{ArtifactIdNumber, Index, QueuedCommit}; use chrono::{DateTime, TimeZone, Utc}; -use hashbrown::{HashMap, HashSet}; +use hashbrown::HashMap; use rusqlite::params; use rusqlite::OptionalExtension; use std::path::PathBuf; @@ -1061,13 +1060,18 @@ impl Connection for SqliteConnection { } async fn collector_end_step(&self, aid: ArtifactIdNumber, step: &str) { - self.raw_ref() + let did_modify = self + .raw_ref() .execute( "update collector_progress set end = strftime('%s','now') \ - where aid = ? and step = ? and start is not null;", + where aid = ? and step = ? and start is not null and end is null;", params![&aid.0, &step], ) - .unwrap(); + .unwrap() + == 1; + if !did_modify { + log::error!("did not end {} for {:?}", step, aid); + } } async fn collector_remove_step(&self, aid: ArtifactIdNumber, step: &str) { @@ -1278,33 +1282,6 @@ impl Connection for SqliteConnection { ) -> anyhow::Result<()> { no_queue_implementation_abort!() } - - async fn get_compile_test_cases_with_measurements( - &self, - artifact_row_id: &ArtifactIdNumber, - ) -> anyhow::Result> { - Ok(self - .raw_ref() - .prepare_cached( - "SELECT DISTINCT crate, profile, scenario, backend, target - FROM pstat_series - WHERE id IN ( - SELECT DISTINCT series - FROM pstat - WHERE aid = ? - );", - )? - .query_map(params![artifact_row_id.0], |row| { - Ok(CompileTestCase { - benchmark: Benchmark::from(row.get::<_, String>(0)?.as_str()), - profile: row.get::<_, String>(1)?.parse().unwrap(), - scenario: row.get::<_, String>(2)?.parse().unwrap(), - backend: row.get::<_, String>(3)?.parse().unwrap(), - target: row.get::<_, String>(4)?.parse().unwrap(), - }) - })? - .collect::>()?) - } } fn parse_artifact_id(ty: &str, sha: &str, date: Option) -> ArtifactId {