From d58a8405378add6eae2f7c62d5b620eea86161c1 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 6 Apr 2022 14:41:23 +1000 Subject: [PATCH] Make `--include` and `--exclude` use exact matching. In #1138 I changed `--include`/`--exclude` to do exact matching of benchmark names. This was a good idea at the time, but since then we have renamed many benchmarks. There is now no benchmark with a name that is a prefix of another benchmark's name. Also, we now have version numbers in many benchmark names, which are a pain to remember and type. This commit lets you specify a benchmark name by prefix. E.g. `--include=stm` will match `stm32f4-0.14.0`. --- collector/README.md | 12 +++---- collector/src/main.rs | 79 +++++++++++++++++++++++++------------------ 2 files changed, 53 insertions(+), 38 deletions(-) diff --git a/collector/README.md b/collector/README.md index 1c1c6f352..1c6534c75 100644 --- a/collector/README.md +++ b/collector/README.md @@ -109,15 +109,15 @@ The following options alter the behaviour of the `bench_local` subcommand. supports postgres as a backend and the URL can be specified (beginning with `postgres://`), but this is unlikely to be useful for local collection. - `--exclude `: this is used to run a subset of the benchmarks. The - argument is a comma-separated list of benchmark names. When this option is - specified, a benchmark is excluded from the run if its name matches one or - more of the given names. + argument is a comma-separated list of benchmark prefixes. When this option is + specified, a benchmark is excluded from the run if its name matches one of + the given prefixes. - `--id ` the identifier that will be used to identify the results in the database. - `--include `: the inverse of `--exclude`. The argument is a - comma-separated list of benchmark names. When this option is specified, a - benchmark is included in the run only if its name matches one or more of the - given names. + comma-separated list of benchmark prefixes. When this option is specified, a + benchmark is included in the run only if its name matches one of the given + prefixes. - `--profiles `: the profiles to be benchmarked. The possible choices are one or more (comma-separated) of `Check`, `Debug`, `Doc`, `Opt`, and `All`. The default is `Check,Debug,Opt`. diff --git a/collector/src/main.rs b/collector/src/main.rs index ba41f03c4..18e697639 100644 --- a/collector/src/main.rs +++ b/collector/src/main.rs @@ -5,7 +5,7 @@ use clap::Parser; use collector::category::Category; use database::{ArtifactId, Commit}; use log::debug; -use std::collections::HashSet; +use std::collections::HashMap; use std::fs; use std::fs::File; use std::io::BufWriter; @@ -331,26 +331,37 @@ 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::>()); + // For each --include/--exclude entry, we count how many times it's used, + // to enable `check_for_unused` below. + fn to_hashmap(xyz: Option<&str>) -> Option> { + xyz.map(|list| { + list.split(',') + .map(|x| (x, 0)) + .collect::>() + }) + } + + let mut includes = to_hashmap(include); + let mut excludes = to_hashmap(exclude); for (path, name) in paths { let mut skip = false; - if let Some(includes) = includes.as_mut() { - if !includes.remove(name.as_str()) { - debug!( - "benchmark {} - not named by --include argument, skipping", - name - ); - skip = true; + + let name_matches = |prefixes: &mut HashMap<&str, usize>| { + for (prefix, n) in prefixes.iter_mut() { + if name.as_str().starts_with(prefix) { + *n += 1; + return true; + } } - } + false + }; + if let Some(includes) = includes.as_mut() { + skip |= !name_matches(includes); + } if let Some(excludes) = excludes.as_mut() { - if excludes.remove(name.as_str()) { - debug!("benchmark {} - named by --exclude argument, skipping", name); - skip = true; - } + skip |= name_matches(excludes); } if skip { continue; @@ -360,22 +371,26 @@ fn get_benchmarks( benchmarks.push(Benchmark::new(name, path)?); } - if let Some(includes) = includes { - if !includes.is_empty() { - bail!( - "Warning: one or more invalid --include entries: {:?}", - includes - ); - } - } - if let Some(excludes) = excludes { - if !excludes.is_empty() { - bail!( - "Warning: one or more invalid --exclude entries: {:?}", - excludes - ); + // All prefixes must be used at least once. This is to catch typos. + let check_for_unused = |option, prefixes: Option>| { + if let Some(prefixes) = prefixes { + let unused: Vec<_> = prefixes + .into_iter() + .filter_map(|(i, n)| if n == 0 { Some(i) } else { None }) + .collect(); + if !unused.is_empty() { + bail!( + "Warning: one or more unused --{} entries: {:?}", + option, + unused + ); + } } - } + Ok(()) + }; + + check_for_unused("include", includes)?; + check_for_unused("exclude", excludes)?; benchmarks.sort_by_key(|benchmark| benchmark.name.clone()); @@ -729,11 +744,11 @@ struct LocalOptions { #[clap(long, parse(from_os_str))] cargo: Option, - /// Exclude all benchmarks in this comma-separated list + /// Exclude all benchmarks matching a prefix in this comma-separated list #[clap(long)] exclude: Option, - /// Include only benchmarks in this comma-separated list + /// Include only benchmarks matching a prefix in this comma-separated list #[clap(long)] include: Option,