From d49b1ffdab56fdb8c3062e766e9ca9e8e5d818c1 Mon Sep 17 00:00:00 2001 From: blyxyas Date: Thu, 30 Jan 2025 19:29:12 +0100 Subject: [PATCH 1/3] Implement --perf flag to lintcheck for benchmarking Turns out I was completely overcomplicating myself, there was no need for an external tool such as becnhv2 or even the original becnh, we already had the benchmarking infrastructure right under our noses! This PR implements a new **lintcheck** option called --perf, using it as a flag will mean that lintcheck builds Clippy as a release package and hooks perf to it. The realization that lintcheck is already 90% of what a benchmarking tool needs came to me in a dream. --- lintcheck/src/config.rs | 5 +++++ lintcheck/src/main.rs | 41 +++++++++++++++++++++++++++++++++++------ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/lintcheck/src/config.rs b/lintcheck/src/config.rs index af243f94274d..7fa4fb7c0346 100644 --- a/lintcheck/src/config.rs +++ b/lintcheck/src/config.rs @@ -46,6 +46,11 @@ pub(crate) struct LintcheckConfig { /// Run clippy on the dependencies of crates specified in crates-toml #[clap(long, conflicts_with("max_jobs"))] pub recursive: bool, + /// Also produce a `perf.data` file, implies --jobs=1, + /// the `perf.data` file can be found at + /// `target/lintcheck/sources/-/perf.data` + #[clap(long)] + pub perf: bool, #[command(subcommand)] pub subcommand: Option, } diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index e88d9f427bec..0438ff40ed1c 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -116,7 +116,25 @@ impl Crate { clippy_args.extend(lint_levels_args.iter().map(String::as_str)); - let mut cmd = Command::new("cargo"); + let mut cmd; + + if config.perf { + cmd = Command::new("perf"); + cmd.args(&[ + "record", + "-e", + "instructions", // Only count instructions + "-g", // Enable call-graph, useful for flamegraphs and produces richer reports + "--quiet", // Do not tamper with lintcheck's normal output + "-o", + "perf.data", + "--", + "cargo", + ]); + } else { + cmd = Command::new("cargo"); + } + cmd.arg(if config.fix { "fix" } else { "check" }) .arg("--quiet") .current_dir(&self.path) @@ -234,9 +252,15 @@ fn normalize_diag( } /// Builds clippy inside the repo to make sure we have a clippy executable we can use. -fn build_clippy() -> String { +fn build_clippy(release_build: bool) -> String { let output = Command::new("cargo") - .args(["run", "--bin=clippy-driver", "--", "--version"]) + .args([ + "run", + "--bin=clippy-driver", + if release_build { "-r" } else { "" }, + "--", + "--version", + ]) .stderr(Stdio::inherit()) .output() .unwrap(); @@ -270,13 +294,18 @@ fn main() { #[allow(clippy::too_many_lines)] fn lintcheck(config: LintcheckConfig) { - let clippy_ver = build_clippy(); - let clippy_driver_path = fs::canonicalize(format!("target/debug/clippy-driver{EXE_SUFFIX}")).unwrap(); + let clippy_ver = build_clippy(config.perf); + let clippy_driver_path = fs::canonicalize(format!( + "target/{}/clippy-driver{EXE_SUFFIX}", + if config.perf { "release" } else { "debug" } + )) + .unwrap(); // assert that clippy is found assert!( clippy_driver_path.is_file(), - "target/debug/clippy-driver binary not found! {}", + "target/{}/clippy-driver binary not found! {}", + if config.perf { "release" } else { "debug" }, clippy_driver_path.display() ); From fc00cdcf285888a3c029df3ce36cc75692b6140b Mon Sep 17 00:00:00 2001 From: blyxyas Date: Thu, 30 Jan 2025 21:04:50 +0100 Subject: [PATCH 2/3] Actually make --perf imply -j=1, review comments --- lintcheck/src/config.rs | 3 +++ lintcheck/src/main.rs | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/lintcheck/src/config.rs b/lintcheck/src/config.rs index 7fa4fb7c0346..408fb162eb6f 100644 --- a/lintcheck/src/config.rs +++ b/lintcheck/src/config.rs @@ -2,6 +2,7 @@ use clap::{Parser, Subcommand, ValueEnum}; use std::num::NonZero; use std::path::PathBuf; +#[allow(clippy::struct_excessive_bools)] #[derive(Parser, Clone, Debug)] #[command(args_conflicts_with_subcommands = true)] pub(crate) struct LintcheckConfig { @@ -11,6 +12,8 @@ pub(crate) struct LintcheckConfig { short = 'j', value_name = "N", default_value_t = 0, + default_value_if("perf", "true", Some("1")), // Limit jobs to 1 when benchmarking + required = false, hide_default_value = true )] pub max_jobs: usize, diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index 0438ff40ed1c..af1b66bd039f 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -131,6 +131,7 @@ impl Crate { "--", "cargo", ]); + cmd.env("CARGO_PROFILE_RELEASE_DEBUG", "true"); } else { cmd = Command::new("cargo"); } @@ -285,6 +286,13 @@ fn main() { let config = LintcheckConfig::new(); + if config.perf && config.max_jobs != 1 { + eprintln!( + "Lintcheck's --perf flag must be triggered only with 1 job,\nremove either the --jobs/-j flag or the --perf flag" + ); + return; + } + match config.subcommand { Some(Commands::Diff { old, new, truncate }) => json::diff(&old, &new, truncate), Some(Commands::Popular { output, number }) => popular_crates::fetch(output, number).unwrap(), From 48ae7ec894ae4749c97358454a926188f579beb4 Mon Sep 17 00:00:00 2001 From: blyxyas Date: Fri, 31 Jan 2025 16:06:08 +0100 Subject: [PATCH 3/3] Adress review comments --- lintcheck/src/config.rs | 1 + lintcheck/src/main.rs | 34 +++++++++++++++------------------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/lintcheck/src/config.rs b/lintcheck/src/config.rs index 408fb162eb6f..83c3d7aba021 100644 --- a/lintcheck/src/config.rs +++ b/lintcheck/src/config.rs @@ -13,6 +13,7 @@ pub(crate) struct LintcheckConfig { value_name = "N", default_value_t = 0, default_value_if("perf", "true", Some("1")), // Limit jobs to 1 when benchmarking + conflicts_with("perf"), required = false, hide_default_value = true )] diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index af1b66bd039f..8d0d41ab9450 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -131,7 +131,6 @@ impl Crate { "--", "cargo", ]); - cmd.env("CARGO_PROFILE_RELEASE_DEBUG", "true"); } else { cmd = Command::new("cargo"); } @@ -254,17 +253,21 @@ fn normalize_diag( /// Builds clippy inside the repo to make sure we have a clippy executable we can use. fn build_clippy(release_build: bool) -> String { - let output = Command::new("cargo") - .args([ - "run", - "--bin=clippy-driver", - if release_build { "-r" } else { "" }, - "--", - "--version", - ]) - .stderr(Stdio::inherit()) - .output() - .unwrap(); + let mut build_cmd = Command::new("cargo"); + build_cmd.args([ + "run", + "--bin=clippy-driver", + if release_build { "-r" } else { "" }, + "--", + "--version", + ]); + + if release_build { + build_cmd.env("CARGO_PROFILE_RELEASE_DEBUG", "true"); + } + + let output = build_cmd.stderr(Stdio::inherit()).output().unwrap(); + if !output.status.success() { eprintln!("Error: Failed to compile Clippy!"); std::process::exit(1); @@ -286,13 +289,6 @@ fn main() { let config = LintcheckConfig::new(); - if config.perf && config.max_jobs != 1 { - eprintln!( - "Lintcheck's --perf flag must be triggered only with 1 job,\nremove either the --jobs/-j flag or the --perf flag" - ); - return; - } - match config.subcommand { Some(Commands::Diff { old, new, truncate }) => json::diff(&old, &new, truncate), Some(Commands::Popular { output, number }) => popular_crates::fetch(output, number).unwrap(),