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
This commit is contained in:
Ed Page 2022-12-22 12:03:26 -06:00
parent d2d022248b
commit 4a34b9dd43
3 changed files with 84 additions and 68 deletions

View file

@ -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<Id, MatchedArg> {
self.matches.args.entry(arg)
}

View file

@ -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,48 +456,78 @@ 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<Id> {
fn gather_conflicts(&self, cmd: &Command, arg_id: &Id) -> Vec<Id> {
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) {
fn get_direct_conflicts(&self, arg_id: &Id) -> Option<&[Id]> {
self.potential.get(arg_id).map(Vec::as_slice)
}
}
fn gather_direct_conflicts(cmd: &Command, id: &Id) -> Vec<Id> {
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
}
fn gather_arg_direct_conflicts(cmd: &Command, arg: &Arg) -> Vec<Id> {
let mut conf = arg.blacklist.clone();
for group_id in cmd.groups_for_arg(arg_id) {
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_id {
if member_id != arg.get_id() {
conf.push(member_id.clone());
}
}
@ -517,19 +538,10 @@ impl Conflicts {
conf.extend(arg.overrides.iter().cloned());
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_group_direct_conflicts(group: &ArgGroup) -> Vec<Id> {
group.conflicts.clone()
}
pub(crate) fn get_possible_values_cli(a: &Arg) -> Vec<PossibleValue> {

View file

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