From daf36a4ceb4ab443ad4d7d12a839ee9449a8c8ce Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 10 Jan 2022 11:53:21 +1100 Subject: [PATCH 1/3] Upgrade to clap 3. This requires moving away from the deprecated `clap_app!` macro. Clap's derive API (formerly `StructOpt`) is nicer anyway, because it's not stringly-typed. There is quite a bit of furniture moving in `execute.rs` to better separate the perf tools used for `bench_*` subcommands from those used for the profile subcommands. --- Cargo.lock | 97 +++++- collector/Cargo.toml | 7 +- collector/src/execute.rs | 198 +++++------ collector/src/main.rs | 643 +++++++++++++++++------------------- collector/src/rustc-fake.rs | 58 ++-- 5 files changed, 519 insertions(+), 484 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4986b417b..381b93045 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -220,19 +220,49 @@ dependencies = [ "ansi_term 0.11.0", "atty", "bitflags", - "strsim", - "textwrap", + "strsim 0.8.0", + "textwrap 0.11.0", "unicode-width", "vec_map", ] +[[package]] +name = "clap" +version = "3.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1957aa4a5fb388f0a0a73ce7556c5b42025b874e5cdc2c670775e346e97adec0" +dependencies = [ + "atty", + "bitflags", + "clap_derive", + "indexmap", + "lazy_static", + "os_str_bytes", + "strsim 0.10.0", + "termcolor", + "textwrap 0.14.2", +] + +[[package]] +name = "clap_derive" +version = "3.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "517358c28fcef6607bf6f76108e02afad7e82297d132a6b846dcc1fc3efcd153" +dependencies = [ + "heck", + "proc-macro-error", + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "collector" version = "0.1.0" dependencies = [ "anyhow", "chrono", - "clap", + "clap 3.0.6", "crossbeam-utils", "database", "env_logger", @@ -344,7 +374,7 @@ dependencies = [ "async-trait", "bytes", "chrono", - "clap", + "clap 2.33.3", "csv", "env_logger", "futures", @@ -663,6 +693,12 @@ dependencies = [ "http", ] +[[package]] +name = "heck" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2540771e65fc8cb83cd6e8a237f70c319bd5c29f78ed1084ba5d50eeac86f7f9" + [[package]] name = "hermit-abi" version = "0.1.18" @@ -1161,6 +1197,15 @@ dependencies = [ "vcpkg", ] +[[package]] +name = "os_str_bytes" +version = "6.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e22443d1643a904602595ba1cd8f7d896afe56d26712531c5ff73a15b2fbf64" +dependencies = [ + "memchr", +] + [[package]] name = "output_vt100" version = "0.1.2" @@ -1328,6 +1373,30 @@ dependencies = [ "output_vt100", ] +[[package]] +name = "proc-macro-error" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c" +dependencies = [ + "proc-macro-error-attr", + "proc-macro2", + "quote", + "syn", + "version_check", +] + +[[package]] +name = "proc-macro-error-attr" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869" +dependencies = [ + "proc-macro2", + "quote", + "version_check", +] + [[package]] name = "proc-macro-hack" version = "0.5.19" @@ -1342,9 +1411,9 @@ checksum = "bc881b2c22681370c6a780e47af9840ef841837bc98118431d4e1868bd0c1086" [[package]] name = "proc-macro2" -version = "1.0.27" +version = "1.0.36" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f0d8caf72986c1a598726adc988bb5984792ef84f5ee5aa50209145ee8077038" +checksum = "c7342d5883fbccae1cc37a2353b09c87c9b0f3afd73f5fb9bba687a1f733b029" dependencies = [ "unicode-xid", ] @@ -1818,6 +1887,12 @@ version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a" +[[package]] +name = "strsim" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "73473c0e59e6d5812c5dfe2a064a6444949f089e20eec9a2e5506596494e4623" + [[package]] name = "subtle" version = "2.4.0" @@ -1826,9 +1901,9 @@ checksum = "1e81da0851ada1f3e9d4312c704aa4f8806f0f9d69faaf8df2f3464b4a9437c2" [[package]] name = "syn" -version = "1.0.73" +version = "1.0.85" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f71489ff30030d2ae598524f61326b902466f72a0fb1a8564c001cc63425bcc7" +checksum = "a684ac3dcd8913827e18cd09a68384ee66c1de24157e3c556c9ab16d85695fb7" dependencies = [ "proc-macro2", "quote", @@ -1878,6 +1953,12 @@ dependencies = [ "unicode-width", ] +[[package]] +name = "textwrap" +version = "0.14.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0066c8d12af8b5acd21e00547c3797fde4e8677254a7ee429176ccebbe93dd80" + [[package]] name = "thiserror" version = "1.0.25" diff --git a/collector/Cargo.toml b/collector/Cargo.toml index 3b9e89de1..25fea9fb6 100644 --- a/collector/Cargo.toml +++ b/collector/Cargo.toml @@ -1,11 +1,12 @@ [package] -authors = ["Mark Simulacrum "] +authors = ["The Rust Compiler Team"] name = "collector" version = "0.1.0" -edition = '2018' +edition = "2018" +description = "Collects Rust performance data" [dependencies] -clap = "2.25" +clap = { version = "3.0.6", features = ["derive"] } env_logger = "0.8" anyhow = "1" thiserror = "1" diff --git a/collector/src/execute.rs b/collector/src/execute.rs index 86b94d32a..85f69ea17 100644 --- a/collector/src/execute.rs +++ b/collector/src/execute.rs @@ -1,7 +1,7 @@ //! Execute benchmarks. use crate::{Compiler, ProfileKind, ScenarioKind}; -use anyhow::{anyhow, bail, Context}; +use anyhow::{bail, Context}; use collector::command_output; use collector::etw_parser; use database::{PatchName, QueryLabel}; @@ -163,19 +163,25 @@ pub struct Benchmark { config: BenchmarkConfig, } +// Tools usable with the benchmarking subcommands. #[derive(Clone, Copy, Debug, PartialEq)] -pub enum Profiler { +pub enum Bencher { PerfStat, PerfStatSelfProfile, XperfStat, XperfStatSelfProfile, +} + +// Tools usable with the profiling subcommands, and named on the command line. +#[derive(Clone, Copy, Debug, PartialEq, clap::ArgEnum)] +pub enum Profiler { SelfProfile, TimePasses, PerfRecord, - OProfile, + Oprofile, Cachegrind, Callgrind, - DHAT, + Dhat, Massif, Eprintln, LlvmLines, @@ -184,106 +190,80 @@ pub enum Profiler { LlvmIr, } -impl Profiler { - pub fn from_name(name: &str) -> anyhow::Result { - match name { - // Even though `PerfStat` is a valid `Profiler` value, "perf-stat" - // is rejected because it can't be used with the `profiler` - // subcommand. (It's used with `bench_local` instead.) - "perf-stat" => Err(anyhow!("'perf-stat' cannot be used as the profiler")), - "xperf-stat" => Err(anyhow!("'xperf-stat' cannot be used as the profiler")), - "self-profile" => Ok(Profiler::SelfProfile), - "time-passes" => Ok(Profiler::TimePasses), - "perf-record" => Ok(Profiler::PerfRecord), - "oprofile" => Ok(Profiler::OProfile), - "cachegrind" => Ok(Profiler::Cachegrind), - "callgrind" => Ok(Profiler::Callgrind), - "dhat" => Ok(Profiler::DHAT), - "massif" => Ok(Profiler::Massif), - "eprintln" => Ok(Profiler::Eprintln), - "llvm-lines" => Ok(Profiler::LlvmLines), - "mono-items" => Ok(Profiler::MonoItems), - "dep-graph" => Ok(Profiler::DepGraph), - "llvm-ir" => Ok(Profiler::LlvmIr), - _ => Err(anyhow!("'{}' is not a known profiler", name)), - } - } +#[derive(Clone, Copy, Debug, PartialEq)] +pub enum PerfTool { + Profile(Profiler), + Bench(Bencher), +} - fn name(&self) -> &'static str { +impl PerfTool { + fn name(&self) -> String { match self { - Profiler::PerfStat => "perf-stat", - Profiler::PerfStatSelfProfile => "perf-stat-self-profile", - Profiler::XperfStat => "xperf-stat", - Profiler::XperfStatSelfProfile => "xperf-stat-self-profile", - Profiler::SelfProfile => "self-profile", - Profiler::TimePasses => "time-passes", - Profiler::PerfRecord => "perf-record", - Profiler::OProfile => "oprofile", - Profiler::Cachegrind => "cachegrind", - Profiler::Callgrind => "callgrind", - Profiler::DHAT => "dhat", - Profiler::Massif => "massif", - Profiler::Eprintln => "eprintln", - Profiler::LlvmLines => "llvm-lines", - Profiler::MonoItems => "mono-items", - Profiler::DepGraph => "dep-graph", - Profiler::LlvmIr => "llvm-ir", + PerfTool::Bench(b) => format!("{:?}", b), + PerfTool::Profile(p) => format!("{:?}", p), } } // What cargo subcommand do we need to run for this profiler? If not // `rustc`, must be a subcommand that itself invokes `rustc`. - fn subcommand(&self, profile_kind: ProfileKind) -> Option<&'static str> { + fn cargo_subcommand(&self, profile_kind: ProfileKind) -> Option<&'static str> { + use Bencher::*; + use PerfTool::*; + use Profiler::*; match self { - Profiler::PerfStat - | Profiler::PerfStatSelfProfile - | Profiler::XperfStat - | Profiler::XperfStatSelfProfile - | Profiler::SelfProfile - | Profiler::TimePasses - | Profiler::PerfRecord - | Profiler::OProfile - | Profiler::Cachegrind - | Profiler::Callgrind - | Profiler::DHAT - | Profiler::Massif - | Profiler::DepGraph - | Profiler::MonoItems - | Profiler::LlvmIr - | Profiler::Eprintln => { + Bench(PerfStat) + | Bench(PerfStatSelfProfile) + | Bench(XperfStat) + | Bench(XperfStatSelfProfile) + | Profile(SelfProfile) + | Profile(TimePasses) + | Profile(PerfRecord) + | Profile(Oprofile) + | Profile(Cachegrind) + | Profile(Callgrind) + | Profile(Dhat) + | Profile(Massif) + | Profile(Eprintln) + | Profile(DepGraph) + | Profile(MonoItems) + | Profile(LlvmIr) => { if profile_kind == ProfileKind::Doc { Some("rustdoc") } else { Some("rustc") } } - Profiler::LlvmLines => match profile_kind { + Profile(LlvmLines) => match profile_kind { ProfileKind::Debug | ProfileKind::Opt => Some("llvm-lines"), ProfileKind::Check | ProfileKind::Doc => None, + ProfileKind::All => unreachable!(), }, } } fn is_scenario_kind_allowed(&self, scenario_kind: ScenarioKind) -> bool { + use Bencher::*; + use PerfTool::*; + use Profiler::*; match self { - Profiler::PerfStat - | Profiler::PerfStatSelfProfile - | Profiler::XperfStat - | Profiler::XperfStatSelfProfile - | Profiler::SelfProfile - | Profiler::TimePasses - | Profiler::PerfRecord - | Profiler::OProfile - | Profiler::Cachegrind - | Profiler::Callgrind - | Profiler::DHAT - | Profiler::Massif - | Profiler::MonoItems - | Profiler::LlvmIr - | Profiler::Eprintln => true, + Bench(PerfStat) + | Bench(PerfStatSelfProfile) + | Bench(XperfStat) + | Bench(XperfStatSelfProfile) + | Profile(SelfProfile) + | Profile(TimePasses) + | Profile(PerfRecord) + | Profile(Oprofile) + | Profile(Cachegrind) + | Profile(Callgrind) + | Profile(Dhat) + | Profile(Massif) + | Profile(MonoItems) + | Profile(LlvmIr) + | Profile(Eprintln) => true, // only incremental - Profiler::DepGraph => scenario_kind != ScenarioKind::Full, - Profiler::LlvmLines => scenario_kind == ScenarioKind::Full, + Profile(DepGraph) => scenario_kind != ScenarioKind::Full, + Profile(LlvmLines) => scenario_kind == ScenarioKind::Full, } } } @@ -377,20 +357,20 @@ impl<'a> CargoProcess<'a> { // Get the subcommand. If it's not `rustc` it must should be a // subcommand that itself invokes `rustc` (so that the `FAKE_RUSTC` // machinery works). - let subcommand = + let cargo_subcommand = if let Some((ref mut processor, scenario_kind, ..)) = self.processor_etc { - let profiler = processor.profiler(self.profile_kind); - if !profiler.is_scenario_kind_allowed(scenario_kind) { + let perf_tool = processor.perf_tool(self.profile_kind); + if !perf_tool.is_scenario_kind_allowed(scenario_kind) { return Err(anyhow::anyhow!( - "this profiler doesn't support {:?} scenarios", + "this perf tool doesn't support {:?} scenarios", scenario_kind )); } - match profiler.subcommand(self.profile_kind) { + match perf_tool.cargo_subcommand(self.profile_kind) { None => { return Err(anyhow::anyhow!( - "this profiler doesn't support the {:?} profile", + "this perf tool doesn't support the {:?} profile", self.profile_kind )) } @@ -403,7 +383,7 @@ impl<'a> CargoProcess<'a> { } }; - let mut cmd = self.base_command(self.cwd, subcommand); + let mut cmd = self.base_command(self.cwd, cargo_subcommand); cmd.arg("-p").arg(self.get_pkgid(self.cwd)?); match self.profile_kind { ProfileKind::Check => { @@ -414,6 +394,7 @@ impl<'a> CargoProcess<'a> { ProfileKind::Opt => { cmd.arg("--release"); } + ProfileKind::All => unreachable!(), } cmd.args(&self.cargo_args); if env::var_os("CARGO_RECORD_TIMING").is_some() { @@ -433,13 +414,13 @@ impl<'a> CargoProcess<'a> { .as_mut() .map(|v| &mut v.0) .expect("needs_final needs a processor"); - let profiler = processor.profiler(self.profile_kind).name(); + let perf_tool_name = processor.perf_tool(self.profile_kind).name(); // If we're using a processor, we expect that only the crate // we're interested in benchmarking will be built, not any // dependencies. cmd.env("EXPECT_ONLY_WRAPPED_RUSTC", "1"); cmd.arg("--wrap-rustc-with"); - cmd.arg(profiler); + cmd.arg(perf_tool_name); cmd.args(&self.rustc_args); // If we're not going to be in a processor, then there's no @@ -548,8 +529,8 @@ pub struct ProcessOutputData<'a> { /// Trait used by `Benchmark::measure()` to provide different kinds of /// processing. pub trait Processor { - /// The `Profiler` being used. - fn profiler(&self, _: ProfileKind) -> Profiler; + /// The `PerfTool` being used. + fn perf_tool(&self, _: ProfileKind) -> PerfTool; /// Process the output produced by the particular `Profiler` being used. fn process_output( @@ -648,6 +629,7 @@ impl<'a> BenchProcessor<'a> { ProfileKind::Debug => database::Profile::Debug, ProfileKind::Doc => database::Profile::Doc, ProfileKind::Opt => database::Profile::Opt, + ProfileKind::All => unreachable!(), }; if let Some(files) = stats.2 { @@ -819,18 +801,18 @@ impl Upload { } impl<'a> Processor for BenchProcessor<'a> { - fn profiler(&self, _profile: ProfileKind) -> Profiler { + fn perf_tool(&self, _profile: ProfileKind) -> PerfTool { if self.is_first_collection && self.is_self_profile { if cfg!(unix) { - Profiler::PerfStatSelfProfile + PerfTool::Bench(Bencher::PerfStatSelfProfile) } else { - Profiler::XperfStatSelfProfile + PerfTool::Bench(Bencher::XperfStatSelfProfile) } } else { if cfg!(unix) { - Profiler::PerfStat + PerfTool::Bench(Bencher::PerfStat) } else { - Profiler::XperfStat + PerfTool::Bench(Bencher::XperfStat) } } } @@ -840,10 +822,10 @@ impl<'a> Processor for BenchProcessor<'a> { } fn finished_first_collection(&mut self, profile: ProfileKind) -> bool { - let original = self.profiler(profile); + let original = self.perf_tool(profile); self.is_first_collection = false; - // We need to run again if we're going to use a different profiler - self.profiler(profile) != original + // We need to run again if we're going to use a different perf tool + self.perf_tool(profile) != original } fn process_output( @@ -879,6 +861,7 @@ impl<'a> Processor for BenchProcessor<'a> { res, ); } + ScenarioKind::All => unreachable!(), } Ok(Retry::No) } @@ -922,8 +905,8 @@ impl<'a> ProfileProcessor<'a> { } impl<'a> Processor for ProfileProcessor<'a> { - fn profiler(&self, _: ProfileKind) -> Profiler { - self.profiler + fn perf_tool(&self, _: ProfileKind) -> PerfTool { + PerfTool::Profile(self.profiler) } fn process_output( @@ -949,13 +932,6 @@ impl<'a> Processor for ProfileProcessor<'a> { }; match self.profiler { - Profiler::PerfStat - | Profiler::PerfStatSelfProfile - | Profiler::XperfStat - | Profiler::XperfStatSelfProfile => { - panic!("unexpected profiler"); - } - // -Zself-profile produces (via rustc-fake) a data directory called // `Zsp` containing three files with names of the form // `$BENCHMARK-$PID.{events,string_data,string_index}`. We copy it @@ -1043,7 +1019,7 @@ impl<'a> Processor for ProfileProcessor<'a> { // `oprofile_data`. We copy it from the temp dir to the output dir, // giving it a new name in the process, and then post-process it // twice to produce another two data files in the output dir. - Profiler::OProfile => { + Profiler::Oprofile => { let tmp_opout_dir = filepath(data.cwd.as_ref(), "oprofile_data"); let opout_dir = filepath(self.output_dir, &out_file("opout")); let oprep_file = filepath(self.output_dir, &out_file("oprep")); @@ -1120,7 +1096,7 @@ impl<'a> Processor for ProfileProcessor<'a> { // DHAT produces (via rustc-fake) a data file called `dhout`. We // copy it from the temp dir to the output dir, giving it a new // name in the process. - Profiler::DHAT => { + Profiler::Dhat => { let tmp_dhout_file = filepath(data.cwd.as_ref(), "dhout"); let dhout_file = filepath(self.output_dir, &out_file("dhout")); diff --git a/collector/src/main.rs b/collector/src/main.rs index 6d2e1c927..0136a635b 100644 --- a/collector/src/main.rs +++ b/collector/src/main.rs @@ -1,14 +1,11 @@ #![recursion_limit = "1024"] -#[macro_use] -extern crate clap; - use anyhow::{bail, Context}; +use clap::Parser; use database::{ArtifactId, Commit}; use log::debug; use std::collections::HashSet; use std::fs; -use std::io::{stderr, Write}; use std::path::{Path, PathBuf}; use std::process; use std::process::{Command, Stdio}; @@ -42,12 +39,17 @@ impl<'a> Compiler<'a> { } } -#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, clap::ArgEnum)] +#[clap(rename_all = "PascalCase")] pub enum ProfileKind { Check, Debug, Doc, Opt, + + // This one is only specified from the command line, and is converted to + // one of the above variants by `expand_all()`. + All, } impl ProfileKind { @@ -60,18 +62,30 @@ impl ProfileKind { ] } - fn default() -> Vec { - // Don't run rustdoc by default. + fn all_non_doc() -> Vec { vec![ProfileKind::Check, ProfileKind::Debug, ProfileKind::Opt] } + + fn expand_all(vec: Vec) -> Vec { + if vec.contains(&ProfileKind::All) { + Self::all() + } else { + vec + } + } } -#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, clap::ArgEnum)] +#[clap(rename_all = "PascalCase")] pub enum ScenarioKind { Full, IncrFull, IncrUnchanged, IncrPatched, + + // This one is only specified from the command line, and is converted to + // one of the above variants by `expand_all()`. + All, } impl ScenarioKind { @@ -88,99 +102,23 @@ impl ScenarioKind { vec![ScenarioKind::Full] } + fn expand_all(vec: Vec) -> Vec { + if vec.contains(&ScenarioKind::All) { + Self::all() + } else { + vec + } + } + fn is_incr(self) -> bool { match self { ScenarioKind::Full => false, ScenarioKind::IncrFull | ScenarioKind::IncrUnchanged | ScenarioKind::IncrPatched => { true } + ScenarioKind::All => unreachable!(), } } - - fn default() -> Vec { - Self::all() - } -} - -// How the --builds arg maps to ProfileKinds. -const STRINGS_AND_PROFILE_KINDS: &[(&str, ProfileKind)] = &[ - ("Check", ProfileKind::Check), - ("Debug", ProfileKind::Debug), - ("Doc", ProfileKind::Doc), - ("Opt", ProfileKind::Opt), -]; - -// How the --runs arg maps to ScenarioKinds. -const STRINGS_AND_SCENARIO_KINDS: &[(&str, ScenarioKind)] = &[ - ("Full", ScenarioKind::Full), - ("IncrFull", ScenarioKind::IncrFull), - ("IncrUnchanged", ScenarioKind::IncrUnchanged), - ("IncrPatched", ScenarioKind::IncrPatched), -]; - -fn profile_kinds_from_arg(arg: &Option<&str>) -> anyhow::Result> { - if let Some(arg) = arg { - kinds_from_arg("build", STRINGS_AND_PROFILE_KINDS, arg) - } else { - Ok(ProfileKind::default()) - } -} - -fn scenario_kinds_from_arg(arg: Option<&str>) -> anyhow::Result> { - if let Some(arg) = arg { - kinds_from_arg("run", STRINGS_AND_SCENARIO_KINDS, arg) - } else { - Ok(ScenarioKind::default()) - } -} - -fn iterations_from_arg(arg: Option<&str>) -> anyhow::Result { - if let Some(arg) = arg { - if let Ok(iterations) = usize::from_str_radix(arg, 10) { - Ok(iterations) - } else { - anyhow::bail!( - "cannot parse iteration count '{}'. Must be a decimal number.", - arg - ); - } - } else { - Ok(1) - } -} - -// Converts a comma-separated list of kind names to a vector of kinds with no -// duplicates. -fn kinds_from_arg( - name: &str, - strings_and_kinds: &[(&str, K)], - arg: &str, -) -> anyhow::Result> -where - K: Copy + Eq + ::std::hash::Hash, -{ - let mut kind_set = HashSet::new(); - - for s in arg.split(',') { - if let Some((_s, k)) = strings_and_kinds.iter().find(|(str, _k)| s == *str) { - kind_set.insert(k); - } else if s == "All" { - for (_, k) in strings_and_kinds.iter() { - kind_set.insert(k); - } - } else { - anyhow::bail!("'{}' is not a known {} kind", s, name); - } - } - - // Nb: the element order of `v` must match that of `strings_and_kinds`. - let mut v = vec![]; - for (_s, k) in strings_and_kinds.iter() { - if kind_set.contains(k) { - v.push(*k); - } - } - Ok(v) } fn n_normal_benchmarks_remaining(n: usize) -> String { @@ -378,8 +316,8 @@ fn is_installed(name: &str) -> bool { fn get_benchmarks( benchmark_dir: &Path, - include: Option<&str>, - exclude: Option<&str>, + include: Option, + exclude: Option, ) -> anyhow::Result> { let mut benchmarks = Vec::new(); @@ -408,8 +346,10 @@ fn get_benchmarks( paths.push((path, name)); } - let mut includes = include.map(|list| list.split(',').collect::>()); - let mut excludes = exclude.map(|list| list.split(',').collect::>()); + let include = include.as_ref(); + let exclude = exclude.as_ref(); + let mut includes = include.map(|list| list.split(',').collect::>()); + let mut excludes = exclude.map(|list| list.split(',').collect::>()); for (path, name) in paths { let mut skip = false; @@ -472,10 +412,10 @@ fn get_benchmarks( fn get_local_toolchain( profile_kinds: &[ProfileKind], rustc: &str, - rustdoc: Option<&str>, - cargo: Option<&str>, + rustdoc: Option<&Path>, + cargo: Option<&Path>, ) -> anyhow::Result<(PathBuf, Option, PathBuf)> { - // + prefixed rustc is an indicator to fetch the rustc of the toolchain + // `+`-prefixed rustc is an indicator to fetch the rustc of the toolchain // specified. This follows the similar pattern used by rustup's binaries // (e.g., `rustc +stage1`). let rustc = if let Some(toolchain) = rustc.strip_prefix('+') { @@ -520,13 +460,13 @@ fn get_local_toolchain( } else { PathBuf::from(rustc) .canonicalize() - .with_context(|| format!("failed to canonicalize rustc executable '{}'", rustc))? + .with_context(|| format!("failed to canonicalize rustc executable {:?}", rustc))? }; let rustdoc = - if let Some(rustdoc) = rustdoc { - Some(PathBuf::from(rustdoc).canonicalize().with_context(|| { - format!("failed to canonicalize rustdoc executable '{}'", rustdoc) + if let Some(rustdoc) = &rustdoc { + Some(rustdoc.canonicalize().with_context(|| { + format!("failed to canonicalize rustdoc executable {:?}", rustdoc) })?) } else if profile_kinds.contains(&ProfileKind::Doc) { // We need a `rustdoc`. Look for one next to `rustc`. @@ -544,10 +484,10 @@ fn get_local_toolchain( None }; - let cargo = if let Some(cargo) = cargo { - PathBuf::from(cargo) + let cargo = if let Some(cargo) = &cargo { + cargo .canonicalize() - .with_context(|| format!("failed to canonicalize cargo executable '{}'", cargo))? + .with_context(|| format!("failed to canonicalize cargo executable {:?}", cargo))? } else { // Use the nightly cargo from `rustup`. let output = Command::new("rustup") @@ -595,6 +535,7 @@ fn generate_cachegrind_diffs( ScenarioKind::IncrPatched => (0..benchmark.patches.len()) .map(|i| format!("{:?}{}", kind, i)) .collect::>(), + ScenarioKind::All => unreachable!(), } }) { let filename = |prefix, id| { @@ -745,137 +686,196 @@ fn main() { } } -fn main_result() -> anyhow::Result { - env_logger::init(); +#[derive(Debug, clap::Parser)] +#[clap(about, version, author)] +struct Cli { + #[clap(subcommand)] + command: Commands, +} - let matches = clap_app!(rustc_perf_collector => - (version: "0.1") - (author: "The Rust Compiler Team") - (about: "Collects Rust performance data") +#[derive(Debug, clap::Args)] +struct RustcOption { + /// The path to the local rustc to benchmark + // Not a `PathBuf` because it can be a file path *or* a `+`-prefixed + // toolchain name, and `PathBuf` doesn't work well for the latter. + rustc: String, +} - // For each subcommand we list the mandatory arguments in the required - // order, followed by the options in alphabetical order. +#[derive(Debug, clap::Args)] +struct IdOption { + /// Identifier to associate benchmark results with + id: String, +} - (@subcommand bench_local => - (about: "Benchmarks a local rustc") +#[derive(Debug, clap::Args)] +struct LocalOptions { + /// Measure the build profiles in this comma-separated list + // This must be normalized via `ProfilerKind::expand_all()` before use. + #[clap( + long = "builds", + arg_enum, + multiple_values = true, + use_delimiter = true, + require_delimiter = true, + // Don't run rustdoc by default + default_value = "Check,Debug,Opt", + )] + profile_kinds: Vec, + + /// The path to the local Cargo to use + #[clap(long, parse(from_os_str))] + cargo: Option, + + /// Exclude all benchmarks in this comma-separated list + #[clap(long)] + exclude: Option, + + /// Include only benchmarks in this comma-separated list + #[clap(long)] + include: Option, + + /// The path to the local rustdoc to benchmark + #[clap(long, parse(from_os_str))] + rustdoc: Option, + + /// Measure the scenarios in this comma-separated list + #[clap( + long = "runs", + arg_enum, + multiple_values = true, + use_delimiter = true, + require_delimiter = true, + default_value = "All" + )] + scenario_kinds: Vec, +} - // Mandatory arguments - (@arg RUSTC: +required +takes_value "The path to the local rustc to benchmark") - (@arg ID: +required +takes_value "Identifier to associate benchmark results with") +#[derive(Debug, clap::Args)] +struct SelfProfileOption { + /// Collect self-profile data + #[clap(long = "self-profile")] + self_profile: bool, +} - // Options - (@arg BUILDS: --builds +takes_value - "One or more (comma-separated) of: 'Check', 'Debug',\n\ - 'Doc', 'Opt', 'All'") - (@arg CARGO: --cargo +takes_value "The path to the local Cargo to use") - (@arg DB: --db +takes_value "Database output file") - (@arg EXCLUDE: --exclude +takes_value - "Exclude all benchmarks that are listed in\n\ - this comma-separated list of patterns") - (@arg INCLUDE: --include +takes_value - "Include only benchmarks that are listed in\n\ - this comma-separated list of patterns") - (@arg BENCH_RUSTC: --("bench-rustc") - "Run the special `rustc` benchmark") - (@arg RUNS: --runs +takes_value - "One or more (comma-separated) of: 'Full',\n\ - 'IncrFull', 'IncrUnchanged', 'IncrPatched', 'All'") - (@arg ITERATIONS: --iterations +takes_value - "The number of iterations to do for each benchmark") - (@arg RUSTDOC: --rustdoc +takes_value "The path to the local rustdoc to benchmark") - (@arg SELF_PROFILE: --("self-profile") "Collect self-profile data") - ) +#[derive(Debug, clap::Args)] +struct DbOption { + /// Database output file + // This would be better as a `PathBuf`, but it's used in various ways that + // make that tricky without adjusting several points in the code. + #[clap(long, default_value = "results.db")] + db: String, +} - (@subcommand bench_next => - (about: "Benchmarks the next commit for perf.rust-lang.org, including the special `rustc` benchmark") +#[derive(Debug, clap::Args)] +struct BenchRustcOption { + // Run the special `rustc` benchmark + #[clap(long = "bench-rustc")] + bench_rustc: bool, +} - // Mandatory arguments - (@arg SITE_URL: +required +takes_value "Site URL") +// For each subcommand we list the mandatory arguments in the required +// order, followed by the options in alphabetical order. +#[derive(Debug, clap::Subcommand)] +#[clap(rename_all = "snake_case")] +enum Commands { + /// Benchmarks a local rustc + BenchLocal { + #[clap(flatten)] + rustc: RustcOption, - // Options - (@arg DB: --db +takes_value "Database output file") - (@arg BENCH_RUSTC: --("bench-rustc") - "Run the special `rustc` benchmark") - (@arg SELF_PROFILE: --("self-profile") "Collect self-profile data") - ) + #[clap(flatten)] + id: IdOption, - (@subcommand bench_published => - (about: "Benchmarks a published toolchain for perf.rust-lang.org's dashboard") + #[clap(flatten)] + local: LocalOptions, - // Mandatory arguments - (@arg TOOLCHAIN: +required +takes_value "Toolchain (e.g. stable, beta, 1.26.0)") + #[clap(flatten)] + db: DbOption, - // Options - (@arg DB: --db +takes_value "Database output file") - ) + #[clap(flatten)] + bench_rustc: BenchRustcOption, - (@subcommand profile_local => - (about: "Profiles a local rustc with one of several profilers") - - // Mandatory arguments - (@arg PROFILER: +required +takes_value - "One of: 'self-profile', 'time-passes', 'perf-record',\n\ - 'oprofile', 'cachegrind', 'callgrind', 'dhat', 'massif',\n\ - 'eprintln', 'llvm-lines', 'mono-items', 'dep-graph'") - (@arg RUSTC: +required +takes_value "The path to the local rustc to benchmark") - (@arg ID: +required +takes_value "Identifier to associate benchmark results with") - - // Options - (@arg BUILDS: --builds +takes_value - "One or more (comma-separated) of: 'Check', \n\ - 'Debug', 'Doc', 'Opt', 'All'") - (@arg CARGO: --cargo +takes_value "The path to the local Cargo to use") - (@arg EXCLUDE: --exclude +takes_value - "Exclude all benchmarks that are listed in\n\ - this comma-separated list of patterns") - (@arg INCLUDE: --include +takes_value - "Include only benchmarks that are listed in\n\ - this comma-separated list of patterns") - (@arg OUT_DIR: --("out-dir") +takes_value "Output directory") - (@arg RUNS: --runs +takes_value - "One or more (comma-separated) of: 'Full',\n\ - 'IncrFull', 'IncrUnchanged', 'IncrPatched', 'All'") - (@arg RUSTDOC: --rustdoc +takes_value "The path to the local rustdoc to benchmark") - ) + /// The number of iterations to do for each benchmark + #[clap(long, default_value = "1")] + iterations: usize, - (@subcommand diff_local => - (about: "Profiles and compares two toolchains with one of several profilers") - - // Mandatory arguments - (@arg PROFILER: +required +takes_value - "One of: 'self-profile', 'time-passes', 'perf-record',\n\ - 'oprofile', 'cachegrind', 'callgrind', 'dhat', 'massif',\n\ - 'eprintln', 'llvm-lines', 'mono-items', 'dep-graph'") - (@arg RUSTC_BEFORE: +required +takes_value "The path to the local rustc to benchmark") - (@arg RUSTC_AFTER: +required +takes_value "The path to the local rustc to benchmark") - - // Options - (@arg BUILDS: --builds +takes_value - "One or more (comma-separated) of: 'Check', \n\ - 'Debug', 'Doc', 'Opt', 'All'") - (@arg CARGO: --cargo +takes_value "The path to the local Cargo to use") - (@arg EXCLUDE: --exclude +takes_value - "Exclude all benchmarks that are listed in\n\ - this comma-separated list of patterns") - (@arg INCLUDE: --include +takes_value - "Include only benchmarks that are listed in\n\ - this comma-separated list of patterns") - (@arg OUT_DIR: --("out-dir") +takes_value "Output directory") - (@arg RUNS: --runs +takes_value - "One or more (comma-separated) of: 'Full',\n\ - 'IncrFull', 'IncrUnchanged', 'IncrPatched', 'All'") - (@arg RUSTDOC: --rustdoc +takes_value "The path to the local rustdoc to benchmark") - ) + #[clap(flatten)] + self_profile: SelfProfileOption, + }, - (@subcommand install_next => - (about: "Installs the next commit for perf.rust-lang.org") + /// Benchmarks the next commit for perf.rust-lang.org + BenchNext { + /// Site URL + site_url: String, - // Mandatory arguments: (none) + #[clap(flatten)] + db: DbOption, - // Options: (none) - ) - ) - .get_matches(); + #[clap(flatten)] + bench_rustc: BenchRustcOption, + + #[clap(flatten)] + self_profile: SelfProfileOption, + }, + + /// Benchmarks a published toolchain for perf.rust-lang.org's dashboard + BenchPublished { + /// Toolchain (e.g. stable, beta, 1.26.0) + toolchain: String, + + #[clap(flatten)] + db: DbOption, + }, + + /// Profiles a local rustc with one of several profilers + ProfileLocal { + /// Profiler to use + #[clap(arg_enum)] + profiler: Profiler, + + #[clap(flatten)] + rustc: RustcOption, + + #[clap(flatten)] + id: IdOption, + + #[clap(flatten)] + local: LocalOptions, + + /// Output directory + #[clap(long = "out-dir", default_value = "results/")] + out_dir: PathBuf, + }, + + /// Profiles and compares two toolchains with one of several profilers. If + /// the profiler is Cachegrind, also performs a diff + DiffLocal { + /// Profiler to use + #[clap(arg_enum)] + profiler: Profiler, + + /// The path to the first local rustc to benchmark + rustc_before: String, + + /// The path to the second local rustc to benchmark + rustc_after: String, + + #[clap(flatten)] + local: LocalOptions, + + /// Output directory + #[clap(long = "out-dir", default_value = "results/")] + out_dir: PathBuf, + }, + + /// Installs the next commit for perf.rust-lang.org + InstallNext, +} + +fn main_result() -> anyhow::Result { + env_logger::init(); + + let args = Cli::parse(); let benchmark_dir = PathBuf::from("collector/benchmarks"); @@ -887,44 +887,40 @@ fn main_result() -> anyhow::Result { .enable_io(); let mut rt = builder.build().expect("built runtime"); - let default_db = "results.db"; - let default_out_dir = std::ffi::OsStr::new("results"); - // XXX: This doesn't necessarily work for all archs let target_triple = format!("{}-unknown-linux-gnu", std::env::consts::ARCH); - let ret = match matches.subcommand() { - ("bench_local", Some(sub_m)) => { - // Mandatory arguments - let rustc = sub_m.value_of("RUSTC").unwrap(); - let id = sub_m.value_of("ID").unwrap(); - - // Options - let profile_kinds = profile_kinds_from_arg(&sub_m.value_of("BUILDS"))?; - let cargo = sub_m.value_of("CARGO"); - let db = sub_m.value_of("DB").unwrap_or(default_db); - let exclude = sub_m.value_of("EXCLUDE"); - let include = sub_m.value_of("INCLUDE"); - let bench_rustc = sub_m.is_present("BENCH_RUSTC"); - let scenario_kinds = scenario_kinds_from_arg(sub_m.value_of("RUNS"))?; - let iterations = iterations_from_arg(sub_m.value_of("ITERATIONS"))?; - let rustdoc = sub_m.value_of("RUSTDOC"); - let is_self_profile = sub_m.is_present("SELF_PROFILE"); - - let pool = database::Pool::open(db); - - let (rustc, rustdoc, cargo) = - get_local_toolchain(&profile_kinds, rustc, rustdoc, cargo)?; + match args.command { + Commands::BenchLocal { + rustc, + id, + local, + db, + bench_rustc, + iterations, + self_profile, + } => { + let profile_kinds = ProfileKind::expand_all(local.profile_kinds); + let scenario_kinds = ScenarioKind::expand_all(local.scenario_kinds); + + let pool = database::Pool::open(&db.db); + + let (rustc, rustdoc, cargo) = get_local_toolchain( + &profile_kinds, + &rustc.rustc, + local.rustdoc.as_deref(), + local.cargo.as_deref(), + )?; - let benchmarks = get_benchmarks(&benchmark_dir, include, exclude)?; + let benchmarks = get_benchmarks(&benchmark_dir, local.include, local.exclude)?; let res = bench( &mut rt, pool, - &ArtifactId::Tag(id.to_string()), + &ArtifactId::Tag(id.id), &profile_kinds, &scenario_kinds, - bench_rustc, + bench_rustc.bench_rustc, Compiler { rustc: &rustc, rustdoc: rustdoc.as_deref(), @@ -934,21 +930,18 @@ fn main_result() -> anyhow::Result { }, &benchmarks, Some(iterations), - is_self_profile, + self_profile.self_profile, ); res.fail_if_nonzero()?; Ok(0) } - ("bench_next", Some(sub_m)) => { - // Mandatory arguments - let site_url = sub_m.value_of("SITE_URL").unwrap(); - - // Options - let db = sub_m.value_of("DB").unwrap_or(default_db); - let bench_rustc = sub_m.is_present("BENCH_RUSTC"); - let is_self_profile = sub_m.is_present("SELF_PROFILE"); - + Commands::BenchNext { + site_url, + db, + bench_rustc, + self_profile, + } => { println!("processing commits"); let client = reqwest::blocking::Client::new(); let response: collector::api::next_commit::Response = client @@ -964,16 +957,12 @@ fn main_result() -> anyhow::Result { }; let commit = get_commit_or_fake_it(&next.sha)?; - let pool = database::Pool::open(db); + let pool = database::Pool::open(&db.db); let sysroot = Sysroot::install(commit.sha.to_string(), &target_triple) .with_context(|| format!("failed to install sysroot for {:?}", commit))?; - let benchmarks = get_benchmarks( - &benchmark_dir, - next.include.as_deref(), - next.exclude.as_deref(), - )?; + let benchmarks = get_benchmarks(&benchmark_dir, next.include, next.exclude)?; let res = bench( &mut rt, @@ -981,11 +970,11 @@ fn main_result() -> anyhow::Result { &ArtifactId::Commit(commit), &ProfileKind::all(), &ScenarioKind::all(), - bench_rustc, + bench_rustc.bench_rustc, Compiler::from_sysroot(&sysroot), &benchmarks, next.runs.map(|v| v as usize), - is_self_profile, + self_profile.self_profile, ); client.post(&format!("{}/perf/onpush", site_url)).send()?; @@ -994,13 +983,7 @@ fn main_result() -> anyhow::Result { Ok(0) } - ("bench_published", Some(sub_m)) => { - // Mandatory arguments - let toolchain = sub_m.value_of("TOOLCHAIN").unwrap(); - - // Options - let db = sub_m.value_of("DB").unwrap_or(default_db); - + Commands::BenchPublished { toolchain, db } => { let status = Command::new("rustup") .args(&["install", "--profile=minimal", &toolchain]) .status() @@ -1009,20 +992,17 @@ fn main_result() -> anyhow::Result { anyhow::bail!("failed to install toolchain for {}", toolchain); } - let pool = database::Pool::open(db); + let pool = database::Pool::open(&db.db); - let scenario_kinds = if collector::version_supports_incremental(toolchain) { - ScenarioKind::all() + let profile_kinds = if collector::version_supports_doc(&toolchain) { + ProfileKind::all() } else { - ScenarioKind::all_non_incr() + ProfileKind::all_non_doc() }; - let proile_kinds = if collector::version_supports_doc(toolchain) { - ProfileKind::all() + let scenario_kinds = if collector::version_supports_incremental(&toolchain) { + ScenarioKind::all() } else { - let mut all = ProfileKind::all(); - let doc = all.iter().position(|bk| *bk == ProfileKind::Doc).unwrap(); - all.remove(doc); - all + ScenarioKind::all_non_incr() }; let which = |tool| { @@ -1049,8 +1029,8 @@ fn main_result() -> anyhow::Result { let res = bench( &mut rt, pool, - &ArtifactId::Tag(toolchain.to_string()), - &proile_kinds, + &ArtifactId::Tag(toolchain), + &profile_kinds, &scenario_kinds, /* bench_rustc */ false, Compiler { @@ -1068,23 +1048,23 @@ fn main_result() -> anyhow::Result { Ok(0) } - ("profile_local", Some(sub_m)) => { - // Mandatory arguments - let profiler = Profiler::from_name(sub_m.value_of("PROFILER").unwrap())?; - let rustc = sub_m.value_of("RUSTC").unwrap(); - let id = sub_m.value_of("ID").unwrap(); - + Commands::ProfileLocal { + profiler, + rustc, + id, + local, + out_dir, + } => { // Options - let profile_kinds = profile_kinds_from_arg(&sub_m.value_of("BUILDS"))?; - let cargo = sub_m.value_of("CARGO"); - let exclude = sub_m.value_of("EXCLUDE"); - let include = sub_m.value_of("INCLUDE"); - let out_dir = PathBuf::from(sub_m.value_of_os("OUT_DIR").unwrap_or(default_out_dir)); - let scenario_kinds = scenario_kinds_from_arg(sub_m.value_of("RUNS"))?; - let rustdoc = sub_m.value_of("RUSTDOC"); - - let (rustc, rustdoc, cargo) = - get_local_toolchain(&profile_kinds, rustc, rustdoc, cargo)?; + let profile_kinds = ProfileKind::expand_all(local.profile_kinds); + let scenario_kinds = ScenarioKind::expand_all(local.scenario_kinds); + + let (rustc, rustdoc, cargo) = get_local_toolchain( + &profile_kinds, + &rustc.rustc, + local.rustdoc.as_deref(), + local.cargo.as_deref(), + )?; let compiler = Compiler { rustc: &rustc, rustdoc: rustdoc.as_deref(), @@ -1092,11 +1072,11 @@ fn main_result() -> anyhow::Result { triple: &target_triple, is_nightly: true, }; - let benchmarks = get_benchmarks(&benchmark_dir, include, exclude)?; + let benchmarks = get_benchmarks(&benchmark_dir, local.include, local.exclude)?; let mut errors = BenchmarkErrors::new(); profile( compiler, - id, + &id.id, profiler, &out_dir, &benchmarks, @@ -1108,23 +1088,20 @@ fn main_result() -> anyhow::Result { Ok(0) } - ("diff_local", Some(sub_m)) => { - // Mandatory arguments - let profiler = Profiler::from_name(sub_m.value_of("PROFILER").unwrap())?; - let rustc1 = sub_m.value_of("RUSTC_BEFORE").unwrap(); - let rustc2 = sub_m.value_of("RUSTC_AFTER").unwrap(); + Commands::DiffLocal { + profiler, + rustc_before, + rustc_after, + local, + out_dir, + } => { + let profile_kinds = ProfileKind::expand_all(local.profile_kinds); + let scenario_kinds = ScenarioKind::expand_all(local.scenario_kinds); - // Options - let profile_kinds = profile_kinds_from_arg(&sub_m.value_of("BUILDS"))?; - let cargo = sub_m.value_of("CARGO"); - let exclude = sub_m.value_of("EXCLUDE"); - let include = sub_m.value_of("INCLUDE"); - let out_dir = PathBuf::from(sub_m.value_of_os("OUT_DIR").unwrap_or(default_out_dir)); - let scenario_kinds = scenario_kinds_from_arg(sub_m.value_of("RUNS"))?; - let rustdoc = sub_m.value_of("RUSTDOC"); check_installed("valgrind")?; check_installed("cg_annotate")?; check_installed("rustfilt")?; + // Avoid just straight running rustup-toolchain-install-master which // will install the current master commit (fetching quite a bit of // data, including hitting GitHub)... @@ -1136,16 +1113,20 @@ fn main_result() -> anyhow::Result { anyhow::bail!("rustup-toolchain-install-master is not installed but must be"); } - let id1 = rustc1.strip_prefix('+').unwrap_or("before"); - let id2 = rustc2.strip_prefix('+').unwrap_or("after"); + let id1 = rustc_before.strip_prefix('+').unwrap_or("before"); + let id2 = rustc_after.strip_prefix('+').unwrap_or("after"); let mut toolchains = Vec::new(); - for (id, rustc) in [(id1, rustc1), (id2, rustc2)] { - let (rustc, rustdoc, cargo) = - get_local_toolchain(&profile_kinds, rustc, rustdoc, cargo)?; + for (id, rustc) in [(id1, &rustc_before), (id2, &rustc_after)] { + let (rustc, rustdoc, cargo) = get_local_toolchain( + &profile_kinds, + rustc, + local.rustdoc.as_deref(), + local.cargo.as_deref(), + )?; toolchains.push((id.to_owned(), rustc, rustdoc, cargo)); } - let benchmarks = get_benchmarks(&benchmark_dir, include, exclude)?; + let benchmarks = get_benchmarks(&benchmark_dir, local.include, local.exclude)?; let mut errors = BenchmarkErrors::new(); for (id, rustc, rustdoc, cargo) in &toolchains { let compiler = Compiler { @@ -1194,11 +1175,7 @@ fn main_result() -> anyhow::Result { Ok(0) } - ("install_next", Some(_sub_m)) => { - // Mandatory arguments: (none) - - // Options: (none) - + Commands::InstallNext => { let last_sha = Command::new("git") .arg("ls-remote") .arg("https://github.com/rust-lang/rust.git") @@ -1218,13 +1195,7 @@ fn main_result() -> anyhow::Result { Ok(0) } - - _ => { - let _ = writeln!(stderr(), "{}", matches.usage()); - Ok(2) - } - }; - ret + } } pub fn get_commit_or_fake_it(sha: &str) -> anyhow::Result { diff --git a/collector/src/rustc-fake.rs b/collector/src/rustc-fake.rs index c789ca606..89f28bf39 100644 --- a/collector/src/rustc-fake.rs +++ b/collector/src/rustc-fake.rs @@ -59,8 +59,9 @@ fn main() { raise_priority(); + // These strings come from `PerfTool::name()`. match wrapper { - "perf-stat" | "perf-stat-self-profile" => { + "PerfStat" | "PerfStatSelfProfile" => { let mut cmd = Command::new("perf"); determinism_env(&mut cmd); let has_perf = cmd.output().is_ok(); @@ -102,22 +103,27 @@ fn main() { } } - "xperf-stat" | "xperf-stat-self-profile" => { - // For Windows, we use a combination of xperf and tracelog to capture ETW events including hardware performance counters. - // To do this, we start an ETW trace using tracelog, telling it to include the InstructionRetired and TotalCycles PMCs - // for each CSwitch event that is recorded. Then when ETW records a context switch event, it will be preceeded by a - // PMC event which contains the raw counters at that instant. After we've finished compilation, we then use xperf - // to stop the trace and dump the results to a plain text file. This file is then processed by the `etw_parser` module - // which finds events related to the rustc process and calculates the total values for those performance counters. - // Conceptually, this is similar to how `perf` works on Linux except we have to do more of the work ourselves as there - // isn't an out of the box way to get the data we care about. - - // Read the path to xperf.exe and tracelog.exe from an environment variable, falling back to assuming it's on the PATH. + "XperfStat" | "XperfStatSelfProfile" => { + // For Windows, we use a combination of xperf and tracelog to capture ETW events + // including hardware performance counters. To do this, we start an ETW trace using + // tracelog, telling it to include the InstructionRetired and TotalCycles PMCs for + // each CSwitch event that is recorded. Then when ETW records a context switch + // event, it will be preceeded by a PMC event which contains the raw counters at + // that instant. After we've finished compilation, we then use xperf to stop the + // trace and dump the results to a plain text file. This file is then processed by + // the `etw_parser` module which finds events related to the rustc process and + // calculates the total values for those performance counters. Conceptually, this + // is similar to how `perf` works on Linux except we have to do more of the work + // ourselves as there isn't an out of the box way to get the data we care about. + + // Read the path to xperf.exe and tracelog.exe from an environment variable, + // falling back to assuming it's on the PATH. let xperf = std::env::var("XPERF").unwrap_or("xperf.exe".to_string()); let mut cmd = Command::new(&xperf); assert!(cmd.output().is_ok(), "xperf.exe could not be started"); - // go ahead and run `xperf -stop rustc-perf-counters` in case there are leftover counters running from a failed prior attempt + // Go ahead and run `xperf -stop rustc-perf-counters` in case there are leftover + // counters running from a failed prior attempt. let mut cmd = Command::new(&xperf); cmd.args(&["-stop", "rustc-perf-counters"]); cmd.status().expect("failed to spawn xperf"); @@ -180,7 +186,7 @@ fn main() { } } - "self-profile" => { + "SelfProfile" => { let mut cmd = Command::new(&tool); determinism_env(&mut cmd); cmd.arg("-Zself-profile-events=all"); @@ -189,7 +195,7 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "time-passes" => { + "TimePasses" => { args.insert(0, "-Ztime-passes".into()); let mut cmd = Command::new(&tool); @@ -200,7 +206,7 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "perf-record" => { + "PerfRecord" => { let mut cmd = Command::new("perf"); determinism_env(&mut cmd); let has_perf = cmd.output().is_ok(); @@ -216,7 +222,7 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "oprofile" => { + "Oprofile" => { let mut cmd = Command::new("operf"); determinism_env(&mut cmd); let has_oprofile = cmd.output().is_ok(); @@ -227,7 +233,7 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "cachegrind" => { + "Cachegrind" => { let mut cmd = Command::new("valgrind"); determinism_env(&mut cmd); let has_valgrind = cmd.output().is_ok(); @@ -252,7 +258,7 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "callgrind" => { + "Callgrind" => { let mut cmd = Command::new("valgrind"); determinism_env(&mut cmd); let has_valgrind = cmd.output().is_ok(); @@ -270,7 +276,7 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "dhat" => { + "Dhat" => { let mut cmd = Command::new("valgrind"); determinism_env(&mut cmd); let has_valgrind = cmd.output().is_ok(); @@ -284,7 +290,7 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "massif" => { + "Massif" => { let mut cmd = Command::new("valgrind"); determinism_env(&mut cmd); let has_valgrind = cmd.output().is_ok(); @@ -301,14 +307,14 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "eprintln" => { + "Eprintln" => { let mut cmd = bash_command(tool, args, "2> eprintln"); determinism_env(&mut cmd); assert!(cmd.status().expect("failed to spawn").success()); } - "llvm-lines" => { + "LlvmLines" => { // `cargo llvm-lines` writes its output to stdout. But we can't // redirect it to a file here like we do for "time-passes" and // "eprintln". This is because `cargo llvm-lines` invokes rustc @@ -323,7 +329,7 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "llvm-ir" => { + "LlvmIr" => { args.push("--emit=llvm-ir=./llvm-ir".into()); args.push("-Cno-prepopulate-passes".into()); args.push("-Cpasses=name-anon-globals".into()); @@ -333,7 +339,7 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "mono-items" => { + "MonoItems" => { // Lazy item collection is the default (i.e., without this // option) args.push("-Zprint-mono-items=lazy".into()); @@ -343,7 +349,7 @@ fn main() { assert!(cmd.status().expect("failed to spawn").success()); } - "dep-graph" => { + "DepGraph" => { args.push("-Zdump-dep-graph".into()); args.push("-Zquery-dep-graph".into()); let mut cmd = Command::new(tool); From 60df3837bcc8a12381cd9e448574d4c9059db3cd Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 12 Jan 2022 07:48:40 +1100 Subject: [PATCH 2/3] Address review comments. --- collector/src/main.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/collector/src/main.rs b/collector/src/main.rs index 0136a635b..4866ae81a 100644 --- a/collector/src/main.rs +++ b/collector/src/main.rs @@ -768,7 +768,7 @@ struct DbOption { #[derive(Debug, clap::Args)] struct BenchRustcOption { - // Run the special `rustc` benchmark + /// Run the special `rustc` benchmark #[clap(long = "bench-rustc")] bench_rustc: bool, } @@ -1218,3 +1218,11 @@ pub fn get_commit_or_fake_it(sha: &str) -> anyhow::Result { } })) } + +#[test] +fn verify_app() { + // By default, clap lazily checks subcommands. This provides eager testing + // without having to run the binary for each subcommand. + use clap::IntoApp; + Cli::into_app().debug_assert() +} From cc9e711167e1238f653f6820244e1b2cab1dcc49 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 17 Jan 2022 14:28:25 +1100 Subject: [PATCH 3/3] Upgrade `database` to clap 3. Pretty minor stuff, mostly renamed functions. --- Cargo.lock | 61 +++----------------------- database/Cargo.toml | 2 +- database/src/bin/postgres-to-sqlite.rs | 16 +++---- database/src/bin/sqlite-to-postgres.rs | 5 ++- 4 files changed, 17 insertions(+), 67 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 381b93045..a43c24426 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -61,15 +61,6 @@ dependencies = [ "serde_json", ] -[[package]] -name = "ansi_term" -version = "0.11.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ee49baf6cb617b853aa8d93bf420db2383fab46d314482ca2803b40d5fde979b" -dependencies = [ - "winapi", -] - [[package]] name = "ansi_term" version = "0.12.1" @@ -211,21 +202,6 @@ dependencies = [ "winapi", ] -[[package]] -name = "clap" -version = "2.33.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "37e58ac78573c40708d45522f0d80fa2f01cc4f9b4e2bf749807255454312002" -dependencies = [ - "ansi_term 0.11.0", - "atty", - "bitflags", - "strsim 0.8.0", - "textwrap 0.11.0", - "unicode-width", - "vec_map", -] - [[package]] name = "clap" version = "3.0.6" @@ -238,9 +214,9 @@ dependencies = [ "indexmap", "lazy_static", "os_str_bytes", - "strsim 0.10.0", + "strsim", "termcolor", - "textwrap 0.14.2", + "textwrap", ] [[package]] @@ -262,7 +238,7 @@ version = "0.1.0" dependencies = [ "anyhow", "chrono", - "clap 3.0.6", + "clap", "crossbeam-utils", "database", "env_logger", @@ -374,7 +350,7 @@ dependencies = [ "async-trait", "bytes", "chrono", - "clap 2.33.3", + "clap", "csv", "env_logger", "futures", @@ -1367,7 +1343,7 @@ version = "0.7.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1cab0e7c02cf376875e9335e0ba1da535775beb5450d21e1dffca068818ed98b" dependencies = [ - "ansi_term 0.12.1", + "ansi_term", "ctor", "diff", "output_vt100", @@ -1881,12 +1857,6 @@ dependencies = [ "unicode-normalization", ] -[[package]] -name = "strsim" -version = "0.8.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a" - [[package]] name = "strsim" version = "0.10.0" @@ -1944,15 +1914,6 @@ dependencies = [ "winapi-util", ] -[[package]] -name = "textwrap" -version = "0.11.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d326610f408c7a4eb6f51c37c330e496b08506c9457c9d34287ecc38809fb060" -dependencies = [ - "unicode-width", -] - [[package]] name = "textwrap" version = "0.14.2" @@ -2143,12 +2104,6 @@ dependencies = [ "tinyvec", ] -[[package]] -name = "unicode-width" -version = "0.1.8" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9337591893a19b88d8d87f2cec1e73fad5cdfd10e5a6f349f498ad6ea2ffb1e3" - [[package]] name = "unicode-xid" version = "0.2.2" @@ -2179,12 +2134,6 @@ version = "0.2.13" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "025ce40a007e1907e58d5bc1a594def78e5573bb0b1160bc389634e8f12e4faa" -[[package]] -name = "vec_map" -version = "0.8.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f1bddf1187be692e79c5ffeab891132dfb0f236ed36a43c7ed39f1165ee20191" - [[package]] name = "version_check" version = "0.9.3" diff --git a/database/Cargo.toml b/database/Cargo.toml index a8d836e0d..87cdb635f 100644 --- a/database/Cargo.toml +++ b/database/Cargo.toml @@ -25,4 +25,4 @@ futures = "0.3.5" log = "0.4" bytes = "1" csv = "1" -clap = "2.25" +clap = { version = "3.0.6", features = ["cargo"] } diff --git a/database/src/bin/postgres-to-sqlite.rs b/database/src/bin/postgres-to-sqlite.rs index 6181c1b5a..a7c9ebc2a 100644 --- a/database/src/bin/postgres-to-sqlite.rs +++ b/database/src/bin/postgres-to-sqlite.rs @@ -4,7 +4,6 @@ //! transactions, and will likely fail if used on a populated database. use chrono::{DateTime, Utc}; -use clap::value_t; use database::pool::{postgres, sqlite, ConnectionManager}; use futures::StreamExt; use rusqlite::params; @@ -464,8 +463,9 @@ async fn main() -> anyhow::Result<()> { let matches = clap::App::new("postgres-to-sqlite") .about("Exports a rustc-perf Postgres database to a SQLite database") + .version(clap::crate_version!()) .arg( - clap::Arg::with_name("exclude-tables") + clap::Arg::new("exclude-tables") .long("exclude-tables") .takes_value(true) .value_name("TABLES") @@ -474,24 +474,24 @@ async fn main() -> anyhow::Result<()> { .help("Exclude given tables (as foreign key constraints allow)"), ) .arg( - clap::Arg::with_name("no-self-profile") + clap::Arg::new("no-self-profile") .long("no-self-profile") .help("Exclude some potentially large self-profile tables (additive with --exclude-tables)"), ) .arg( - clap::Arg::with_name("since-weeks-ago") + clap::Arg::new("since-weeks-ago") .long("since-weeks-ago") .takes_value(true) .value_name("WEEKS") .help("Exclude data associated with artifacts whose date value precedes weeks ago"), ) .arg( - clap::Arg::with_name("fast-unsafe") + clap::Arg::new("fast-unsafe") .long("fast-unsafe") .help("Enable faster execution at the risk of corrupting SQLite database in the event of a crash"), ) .arg( - clap::Arg::with_name("postgres-db") + clap::Arg::new("postgres-db") .required(true) .value_name("POSTGRES_DB") .help( @@ -500,7 +500,7 @@ async fn main() -> anyhow::Result<()> { ), ) .arg( - clap::Arg::with_name("sqlite-db") + clap::Arg::new("sqlite-db") .required(true) .value_name("SQLITE_DB") .help("SQLite database file"), @@ -521,7 +521,7 @@ async fn main() -> anyhow::Result<()> { // `RawSelfProfile` is intentionally kept. } - let since_weeks_ago = match clap::value_t!(matches, "since-weeks-ago", u32) { + let since_weeks_ago = match matches.value_of_t("since-weeks-ago") { Ok(weeks) => Some(weeks), Err(err) if err.kind == clap::ErrorKind::ArgumentNotFound => None, Err(err) => err.exit(), diff --git a/database/src/bin/sqlite-to-postgres.rs b/database/src/bin/sqlite-to-postgres.rs index aed8a7513..abfc3a50d 100644 --- a/database/src/bin/sqlite-to-postgres.rs +++ b/database/src/bin/sqlite-to-postgres.rs @@ -614,14 +614,15 @@ async fn main() -> anyhow::Result<()> { let matches = clap::App::new("sqlite-to-postgres") .about("Exports a rustc-perf SQLite database to a Postgres database") + .version(clap::crate_version!()) .arg( - clap::Arg::with_name("sqlite-db") + clap::Arg::new("sqlite-db") .required(true) .value_name("SQLITE_DB") .help("SQLite database file"), ) .arg( - clap::Arg::with_name("postgres-db") + clap::Arg::new("postgres-db") .required(true) .value_name("POSTGRES_DB") .help(