From a733b0b0521e823d848f620f5ad3f7edd89a4db2 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 9 Mar 2024 20:07:53 +0900 Subject: [PATCH] revset: detach index from predicate fn, turn it into position-based This is the step towards removing &CompositeIndex references from the revset evaluation tree. The filter input is changed from &IndexEntry to IndexPosition to simplify the lifetime thingy. We might want to pass around CommitId or Commit object once it's loaded, but that can be implemented later. I don't see significant performance difference in revset benches. --- lib/src/default_index/rev_walk.rs | 2 - lib/src/default_index/revset_engine.rs | 236 ++++++++++++------------- 2 files changed, 112 insertions(+), 126 deletions(-) diff --git a/lib/src/default_index/rev_walk.rs b/lib/src/default_index/rev_walk.rs index 75637f8e69..a1816c49a5 100644 --- a/lib/src/default_index/rev_walk.rs +++ b/lib/src/default_index/rev_walk.rs @@ -80,7 +80,6 @@ pub(super) struct EagerRevWalk { } impl EagerRevWalk { - #[cfg(test)] // TODO pub fn new(iter: T) -> Self { EagerRevWalk { iter: iter.fuse() } } @@ -136,7 +135,6 @@ impl> PeekableRevWalk { self.peeked.as_ref() } - #[cfg(test)] // TODO pub fn next_if( &mut self, index: &I, diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs index bf6d0c26de..32fa7fb554 100644 --- a/lib/src/default_index/revset_engine.rs +++ b/lib/src/default_index/revset_engine.rs @@ -24,7 +24,7 @@ use std::sync::Arc; use itertools::Itertools; -use super::rev_walk::{RevWalk, RevWalkBuilder}; +use super::rev_walk::{EagerRevWalk, RevWalk, RevWalkBuilder}; use super::revset_graph_iterator::RevsetGraphIterator; use crate::backend::{ChangeId, CommitId, MillisSinceEpoch}; use crate::default_index::{AsCompositeIndex, CompositeIndex, IndexEntry, IndexPosition}; @@ -38,22 +38,18 @@ use crate::revset_graph::RevsetGraphEdge; use crate::rewrite; use crate::store::Store; +type BoxedPredicateFn<'a> = Box bool + 'a>; + trait ToPredicateFn: fmt::Debug { /// Creates function that tests if the given entry is included in the set. /// /// The predicate function is evaluated in order of `RevsetIterator`. - fn to_predicate_fn<'a, 'index: 'a>( - &'a self, - index: &'index CompositeIndex, - ) -> Box) -> bool + 'a>; + fn to_predicate_fn(&self) -> BoxedPredicateFn<'_>; } impl ToPredicateFn for Box { - fn to_predicate_fn<'a, 'index: 'a>( - &'a self, - index: &'index CompositeIndex, - ) -> Box) -> bool + 'a> { - ::to_predicate_fn(self, index) + fn to_predicate_fn(&self) -> BoxedPredicateFn<'_> { + ::to_predicate_fn(self) } } @@ -277,11 +273,9 @@ impl InternalRevset for EagerRevset { } impl ToPredicateFn for EagerRevset { - fn to_predicate_fn<'a, 'index: 'a>( - &'a self, - _index: &'index CompositeIndex, - ) -> Box) -> bool + 'a> { - predicate_fn_from_positions(self.positions.iter().copied()) + fn to_predicate_fn(&self) -> BoxedPredicateFn<'_> { + let walk = EagerRevWalk::new(self.positions.iter().copied()); + predicate_fn_from_rev_walk(walk) } } @@ -326,24 +320,21 @@ impl ToPredicateFn for RevWalkRevset where W: RevWalk + Clone, { - fn to_predicate_fn<'a, 'index: 'a>( - &'a self, - index: &'index CompositeIndex, - ) -> Box) -> bool + 'a> { - let positions = self.walk.clone().attach(index); - predicate_fn_from_positions(positions) + fn to_predicate_fn(&self) -> BoxedPredicateFn<'_> { + predicate_fn_from_rev_walk(self.walk.clone()) } } -fn predicate_fn_from_positions<'iter>( - iter: impl Iterator + 'iter, -) -> Box) -> bool + 'iter> { - let mut iter = iter.fuse().peekable(); - Box::new(move |entry| { - while iter.next_if(|&pos| pos > entry.position()).is_some() { +fn predicate_fn_from_rev_walk<'a, W>(walk: W) -> BoxedPredicateFn<'a> +where + W: RevWalk + 'a, +{ + let mut walk = walk.peekable(); + Box::new(move |index, entry_pos| { + while walk.next_if(index, |&pos| pos > entry_pos).is_some() { continue; } - iter.next_if(|&pos| pos == entry.position()).is_some() + walk.next_if(index, |&pos| pos == entry_pos).is_some() }) } @@ -362,15 +353,22 @@ where &'a self, index: &'index CompositeIndex, ) -> Box> + 'a> { - let p = self.predicate.to_predicate_fn(index); - Box::new(self.candidates.entries(index).filter(p)) + Box::new( + self.positions(index) + .map(move |pos| index.entry_by_pos(pos)), + ) } fn positions<'a, 'index: 'a>( &'a self, index: &'index CompositeIndex, ) -> Box + 'a> { - Box::new(self.entries(index).map(|entry| entry.position())) + let mut p = self.predicate.to_predicate_fn(); + Box::new( + self.candidates + .positions(index) + .filter(move |&pos| p(index, pos)), + ) } fn into_predicate<'a>(self: Box) -> Box @@ -386,13 +384,10 @@ where S: ToPredicateFn, P: ToPredicateFn, { - fn to_predicate_fn<'a, 'index: 'a>( - &'a self, - index: &'index CompositeIndex, - ) -> Box) -> bool + 'a> { - let mut p1 = self.candidates.to_predicate_fn(index); - let mut p2 = self.predicate.to_predicate_fn(index); - Box::new(move |entry| p1(entry) && p2(entry)) + fn to_predicate_fn(&self) -> BoxedPredicateFn<'_> { + let mut p1 = self.candidates.to_predicate_fn(); + let mut p2 = self.predicate.to_predicate_fn(); + Box::new(move |index, pos| p1(index, pos) && p2(index, pos)) } } @@ -400,12 +395,9 @@ where struct NotInPredicate(S); impl ToPredicateFn for NotInPredicate { - fn to_predicate_fn<'a, 'index: 'a>( - &'a self, - index: &'index CompositeIndex, - ) -> Box) -> bool + 'a> { - let mut p = self.0.to_predicate_fn(index); - Box::new(move |entry| !p(entry)) + fn to_predicate_fn(&self) -> BoxedPredicateFn<'_> { + let mut p = self.0.to_predicate_fn(); + Box::new(move |index, pos| !p(index, pos)) } } @@ -455,13 +447,10 @@ where S1: ToPredicateFn, S2: ToPredicateFn, { - fn to_predicate_fn<'a, 'index: 'a>( - &'a self, - index: &'index CompositeIndex, - ) -> Box) -> bool + 'a> { - let mut p1 = self.set1.to_predicate_fn(index); - let mut p2 = self.set2.to_predicate_fn(index); - Box::new(move |entry| p1(entry) || p2(entry)) + fn to_predicate_fn(&self) -> BoxedPredicateFn<'_> { + let mut p1 = self.set1.to_predicate_fn(); + let mut p2 = self.set2.to_predicate_fn(); + Box::new(move |index, pos| p1(index, pos) || p2(index, pos)) } } @@ -561,13 +550,10 @@ where S1: ToPredicateFn, S2: ToPredicateFn, { - fn to_predicate_fn<'a, 'index: 'a>( - &'a self, - index: &'index CompositeIndex, - ) -> Box) -> bool + 'a> { - let mut p1 = self.set1.to_predicate_fn(index); - let mut p2 = self.set2.to_predicate_fn(index); - Box::new(move |entry| p1(entry) && p2(entry)) + fn to_predicate_fn(&self) -> BoxedPredicateFn<'_> { + let mut p1 = self.set1.to_predicate_fn(); + let mut p2 = self.set2.to_predicate_fn(); + Box::new(move |index, pos| p1(index, pos) && p2(index, pos)) } } @@ -679,13 +665,10 @@ where S1: ToPredicateFn, S2: ToPredicateFn, { - fn to_predicate_fn<'a, 'index: 'a>( - &'a self, - index: &'index CompositeIndex, - ) -> Box) -> bool + 'a> { - let mut p1 = self.set1.to_predicate_fn(index); - let mut p2 = self.set2.to_predicate_fn(index); - Box::new(move |entry| p1(entry) && !p2(entry)) + fn to_predicate_fn(&self) -> BoxedPredicateFn<'_> { + let mut p1 = self.set1.to_predicate_fn(); + let mut p2 = self.set2.to_predicate_fn(); + Box::new(move |index, pos| p1(index, pos) && !p2(index, pos)) } } @@ -849,8 +832,9 @@ impl<'index> EvaluationContext<'index> { .ancestors_until_roots(root_positions.iter().copied()) .detach(); let candidates = RevWalkRevset { walk }; - let predicate = as_pure_predicate_fn(move |_index, entry| { - entry + let predicate = as_pure_predicate_fn(move |index, pos| { + index + .entry_by_pos(pos) .parent_positions() .iter() .any(|parent_pos| root_positions.contains(parent_pos)) @@ -1024,26 +1008,22 @@ impl fmt::Debug for PurePredicateFn { impl ToPredicateFn for PurePredicateFn where - F: Fn(&CompositeIndex, &IndexEntry<'_>) -> bool, + F: Fn(&CompositeIndex, IndexPosition) -> bool, { - fn to_predicate_fn<'a, 'index: 'a>( - &'a self, - index: &'index CompositeIndex, - ) -> Box) -> bool + 'a> { - let f = &self.0; - Box::new(move |entry| f(index, entry)) + fn to_predicate_fn(&self) -> BoxedPredicateFn<'_> { + Box::new(&self.0) } } fn as_pure_predicate_fn(f: F) -> PurePredicateFn where - F: Fn(&CompositeIndex, &IndexEntry<'_>) -> bool, + F: Fn(&CompositeIndex, IndexPosition) -> bool, { PurePredicateFn(f) } fn box_pure_predicate_fn<'a>( - f: impl Fn(&CompositeIndex, &IndexEntry<'_>) -> bool + 'a, + f: impl Fn(&CompositeIndex, IndexPosition) -> bool + 'a, ) -> Box { Box::new(PurePredicateFn(f)) } @@ -1055,13 +1035,15 @@ fn build_predicate_fn( match predicate { RevsetFilterPredicate::ParentCount(parent_count_range) => { let parent_count_range = parent_count_range.clone(); - box_pure_predicate_fn(move |_index, entry| { + box_pure_predicate_fn(move |index, pos| { + let entry = index.entry_by_pos(pos); parent_count_range.contains(&entry.num_parents()) }) } RevsetFilterPredicate::Description(pattern) => { let pattern = pattern.clone(); - box_pure_predicate_fn(move |_index, entry| { + box_pure_predicate_fn(move |index, pos| { + let entry = index.entry_by_pos(pos); let commit = store.get_commit(&entry.commit_id()).unwrap(); pattern.matches(commit.description()) }) @@ -1071,14 +1053,16 @@ fn build_predicate_fn( // TODO: Make these functions that take a needle to search for accept some // syntax for specifying whether it's a regex and whether it's // case-sensitive. - box_pure_predicate_fn(move |_index, entry| { + box_pure_predicate_fn(move |index, pos| { + let entry = index.entry_by_pos(pos); let commit = store.get_commit(&entry.commit_id()).unwrap(); pattern.matches(&commit.author().name) || pattern.matches(&commit.author().email) }) } RevsetFilterPredicate::Committer(pattern) => { let pattern = pattern.clone(); - box_pure_predicate_fn(move |_index, entry| { + box_pure_predicate_fn(move |index, pos| { + let entry = index.entry_by_pos(pos); let commit = store.get_commit(&entry.commit_id()).unwrap(); pattern.matches(&commit.committer().name) || pattern.matches(&commit.committer().email) @@ -1091,11 +1075,13 @@ fn build_predicate_fn( } else { Box::new(EverythingMatcher) }; - box_pure_predicate_fn(move |index, entry| { - has_diff_from_parent(&store, index, entry, matcher.as_ref()) + box_pure_predicate_fn(move |index, pos| { + let entry = index.entry_by_pos(pos); + has_diff_from_parent(&store, index, &entry, matcher.as_ref()) }) } - RevsetFilterPredicate::HasConflict => box_pure_predicate_fn(move |_index, entry| { + RevsetFilterPredicate::HasConflict => box_pure_predicate_fn(move |index, pos| { + let entry = index.entry_by_pos(pos); let commit = store.get_commit(&entry.commit_id()).unwrap(); commit.has_conflict().unwrap() }), @@ -1161,21 +1147,23 @@ mod tests { }; let set = make_set(&[&id_4, &id_3, &id_2, &id_0]); - let mut p = set.to_predicate_fn(index); - assert!(p(&get_entry(&id_4))); - assert!(p(&get_entry(&id_3))); - assert!(p(&get_entry(&id_2))); - assert!(!p(&get_entry(&id_1))); - assert!(p(&get_entry(&id_0))); + let mut p = set.to_predicate_fn(); + assert!(p(index, get_pos(&id_4))); + assert!(p(index, get_pos(&id_3))); + assert!(p(index, get_pos(&id_2))); + assert!(!p(index, get_pos(&id_1))); + assert!(p(index, get_pos(&id_0))); // Uninteresting entries can be skipped - let mut p = set.to_predicate_fn(index); - assert!(p(&get_entry(&id_3))); - assert!(!p(&get_entry(&id_1))); - assert!(p(&get_entry(&id_0))); + let mut p = set.to_predicate_fn(); + assert!(p(index, get_pos(&id_3))); + assert!(!p(index, get_pos(&id_1))); + assert!(p(index, get_pos(&id_0))); let set = FilterRevset { candidates: make_set(&[&id_4, &id_2, &id_0]), - predicate: as_pure_predicate_fn(|_index, entry| entry.commit_id() != id_4), + predicate: as_pure_predicate_fn(|index, pos| { + index.entry_by_pos(pos).commit_id() != id_4 + }), }; assert_eq!( set.entries(index).collect_vec(), @@ -1185,12 +1173,12 @@ mod tests { set.positions(index).collect_vec(), make_positions(&[&id_2, &id_0]) ); - let mut p = set.to_predicate_fn(index); - assert!(!p(&get_entry(&id_4))); - assert!(!p(&get_entry(&id_3))); - assert!(p(&get_entry(&id_2))); - assert!(!p(&get_entry(&id_1))); - assert!(p(&get_entry(&id_0))); + let mut p = set.to_predicate_fn(); + assert!(!p(index, get_pos(&id_4))); + assert!(!p(index, get_pos(&id_3))); + assert!(p(index, get_pos(&id_2))); + assert!(!p(index, get_pos(&id_1))); + assert!(p(index, get_pos(&id_0))); // Intersection by FilterRevset let set = FilterRevset { @@ -1199,12 +1187,12 @@ mod tests { }; assert_eq!(set.entries(index).collect_vec(), make_entries(&[&id_2])); assert_eq!(set.positions(index).collect_vec(), make_positions(&[&id_2])); - let mut p = set.to_predicate_fn(index); - assert!(!p(&get_entry(&id_4))); - assert!(!p(&get_entry(&id_3))); - assert!(p(&get_entry(&id_2))); - assert!(!p(&get_entry(&id_1))); - assert!(!p(&get_entry(&id_0))); + let mut p = set.to_predicate_fn(); + assert!(!p(index, get_pos(&id_4))); + assert!(!p(index, get_pos(&id_3))); + assert!(p(index, get_pos(&id_2))); + assert!(!p(index, get_pos(&id_1))); + assert!(!p(index, get_pos(&id_0))); let set = UnionRevset { set1: make_set(&[&id_4, &id_2]), @@ -1218,12 +1206,12 @@ mod tests { set.positions(index).collect_vec(), make_positions(&[&id_4, &id_3, &id_2, &id_1]) ); - let mut p = set.to_predicate_fn(index); - assert!(p(&get_entry(&id_4))); - assert!(p(&get_entry(&id_3))); - assert!(p(&get_entry(&id_2))); - assert!(p(&get_entry(&id_1))); - assert!(!p(&get_entry(&id_0))); + let mut p = set.to_predicate_fn(); + assert!(p(index, get_pos(&id_4))); + assert!(p(index, get_pos(&id_3))); + assert!(p(index, get_pos(&id_2))); + assert!(p(index, get_pos(&id_1))); + assert!(!p(index, get_pos(&id_0))); let set = IntersectionRevset { set1: make_set(&[&id_4, &id_2, &id_0]), @@ -1231,12 +1219,12 @@ mod tests { }; assert_eq!(set.entries(index).collect_vec(), make_entries(&[&id_2])); assert_eq!(set.positions(index).collect_vec(), make_positions(&[&id_2])); - let mut p = set.to_predicate_fn(index); - assert!(!p(&get_entry(&id_4))); - assert!(!p(&get_entry(&id_3))); - assert!(p(&get_entry(&id_2))); - assert!(!p(&get_entry(&id_1))); - assert!(!p(&get_entry(&id_0))); + let mut p = set.to_predicate_fn(); + assert!(!p(index, get_pos(&id_4))); + assert!(!p(index, get_pos(&id_3))); + assert!(p(index, get_pos(&id_2))); + assert!(!p(index, get_pos(&id_1))); + assert!(!p(index, get_pos(&id_0))); let set = DifferenceRevset { set1: make_set(&[&id_4, &id_2, &id_0]), @@ -1250,12 +1238,12 @@ mod tests { set.positions(index).collect_vec(), make_positions(&[&id_4, &id_0]) ); - let mut p = set.to_predicate_fn(index); - assert!(p(&get_entry(&id_4))); - assert!(!p(&get_entry(&id_3))); - assert!(!p(&get_entry(&id_2))); - assert!(!p(&get_entry(&id_1))); - assert!(p(&get_entry(&id_0))); + let mut p = set.to_predicate_fn(); + assert!(p(index, get_pos(&id_4))); + assert!(!p(index, get_pos(&id_3))); + assert!(!p(index, get_pos(&id_2))); + assert!(!p(index, get_pos(&id_1))); + assert!(p(index, get_pos(&id_0))); } #[test]