From 4a34b9dd43ffc349cedd4be6d8928b625a06f096 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 22 Dec 2022 12:03:26 -0600 Subject: [PATCH 1/3] perf(parser): Reduce lookups for conflicts We already need to lookup every present-arg for conflicts, so we might as well cache it ahead of time. This let's us move some operations to be immutable so we can more easily cache other lookups. For me, this gave a 70% speed improvement for #4516 with mixed results on normal benchmarks --- src/parser/arg_matcher.rs | 4 ++ src/parser/validator.rs | 140 +++++++++++++++++++++----------------- src/util/mod.rs | 4 +- 3 files changed, 82 insertions(+), 66 deletions(-) diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index 93bac7e1734..c48b10855cb 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -113,6 +113,10 @@ impl ArgMatcher { self.matches.args.keys() } + pub(crate) fn args(&self) -> crate::util::flat_map::Iter<'_, Id, MatchedArg> { + self.matches.args.iter() + } + pub(crate) fn entry(&mut self, arg: Id) -> crate::util::Entry { self.matches.args.entry(arg) } diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 0d97c2f6378..7b7cbf13bf5 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -1,6 +1,6 @@ // Internal use crate::builder::StyledStr; -use crate::builder::{Arg, ArgPredicate, Command, PossibleValue}; +use crate::builder::{Arg, ArgGroup, ArgPredicate, Command, PossibleValue}; use crate::error::{Error, Result as ClapResult}; use crate::output::Usage; use crate::parser::{ArgMatcher, ParseState}; @@ -27,7 +27,7 @@ impl<'cmd> Validator<'cmd> { matcher: &mut ArgMatcher, ) -> ClapResult<()> { debug!("Validator::validate"); - let mut conflicts = Conflicts::new(); + let conflicts = Conflicts::with_args(self.cmd, matcher); let has_subcmd = matcher.subcommand_name().is_some(); if let ParseState::Opt(a) = parse_state { @@ -80,9 +80,9 @@ impl<'cmd> Validator<'cmd> { )); } - ok!(self.validate_conflicts(matcher, &mut conflicts)); + ok!(self.validate_conflicts(matcher, &conflicts)); if !(self.cmd.is_subcommand_negates_reqs_set() && has_subcmd) { - ok!(self.validate_required(matcher, &mut conflicts)); + ok!(self.validate_required(matcher, &conflicts)); } Ok(()) @@ -91,7 +91,7 @@ impl<'cmd> Validator<'cmd> { fn validate_conflicts( &mut self, matcher: &ArgMatcher, - conflicts: &mut Conflicts, + conflicts: &Conflicts, ) -> ClapResult<()> { debug!("Validator::validate_conflicts"); @@ -103,7 +103,7 @@ impl<'cmd> Validator<'cmd> { .filter(|arg_id| self.cmd.find(arg_id).is_some()) { debug!("Validator::validate_conflicts::iter: id={:?}", arg_id); - let conflicts = conflicts.gather_conflicts(self.cmd, matcher, arg_id); + let conflicts = conflicts.gather_conflicts(self.cmd, arg_id); ok!(self.build_conflict_err(arg_id, &conflicts, matcher)); } @@ -243,11 +243,7 @@ impl<'cmd> Validator<'cmd> { } } - fn validate_required( - &mut self, - matcher: &ArgMatcher, - conflicts: &mut Conflicts, - ) -> ClapResult<()> { + fn validate_required(&mut self, matcher: &ArgMatcher, conflicts: &Conflicts) -> ClapResult<()> { debug!("Validator::validate_required: required={:?}", self.required); self.gather_requires(matcher); @@ -276,7 +272,7 @@ impl<'cmd> Validator<'cmd> { debug!("Validator::validate_required:iter:aog={:?}", arg_or_group); if let Some(arg) = self.cmd.find(arg_or_group) { debug!("Validator::validate_required:iter: This is an arg"); - if !is_exclusive_present && !self.is_missing_required_ok(arg, matcher, conflicts) { + if !is_exclusive_present && !self.is_missing_required_ok(arg, conflicts) { debug!( "Validator::validate_required:iter: Missing {:?}", arg.get_id() @@ -374,14 +370,9 @@ impl<'cmd> Validator<'cmd> { Ok(()) } - fn is_missing_required_ok( - &self, - a: &Arg, - matcher: &ArgMatcher, - conflicts: &mut Conflicts, - ) -> bool { + fn is_missing_required_ok(&self, a: &Arg, conflicts: &Conflicts) -> bool { debug!("Validator::is_missing_required_ok: {}", a.get_id()); - let conflicts = conflicts.gather_conflicts(self.cmd, matcher, a.get_id()); + let conflicts = conflicts.gather_conflicts(self.cmd, a.get_id()); !conflicts.is_empty() } @@ -465,71 +456,92 @@ struct Conflicts { } impl Conflicts { - fn new() -> Self { - Self::default() + fn with_args(cmd: &Command, matcher: &ArgMatcher) -> Self { + let mut potential = FlatMap::new(); + potential.extend_unchecked( + matcher + .args() + .filter(|(_, matched)| matched.check_explicit(&ArgPredicate::IsPresent)) + .map(|(id, _)| { + let conf = gather_direct_conflicts(cmd, id); + (id.clone(), conf) + }), + ); + Self { potential } } - fn gather_conflicts(&mut self, cmd: &Command, matcher: &ArgMatcher, arg_id: &Id) -> Vec { + fn gather_conflicts(&self, cmd: &Command, arg_id: &Id) -> Vec { debug!("Conflicts::gather_conflicts: arg={:?}", arg_id); let mut conflicts = Vec::new(); - for other_arg_id in matcher - .arg_ids() - .filter(|arg_id| matcher.check_explicit(arg_id, &ArgPredicate::IsPresent)) - { + + let arg_id_conflicts_storage; + let arg_id_conflicts = if let Some(arg_id_conflicts) = self.get_direct_conflicts(arg_id) { + arg_id_conflicts + } else { + // `is_missing_required_ok` is a case where we check not-present args for conflicts + arg_id_conflicts_storage = gather_direct_conflicts(cmd, arg_id); + &arg_id_conflicts_storage + }; + for (other_arg_id, other_arg_id_conflicts) in self.potential.iter() { if arg_id == other_arg_id { continue; } - if self - .gather_direct_conflicts(cmd, arg_id) - .contains(other_arg_id) - { + if arg_id_conflicts.contains(other_arg_id) { conflicts.push(other_arg_id.clone()); } - if self - .gather_direct_conflicts(cmd, other_arg_id) - .contains(arg_id) - { + if other_arg_id_conflicts.contains(arg_id) { conflicts.push(other_arg_id.clone()); } } + debug!("Conflicts::gather_conflicts: conflicts={:?}", conflicts); conflicts } - fn gather_direct_conflicts(&mut self, cmd: &Command, arg_id: &Id) -> &[Id] { - self.potential.entry(arg_id.clone()).or_insert_with(|| { - let conf = if let Some(arg) = cmd.find(arg_id) { - let mut conf = arg.blacklist.clone(); - for group_id in cmd.groups_for_arg(arg_id) { - let group = cmd.find_group(&group_id).expect(INTERNAL_ERROR_MSG); - conf.extend(group.conflicts.iter().cloned()); - if !group.multiple { - for member_id in &group.args { - if member_id != arg_id { - conf.push(member_id.clone()); - } - } - } - } + fn get_direct_conflicts(&self, arg_id: &Id) -> Option<&[Id]> { + self.potential.get(arg_id).map(Vec::as_slice) + } +} - // Overrides are implicitly conflicts - conf.extend(arg.overrides.iter().cloned()); +fn gather_direct_conflicts(cmd: &Command, id: &Id) -> Vec { + let conf = if let Some(arg) = cmd.find(id) { + gather_arg_direct_conflicts(cmd, arg) + } else if let Some(group) = cmd.find_group(id) { + gather_group_direct_conflicts(group) + } else { + debug_assert!(false, "id={:?} is unknown", id); + Vec::new() + }; + debug!( + "Conflicts::gather_direct_conflicts id={:?}, conflicts={:?}", + id, conf + ); + conf +} - conf - } else if let Some(group) = cmd.find_group(arg_id) { - group.conflicts.clone() - } else { - debug_assert!(false, "id={:?} is unknown", arg_id); - Vec::new() - }; - debug!( - "Conflicts::gather_direct_conflicts id={:?}, conflicts={:?}", - arg_id, conf - ); - conf - }) +fn gather_arg_direct_conflicts(cmd: &Command, arg: &Arg) -> Vec { + let mut conf = arg.blacklist.clone(); + for group_id in cmd.groups_for_arg(arg.get_id()) { + let group = cmd.find_group(&group_id).expect(INTERNAL_ERROR_MSG); + conf.extend(group.conflicts.iter().cloned()); + if !group.multiple { + for member_id in &group.args { + if member_id != arg.get_id() { + conf.push(member_id.clone()); + } + } + } } + + // Overrides are implicitly conflicts + conf.extend(arg.overrides.iter().cloned()); + + conf +} + +fn gather_group_direct_conflicts(group: &ArgGroup) -> Vec { + group.conflicts.clone() } pub(crate) fn get_possible_values_cli(a: &Arg) -> Vec { diff --git a/src/util/mod.rs b/src/util/mod.rs index ed38fbc545f..e6a8f70ed50 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -1,7 +1,7 @@ #![allow(clippy::single_component_path_imports)] -mod flat_map; -mod flat_set; +pub(crate) mod flat_map; +pub(crate) mod flat_set; mod graph; mod id; mod str_to_bool; From dd8435d8f34d8343db2a1bda76f05b699e08fbb3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 22 Dec 2022 12:17:22 -0600 Subject: [PATCH 2/3] perf(parser): Reduce duplicate lookups Didn't actually see much of a gain because this isn't in a hot loop but thought I'd keep it. --- src/parser/validator.rs | 54 ++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 7b7cbf13bf5..088259ffd9f 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -54,8 +54,8 @@ impl<'cmd> Validator<'cmd> { if !has_subcmd && self.cmd.is_arg_required_else_help_set() { let num_user_values = matcher - .arg_ids() - .filter(|arg_id| matcher.check_explicit(arg_id, &ArgPredicate::IsPresent)) + .args() + .filter(|(_, matched)| matched.check_explicit(&ArgPredicate::IsPresent)) .count(); if num_user_values == 0 { let message = self.cmd.write_help_err(false); @@ -97,10 +97,10 @@ impl<'cmd> Validator<'cmd> { ok!(self.validate_exclusive(matcher)); - for arg_id in matcher - .arg_ids() - .filter(|arg_id| matcher.check_explicit(arg_id, &ArgPredicate::IsPresent)) - .filter(|arg_id| self.cmd.find(arg_id).is_some()) + for (arg_id, _) in matcher + .args() + .filter(|(_, matched)| matched.check_explicit(&ArgPredicate::IsPresent)) + .filter(|(arg_id, _)| self.cmd.find(arg_id).is_some()) { debug!("Validator::validate_conflicts::iter: id={:?}", arg_id); let conflicts = conflicts.gather_conflicts(self.cmd, arg_id); @@ -113,9 +113,9 @@ impl<'cmd> Validator<'cmd> { fn validate_exclusive(&self, matcher: &ArgMatcher) -> ClapResult<()> { debug!("Validator::validate_exclusive"); let args_count = matcher - .arg_ids() - .filter(|arg_id| { - matcher.check_explicit(arg_id, &crate::builder::ArgPredicate::IsPresent) + .args() + .filter(|(arg_id, matched)| { + matched.check_explicit(&crate::builder::ArgPredicate::IsPresent) // Avoid including our own groups by checking none of them. If a group is present, the // args for the group will be. && self.cmd.find(arg_id).is_some() @@ -127,14 +127,12 @@ impl<'cmd> Validator<'cmd> { } matcher - .arg_ids() - .filter(|arg_id| { - matcher.check_explicit(arg_id, &crate::builder::ArgPredicate::IsPresent) - }) - .filter_map(|name| { - debug!("Validator::validate_exclusive:iter:{:?}", name); + .args() + .filter(|(_, matched)| matched.check_explicit(&crate::builder::ArgPredicate::IsPresent)) + .filter_map(|(id, _)| { + debug!("Validator::validate_exclusive:iter:{:?}", id); self.cmd - .find(name) + .find(id) // Find `arg`s which are exclusive but also appear with other args. .filter(|&arg| arg.is_exclusive_set() && args_count > 1) }) @@ -196,8 +194,9 @@ impl<'cmd> Validator<'cmd> { conflicting_keys: &[Id], ) -> Option { let used_filtered: Vec = matcher - .arg_ids() - .filter(|arg_id| matcher.check_explicit(arg_id, &ArgPredicate::IsPresent)) + .args() + .filter(|(_, matched)| matched.check_explicit(&ArgPredicate::IsPresent)) + .map(|(n, _)| n) .filter(|n| { // Filter out the args we don't want to specify. self.cmd.find(n).map_or(false, |a| !a.is_hide_set()) @@ -220,14 +219,14 @@ impl<'cmd> Validator<'cmd> { fn gather_requires(&mut self, matcher: &ArgMatcher) { debug!("Validator::gather_requires"); - for name in matcher - .arg_ids() - .filter(|arg_id| matcher.check_explicit(arg_id, &ArgPredicate::IsPresent)) + for (name, matched) in matcher + .args() + .filter(|(_, matched)| matched.check_explicit(&ArgPredicate::IsPresent)) { debug!("Validator::gather_requires:iter:{:?}", name); if let Some(arg) = self.cmd.find(name) { let is_relevant = |(val, req_arg): &(ArgPredicate, Id)| -> Option { - let required = matcher.check_explicit(arg.get_id(), val); + let required = matched.check_explicit(val); required.then(|| req_arg.clone()) }; @@ -251,9 +250,9 @@ impl<'cmd> Validator<'cmd> { let mut highest_index = 0; let is_exclusive_present = matcher - .arg_ids() - .filter(|arg_id| matcher.check_explicit(arg_id, &ArgPredicate::IsPresent)) - .any(|id| { + .args() + .filter(|(_, matched)| matched.check_explicit(&ArgPredicate::IsPresent)) + .any(|(id, _)| { self.cmd .find(id) .map(|arg| arg.is_exclusive_set()) @@ -432,8 +431,9 @@ impl<'cmd> Validator<'cmd> { ); let used: Vec = matcher - .arg_ids() - .filter(|arg_id| matcher.check_explicit(arg_id, &ArgPredicate::IsPresent)) + .args() + .filter(|(_, matched)| matched.check_explicit(&ArgPredicate::IsPresent)) + .map(|(n, _)| n) .filter(|n| { // Filter out the args we don't want to specify. self.cmd.find(n).map_or(false, |a| !a.is_hide_set()) From dde22e74ca5442d904f29fd4833f1cc902d3e7f8 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 22 Dec 2022 12:25:33 -0600 Subject: [PATCH 3/3] style: Update for latest clippy --- clap_mangen/src/lib.rs | 2 +- clap_mangen/src/render.rs | 24 ++++++++++++------------ examples/repl.rs | 2 +- src/builder/command.rs | 4 ++-- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/clap_mangen/src/lib.rs b/clap_mangen/src/lib.rs index 66dda1e03d1..d529325f9f7 100644 --- a/clap_mangen/src/lib.rs +++ b/clap_mangen/src/lib.rs @@ -231,7 +231,7 @@ impl Man { } fn _render_version_section(&self, roff: &mut Roff) { - let version = roman(&render::version(&self.cmd)); + let version = roman(render::version(&self.cmd)); roff.control("SH", ["VERSION"]); roff.text([version]); } diff --git a/clap_mangen/src/render.rs b/clap_mangen/src/render.rs index 9c400ffe2db..ab9c9ec75c1 100644 --- a/clap_mangen/src/render.rs +++ b/clap_mangen/src/render.rs @@ -13,7 +13,7 @@ pub(crate) fn about(roff: &mut Roff, cmd: &clap::Command) { Some(about) => format!("{} - {}", cmd.get_name(), about), None => cmd.get_name().to_string(), }; - roff.text([roman(&s)]); + roff.text([roman(s)]); } pub(crate) fn description(roff: &mut Roff, cmd: &clap::Command) { @@ -36,19 +36,19 @@ pub(crate) fn synopsis(roff: &mut Roff, cmd: &clap::Command) { match (opt.get_short(), opt.get_long()) { (Some(short), Some(long)) => { line.push(roman(lhs)); - line.push(bold(&format!("-{}", short))); + line.push(bold(format!("-{}", short))); line.push(roman("|")); - line.push(bold(&format!("--{}", long))); + line.push(bold(format!("--{}", long))); line.push(roman(rhs)); } (Some(short), None) => { line.push(roman(lhs)); - line.push(bold(&format!("-{} ", short))); + line.push(bold(format!("-{} ", short))); line.push(roman(rhs)); } (None, Some(long)) => { line.push(roman(lhs)); - line.push(bold(&format!("--{}", long))); + line.push(bold(format!("--{}", long))); line.push(roman(rhs)); } (None, None) => continue, @@ -101,12 +101,12 @@ pub(crate) fn options(roff: &mut Roff, cmd: &clap::Command) { if let Some(value) = &opt.get_value_names() { header.push(roman("=")); - header.push(italic(&value.join(" "))); + header.push(italic(value.join(" "))); } if let Some(defs) = option_default_values(opt) { header.push(roman(" ")); - header.push(roman(&defs)); + header.push(roman(defs)); } let mut body = vec![]; @@ -168,13 +168,13 @@ pub(crate) fn options(roff: &mut Roff, cmd: &clap::Command) { header.push(roman(rhs)); if let Some(defs) = option_default_values(pos) { - header.push(roman(&format!(" {}", defs))); + header.push(roman(format!(" {}", defs))); } let mut body = vec![]; let mut arg_help_written = false; if let Some(help) = option_help(pos) { - body.push(roman(&help.to_string())); + body.push(roman(help.to_string())); arg_help_written = true; } @@ -229,7 +229,7 @@ pub(crate) fn subcommands(roff: &mut Roff, cmd: &clap::Command, section: &str) { sub.get_name(), section ); - roff.text([roman(&name)]); + roff.text([roman(name)]); if let Some(about) = sub.get_about().or_else(|| sub.get_long_about()) { for line in about.to_string().lines() { @@ -273,11 +273,11 @@ fn markers(required: bool) -> (&'static str, &'static str) { } fn short_option(opt: char) -> Inline { - bold(&format!("-{}", opt)) + bold(format!("-{}", opt)) } fn long_option(opt: &str) -> Inline { - bold(&format!("--{}", opt)) + bold(format!("--{}", opt)) } fn option_help(opt: &clap::Arg) -> Option<&clap::builder::StyledStr> { diff --git a/examples/repl.rs b/examples/repl.rs index e18fbbd6547..ef265911588 100644 --- a/examples/repl.rs +++ b/examples/repl.rs @@ -29,7 +29,7 @@ fn main() -> Result<(), String> { fn respond(line: &str) -> Result { let args = shlex::split(line).ok_or("error: Invalid quoting")?; let matches = cli() - .try_get_matches_from(&args) + .try_get_matches_from(args) .map_err(|e| e.to_string())?; match matches.subcommand() { Some(("ping", _matches)) => { diff --git a/src/builder/command.rs b/src/builder/command.rs index 203e677a7be..b235a95045f 100644 --- a/src/builder/command.rs +++ b/src/builder/command.rs @@ -487,7 +487,7 @@ impl Command { /// [`Command::try_get_matches_from_mut`]: Command::try_get_matches_from_mut() #[inline] pub fn get_matches(self) -> ArgMatches { - self.get_matches_from(&mut env::args_os()) + self.get_matches_from(env::args_os()) } /// Parse [`env::args_os`], exiting on failure. @@ -545,7 +545,7 @@ impl Command { #[inline] pub fn try_get_matches(self) -> ClapResult { // Start the parsing - self.try_get_matches_from(&mut env::args_os()) + self.try_get_matches_from(env::args_os()) } /// Parse the specified arguments, exiting on failure.