Merge pull request #2187 from Kobzol/revert-2161

Revert #2161

Author: Kobzol

Cargo.lock

@@ -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"] }

collector/Cargo.toml

@@ -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))

collector/src/bin/collector.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<usize>,
         targets: &[Target],
-        already_computed: &hashbrown::HashSet<CompileTestCase>,
     ) -> 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<Scenario>,
-            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<BenchmarkDir> = 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::<Vec<Scenario>>();
-                    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<ManuallyDrop<TempDir>> = 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<CompileTestCase>,
-    ) -> 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.

collector/src/compile/benchmark/mod.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());

collector/src/compile/execute/bencher.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<CompileTestCase>,
 }
 
 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) {