From c847e21b5fea41e104e759a17e5be90ad6621ec6 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Tue, 20 Feb 2024 12:13:47 +0900
Subject: [PATCH 1/5] revset: extract generic combination iterators

I'm going to add pre-filtering to the 'roots..heads' evaluation path, and
difference_by() will be used there to calculate 'heads ~ roots'.

Union and intersection iterators are slightly changed so that all iterators
prioritize iter1's item.
---
 lib/src/default_index/revset_engine.rs | 137 +++++++++++++++++++------
 1 file changed, 103 insertions(+), 34 deletions(-)

diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs
index 15de39cd03..240c6cbea5 100644
--- a/lib/src/default_index/revset_engine.rs
+++ b/lib/src/default_index/revset_engine.rs
@@ -338,10 +338,11 @@ where
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
-        Box::new(UnionRevsetIterator {
-            iter1: self.set1.iter(index).peekable(),
-            iter2: self.set2.iter(index).peekable(),
-        })
+        Box::new(union_by(
+            self.set1.iter(index),
+            self.set2.iter(index),
+            |entry1, entry2| entry1.position().cmp(&entry2.position()),
+        ))
     }
 
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
@@ -367,27 +368,32 @@ where
     }
 }
 
-struct UnionRevsetIterator<I1: Iterator, I2: Iterator> {
+/// Iterator that merges two sorted iterators.
+///
+/// The input items should be sorted in descending order by the `cmp` function.
+struct UnionByIterator<I1: Iterator, I2: Iterator, C> {
     iter1: Peekable<I1>,
     iter2: Peekable<I2>,
+    cmp: C,
 }
 
-impl<'index, I1, I2> Iterator for UnionRevsetIterator<I1, I2>
+impl<I1, I2, C> Iterator for UnionByIterator<I1, I2, C>
 where
-    I1: Iterator<Item = IndexEntry<'index>>,
-    I2: Iterator<Item = IndexEntry<'index>>,
+    I1: Iterator,
+    I2: Iterator<Item = I1::Item>,
+    C: FnMut(&I1::Item, &I2::Item) -> Ordering,
 {
-    type Item = IndexEntry<'index>;
+    type Item = I1::Item;
 
     fn next(&mut self) -> Option<Self::Item> {
         match (self.iter1.peek(), self.iter2.peek()) {
             (None, _) => self.iter2.next(),
             (_, None) => self.iter1.next(),
-            (Some(entry1), Some(entry2)) => match entry1.position().cmp(&entry2.position()) {
+            (Some(item1), Some(item2)) => match (self.cmp)(item1, item2) {
                 Ordering::Less => self.iter2.next(),
                 Ordering::Equal => {
-                    self.iter1.next();
-                    self.iter2.next()
+                    self.iter2.next();
+                    self.iter1.next()
                 }
                 Ordering::Greater => self.iter1.next(),
             },
@@ -395,6 +401,23 @@ where
     }
 }
 
+fn union_by<I1, I2, C>(
+    iter1: I1,
+    iter2: I2,
+    cmp: C,
+) -> UnionByIterator<I1::IntoIter, I2::IntoIter, C>
+where
+    I1: IntoIterator,
+    I2: IntoIterator<Item = I1::Item>,
+    C: FnMut(&I1::Item, &I2::Item) -> Ordering,
+{
+    UnionByIterator {
+        iter1: iter1.into_iter().peekable(),
+        iter2: iter2.into_iter().peekable(),
+        cmp,
+    }
+}
+
 #[derive(Debug)]
 struct IntersectionRevset<S1, S2> {
     set1: S1,
@@ -410,10 +433,11 @@ where
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
-        Box::new(IntersectionRevsetIterator {
-            iter1: self.set1.iter(index).peekable(),
-            iter2: self.set2.iter(index).peekable(),
-        })
+        Box::new(intersection_by(
+            self.set1.iter(index),
+            self.set2.iter(index),
+            |entry1, entry2| entry1.position().cmp(&entry2.position()),
+        ))
     }
 
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
@@ -439,17 +463,22 @@ where
     }
 }
 
-struct IntersectionRevsetIterator<I1: Iterator, I2: Iterator> {
+/// Iterator that intersects two sorted iterators.
+///
+/// The input items should be sorted in descending order by the `cmp` function.
+struct IntersectionByIterator<I1: Iterator, I2: Iterator, C> {
     iter1: Peekable<I1>,
     iter2: Peekable<I2>,
+    cmp: C,
 }
 
-impl<'index, I1, I2> Iterator for IntersectionRevsetIterator<I1, I2>
+impl<I1, I2, C> Iterator for IntersectionByIterator<I1, I2, C>
 where
-    I1: Iterator<Item = IndexEntry<'index>>,
-    I2: Iterator<Item = IndexEntry<'index>>,
+    I1: Iterator,
+    I2: Iterator,
+    C: FnMut(&I1::Item, &I2::Item) -> Ordering,
 {
-    type Item = IndexEntry<'index>;
+    type Item = I1::Item;
 
     fn next(&mut self) -> Option<Self::Item> {
         loop {
@@ -460,13 +489,13 @@ where
                 (_, None) => {
                     return None;
                 }
-                (Some(entry1), Some(entry2)) => match entry1.position().cmp(&entry2.position()) {
+                (Some(item1), Some(item2)) => match (self.cmp)(item1, item2) {
                     Ordering::Less => {
                         self.iter2.next();
                     }
                     Ordering::Equal => {
-                        self.iter1.next();
-                        return self.iter2.next();
+                        self.iter2.next();
+                        return self.iter1.next();
                     }
                     Ordering::Greater => {
                         self.iter1.next();
@@ -477,6 +506,23 @@ where
     }
 }
 
+fn intersection_by<I1, I2, C>(
+    iter1: I1,
+    iter2: I2,
+    cmp: C,
+) -> IntersectionByIterator<I1::IntoIter, I2::IntoIter, C>
+where
+    I1: IntoIterator,
+    I2: IntoIterator,
+    C: FnMut(&I1::Item, &I2::Item) -> Ordering,
+{
+    IntersectionByIterator {
+        iter1: iter1.into_iter().peekable(),
+        iter2: iter2.into_iter().peekable(),
+        cmp,
+    }
+}
+
 #[derive(Debug)]
 struct DifferenceRevset<S1, S2> {
     // The minuend (what to subtract from)
@@ -494,10 +540,11 @@ where
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
-        Box::new(DifferenceRevsetIterator {
-            iter1: self.set1.iter(index).peekable(),
-            iter2: self.set2.iter(index).peekable(),
-        })
+        Box::new(difference_by(
+            self.set1.iter(index),
+            self.set2.iter(index),
+            |entry1, entry2| entry1.position().cmp(&entry2.position()),
+        ))
     }
 
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
@@ -523,17 +570,22 @@ where
     }
 }
 
-struct DifferenceRevsetIterator<I1: Iterator, I2: Iterator> {
+/// Iterator that subtracts `iter2` items from `iter1`.
+///
+/// The input items should be sorted in descending order by the `cmp` function.
+struct DifferenceByIterator<I1: Iterator, I2: Iterator, C> {
     iter1: Peekable<I1>,
     iter2: Peekable<I2>,
+    cmp: C,
 }
 
-impl<'index, I1, I2> Iterator for DifferenceRevsetIterator<I1, I2>
+impl<I1, I2, C> Iterator for DifferenceByIterator<I1, I2, C>
 where
-    I1: Iterator<Item = IndexEntry<'index>>,
-    I2: Iterator<Item = IndexEntry<'index>>,
+    I1: Iterator,
+    I2: Iterator,
+    C: FnMut(&I1::Item, &I2::Item) -> Ordering,
 {
-    type Item = IndexEntry<'index>;
+    type Item = I1::Item;
 
     fn next(&mut self) -> Option<Self::Item> {
         loop {
@@ -544,7 +596,7 @@ where
                 (_, None) => {
                     return self.iter1.next();
                 }
-                (Some(entry1), Some(entry2)) => match entry1.position().cmp(&entry2.position()) {
+                (Some(item1), Some(item2)) => match (self.cmp)(item1, item2) {
                     Ordering::Less => {
                         self.iter2.next();
                     }
@@ -561,6 +613,23 @@ where
     }
 }
 
+fn difference_by<I1, I2, C>(
+    iter1: I1,
+    iter2: I2,
+    cmp: C,
+) -> DifferenceByIterator<I1::IntoIter, I2::IntoIter, C>
+where
+    I1: IntoIterator,
+    I2: IntoIterator,
+    C: FnMut(&I1::Item, &I2::Item) -> Ordering,
+{
+    DifferenceByIterator {
+        iter1: iter1.into_iter().peekable(),
+        iter2: iter2.into_iter().peekable(),
+        cmp,
+    }
+}
+
 pub fn evaluate<I: AsCompositeIndex>(
     expression: &ResolvedExpression,
     store: &Arc<Store>,

From fd7c52900d07235f8b9878d2fc2ad375aa654023 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Tue, 20 Feb 2024 13:19:02 +0900
Subject: [PATCH 2/5] revset: flip ordering of generic combination iterators

As a general-purpose iterator combinator, ascending order makes more sense.
---
 lib/src/default_index/revset_engine.rs | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs
index 240c6cbea5..d33e3f4499 100644
--- a/lib/src/default_index/revset_engine.rs
+++ b/lib/src/default_index/revset_engine.rs
@@ -341,7 +341,7 @@ where
         Box::new(union_by(
             self.set1.iter(index),
             self.set2.iter(index),
-            |entry1, entry2| entry1.position().cmp(&entry2.position()),
+            |entry1, entry2| entry1.position().cmp(&entry2.position()).reverse(),
         ))
     }
 
@@ -370,7 +370,7 @@ where
 
 /// Iterator that merges two sorted iterators.
 ///
-/// The input items should be sorted in descending order by the `cmp` function.
+/// The input items should be sorted in ascending order by the `cmp` function.
 struct UnionByIterator<I1: Iterator, I2: Iterator, C> {
     iter1: Peekable<I1>,
     iter2: Peekable<I2>,
@@ -390,12 +390,12 @@ where
             (None, _) => self.iter2.next(),
             (_, None) => self.iter1.next(),
             (Some(item1), Some(item2)) => match (self.cmp)(item1, item2) {
-                Ordering::Less => self.iter2.next(),
+                Ordering::Less => self.iter1.next(),
                 Ordering::Equal => {
                     self.iter2.next();
                     self.iter1.next()
                 }
-                Ordering::Greater => self.iter1.next(),
+                Ordering::Greater => self.iter2.next(),
             },
         }
     }
@@ -436,7 +436,7 @@ where
         Box::new(intersection_by(
             self.set1.iter(index),
             self.set2.iter(index),
-            |entry1, entry2| entry1.position().cmp(&entry2.position()),
+            |entry1, entry2| entry1.position().cmp(&entry2.position()).reverse(),
         ))
     }
 
@@ -465,7 +465,7 @@ where
 
 /// Iterator that intersects two sorted iterators.
 ///
-/// The input items should be sorted in descending order by the `cmp` function.
+/// The input items should be sorted in ascending order by the `cmp` function.
 struct IntersectionByIterator<I1: Iterator, I2: Iterator, C> {
     iter1: Peekable<I1>,
     iter2: Peekable<I2>,
@@ -491,14 +491,14 @@ where
                 }
                 (Some(item1), Some(item2)) => match (self.cmp)(item1, item2) {
                     Ordering::Less => {
-                        self.iter2.next();
+                        self.iter1.next();
                     }
                     Ordering::Equal => {
                         self.iter2.next();
                         return self.iter1.next();
                     }
                     Ordering::Greater => {
-                        self.iter1.next();
+                        self.iter2.next();
                     }
                 },
             }
@@ -543,7 +543,7 @@ where
         Box::new(difference_by(
             self.set1.iter(index),
             self.set2.iter(index),
-            |entry1, entry2| entry1.position().cmp(&entry2.position()),
+            |entry1, entry2| entry1.position().cmp(&entry2.position()).reverse(),
         ))
     }
 
@@ -572,7 +572,7 @@ where
 
 /// Iterator that subtracts `iter2` items from `iter1`.
 ///
-/// The input items should be sorted in descending order by the `cmp` function.
+/// The input items should be sorted in ascending order by the `cmp` function.
 struct DifferenceByIterator<I1: Iterator, I2: Iterator, C> {
     iter1: Peekable<I1>,
     iter2: Peekable<I2>,
@@ -598,14 +598,14 @@ where
                 }
                 (Some(item1), Some(item2)) => match (self.cmp)(item1, item2) {
                     Ordering::Less => {
-                        self.iter2.next();
+                        return self.iter1.next();
                     }
                     Ordering::Equal => {
                         self.iter2.next();
                         self.iter1.next();
                     }
                     Ordering::Greater => {
-                        return self.iter1.next();
+                        self.iter2.next();
                     }
                 },
             }

From 74d45dcf9251fdad6b4aceb9cc911cae60fdc3ff Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Tue, 20 Feb 2024 13:24:02 +0900
Subject: [PATCH 3/5] revset: add positions() iterator to InternalRevset

I just wanted to clean up the callers, but this might also be marginally
faster.
---
 lib/src/default_index/revset_engine.rs | 142 +++++++++++++++++++------
 1 file changed, 107 insertions(+), 35 deletions(-)

diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs
index d33e3f4499..d87631c6a6 100644
--- a/lib/src/default_index/revset_engine.rs
+++ b/lib/src/default_index/revset_engine.rs
@@ -62,6 +62,11 @@ trait InternalRevset: fmt::Debug + ToPredicateFn {
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a>;
 
+    fn positions<'a, 'index: 'a>(
+        &'a self,
+        index: CompositeIndex<'index>,
+    ) -> Box<dyn Iterator<Item = IndexPosition> + 'a>;
+
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
     where
         Self: 'a;
@@ -75,6 +80,13 @@ impl<T: InternalRevset + ?Sized> InternalRevset for Box<T> {
         <T as InternalRevset>::iter(self, index)
     }
 
+    fn positions<'a, 'index: 'a>(
+        &'a self,
+        index: CompositeIndex<'index>,
+    ) -> Box<dyn Iterator<Item = IndexPosition> + 'a> {
+        <T as InternalRevset>::positions(self, index)
+    }
+
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
     where
         Self: 'a,
@@ -97,6 +109,10 @@ impl<I: AsCompositeIndex> RevsetImpl<I> {
         self.inner.iter(self.index.as_composite())
     }
 
+    fn positions(&self) -> Box<dyn Iterator<Item = IndexPosition> + '_> {
+        self.inner.positions(self.index.as_composite())
+    }
+
     pub fn iter_graph_impl(&self) -> RevsetGraphIterator<'_, '_> {
         RevsetGraphIterator::new(self.index.as_composite(), self.entries())
     }
@@ -127,7 +143,7 @@ impl<I: AsCompositeIndex> Revset for RevsetImpl<I> {
     }
 
     fn is_empty(&self) -> bool {
-        self.entries().next().is_none()
+        self.positions().next().is_none()
     }
 
     fn count_estimate(&self) -> (usize, Option<usize>) {
@@ -135,14 +151,14 @@ impl<I: AsCompositeIndex> Revset for RevsetImpl<I> {
             // Exercise the estimation feature in tests. (If we ever have a Revset
             // implementation in production code that returns estimates, we can probably
             // remove this and rewrite the associated tests.)
-            let count = self.entries().take(10).count();
+            let count = self.positions().take(10).count();
             if count < 10 {
                 (count, Some(count))
             } else {
                 (10, None)
             }
         } else {
-            let count = self.iter().count();
+            let count = self.positions().count();
             (count, Some(count))
         }
     }
@@ -173,6 +189,13 @@ impl InternalRevset for EagerRevset {
         Box::new(entries)
     }
 
+    fn positions<'a, 'index: 'a>(
+        &'a self,
+        _index: CompositeIndex<'index>,
+    ) -> Box<dyn Iterator<Item = IndexPosition> + 'a> {
+        Box::new(self.positions.iter().copied())
+    }
+
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
     where
         Self: 'a,
@@ -230,6 +253,13 @@ where
         (self.walk)(index)
     }
 
+    fn positions<'a, 'index: 'a>(
+        &'a self,
+        index: CompositeIndex<'index>,
+    ) -> Box<dyn Iterator<Item = IndexPosition> + 'a> {
+        Box::new(self.iter(index).map(|entry| entry.position()))
+    }
+
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
     where
         Self: 'a,
@@ -287,6 +317,13 @@ where
         Box::new(self.candidates.iter(index).filter(p))
     }
 
+    fn positions<'a, 'index: 'a>(
+        &'a self,
+        index: CompositeIndex<'index>,
+    ) -> Box<dyn Iterator<Item = IndexPosition> + 'a> {
+        Box::new(self.iter(index).map(|entry| entry.position()))
+    }
+
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
     where
         Self: 'a,
@@ -345,6 +382,17 @@ where
         ))
     }
 
+    fn positions<'a, 'index: 'a>(
+        &'a self,
+        index: CompositeIndex<'index>,
+    ) -> Box<dyn Iterator<Item = IndexPosition> + 'a> {
+        Box::new(union_by(
+            self.set1.positions(index),
+            self.set2.positions(index),
+            |pos1, pos2| pos1.cmp(pos2).reverse(),
+        ))
+    }
+
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
     where
         Self: 'a,
@@ -435,8 +483,19 @@ where
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
         Box::new(intersection_by(
             self.set1.iter(index),
-            self.set2.iter(index),
-            |entry1, entry2| entry1.position().cmp(&entry2.position()).reverse(),
+            self.set2.positions(index),
+            |entry1, pos2| entry1.position().cmp(pos2).reverse(),
+        ))
+    }
+
+    fn positions<'a, 'index: 'a>(
+        &'a self,
+        index: CompositeIndex<'index>,
+    ) -> Box<dyn Iterator<Item = IndexPosition> + 'a> {
+        Box::new(intersection_by(
+            self.set1.positions(index),
+            self.set2.positions(index),
+            |pos1, pos2| pos1.cmp(pos2).reverse(),
         ))
     }
 
@@ -542,8 +601,19 @@ where
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
         Box::new(difference_by(
             self.set1.iter(index),
-            self.set2.iter(index),
-            |entry1, entry2| entry1.position().cmp(&entry2.position()).reverse(),
+            self.set2.positions(index),
+            |entry1, pos2| entry1.position().cmp(pos2).reverse(),
+        ))
+    }
+
+    fn positions<'a, 'index: 'a>(
+        &'a self,
+        index: CompositeIndex<'index>,
+    ) -> Box<dyn Iterator<Item = IndexPosition> + 'a> {
+        Box::new(difference_by(
+            self.set1.positions(index),
+            self.set2.positions(index),
+            |pos1, pos2| pos1.cmp(pos2).reverse(),
         ))
     }
 
@@ -671,10 +741,7 @@ impl<'index> EvaluationContext<'index> {
             }
             ResolvedExpression::Ancestors { heads, generation } => {
                 let head_set = self.evaluate(heads)?;
-                let head_positions = head_set
-                    .iter(index)
-                    .map(|entry| entry.position())
-                    .collect_vec();
+                let head_positions = head_set.positions(index).collect_vec();
                 if generation == &GENERATION_RANGE_FULL {
                     Ok(Box::new(RevWalkRevset::new(move |index| {
                         Box::new(index.walk_revs(&head_positions, &[]))
@@ -696,15 +763,9 @@ impl<'index> EvaluationContext<'index> {
                 generation,
             } => {
                 let root_set = self.evaluate(roots)?;
-                let root_positions = root_set
-                    .iter(index)
-                    .map(|entry| entry.position())
-                    .collect_vec();
+                let root_positions = root_set.positions(index).collect_vec();
                 let head_set = self.evaluate(heads)?;
-                let head_positions = head_set
-                    .iter(index)
-                    .map(|entry| entry.position())
-                    .collect_vec();
+                let head_positions = head_set.positions(index).collect_vec();
                 if generation == &GENERATION_RANGE_FULL {
                     Ok(Box::new(RevWalkRevset::new(move |index| {
                         Box::new(index.walk_revs(&head_positions, &root_positions))
@@ -726,15 +787,9 @@ impl<'index> EvaluationContext<'index> {
                 generation_from_roots,
             } => {
                 let root_set = self.evaluate(roots)?;
-                let root_positions = root_set
-                    .iter(index)
-                    .map(|entry| entry.position())
-                    .collect_vec();
+                let root_positions = root_set.positions(index).collect_vec();
                 let head_set = self.evaluate(heads)?;
-                let head_positions = head_set
-                    .iter(index)
-                    .map(|entry| entry.position())
-                    .collect_vec();
+                let head_positions = head_set.positions(index).collect_vec();
                 if generation_from_roots == &(1..2) {
                     let root_positions_set: HashSet<_> = root_positions.iter().copied().collect();
                     let candidates = RevWalkRevset::new(move |index| {
@@ -782,12 +837,8 @@ impl<'index> EvaluationContext<'index> {
             }
             ResolvedExpression::Heads(candidates) => {
                 let candidate_set = self.evaluate(candidates)?;
-                let head_positions: BTreeSet<_> = index.heads_pos(
-                    candidate_set
-                        .iter(index)
-                        .map(|entry| entry.position())
-                        .collect(),
-                );
+                let head_positions: BTreeSet<_> =
+                    index.heads_pos(candidate_set.positions(index).collect());
                 let positions = head_positions.into_iter().rev().collect();
                 Ok(Box::new(EagerRevset { positions }))
             }
@@ -1057,9 +1108,10 @@ mod tests {
 
         let get_pos = |id: &CommitId| index.as_composite().commit_id_to_pos(id).unwrap();
         let get_entry = |id: &CommitId| index.as_composite().entry_by_id(id).unwrap();
-        let make_entries = |ids: &[&CommitId]| ids.iter().map(|id| get_entry(id)).collect_vec();
+        let make_positions = |ids: &[&CommitId]| ids.iter().copied().map(get_pos).collect_vec();
+        let make_entries = |ids: &[&CommitId]| ids.iter().copied().map(get_entry).collect_vec();
         let make_set = |ids: &[&CommitId]| -> Box<dyn InternalRevset> {
-            let positions = ids.iter().copied().map(get_pos).collect();
+            let positions = make_positions(ids);
             Box::new(EagerRevset { positions })
         };
 
@@ -1084,6 +1136,10 @@ mod tests {
             set.iter(index.as_composite()).collect_vec(),
             make_entries(&[&id_2, &id_0])
         );
+        assert_eq!(
+            set.positions(index.as_composite()).collect_vec(),
+            make_positions(&[&id_2, &id_0])
+        );
         let mut p = set.to_predicate_fn(index.as_composite());
         assert!(!p(&get_entry(&id_4)));
         assert!(!p(&get_entry(&id_3)));
@@ -1100,6 +1156,10 @@ mod tests {
             set.iter(index.as_composite()).collect_vec(),
             make_entries(&[&id_2])
         );
+        assert_eq!(
+            set.positions(index.as_composite()).collect_vec(),
+            make_positions(&[&id_2])
+        );
         let mut p = set.to_predicate_fn(index.as_composite());
         assert!(!p(&get_entry(&id_4)));
         assert!(!p(&get_entry(&id_3)));
@@ -1115,6 +1175,10 @@ mod tests {
             set.iter(index.as_composite()).collect_vec(),
             make_entries(&[&id_4, &id_3, &id_2, &id_1])
         );
+        assert_eq!(
+            set.positions(index.as_composite()).collect_vec(),
+            make_positions(&[&id_4, &id_3, &id_2, &id_1])
+        );
         let mut p = set.to_predicate_fn(index.as_composite());
         assert!(p(&get_entry(&id_4)));
         assert!(p(&get_entry(&id_3)));
@@ -1130,6 +1194,10 @@ mod tests {
             set.iter(index.as_composite()).collect_vec(),
             make_entries(&[&id_2])
         );
+        assert_eq!(
+            set.positions(index.as_composite()).collect_vec(),
+            make_positions(&[&id_2])
+        );
         let mut p = set.to_predicate_fn(index.as_composite());
         assert!(!p(&get_entry(&id_4)));
         assert!(!p(&get_entry(&id_3)));
@@ -1145,6 +1213,10 @@ mod tests {
             set.iter(index.as_composite()).collect_vec(),
             make_entries(&[&id_4, &id_0])
         );
+        assert_eq!(
+            set.positions(index.as_composite()).collect_vec(),
+            make_positions(&[&id_4, &id_0])
+        );
         let mut p = set.to_predicate_fn(index.as_composite());
         assert!(p(&get_entry(&id_4)));
         assert!(!p(&get_entry(&id_3)));

From c93d9efa6c7fcaebe8d7f128bf29f30d42bf181f Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Tue, 20 Feb 2024 13:55:06 +0900
Subject: [PATCH 4/5] revset: rename InternalRevset::iter() to ::entries()

---
 lib/src/default_index/revset_engine.rs | 50 +++++++++++++-------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs
index d87631c6a6..e186e60fbd 100644
--- a/lib/src/default_index/revset_engine.rs
+++ b/lib/src/default_index/revset_engine.rs
@@ -57,7 +57,7 @@ impl<T: ToPredicateFn + ?Sized> ToPredicateFn for Box<T> {
 
 trait InternalRevset: fmt::Debug + ToPredicateFn {
     // All revsets currently iterate in order of descending index position
-    fn iter<'a, 'index: 'a>(
+    fn entries<'a, 'index: 'a>(
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a>;
@@ -73,11 +73,11 @@ trait InternalRevset: fmt::Debug + ToPredicateFn {
 }
 
 impl<T: InternalRevset + ?Sized> InternalRevset for Box<T> {
-    fn iter<'a, 'index: 'a>(
+    fn entries<'a, 'index: 'a>(
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
-        <T as InternalRevset>::iter(self, index)
+        <T as InternalRevset>::entries(self, index)
     }
 
     fn positions<'a, 'index: 'a>(
@@ -106,7 +106,7 @@ impl<I: AsCompositeIndex> RevsetImpl<I> {
     }
 
     fn entries(&self) -> Box<dyn Iterator<Item = IndexEntry<'_>> + '_> {
-        self.inner.iter(self.index.as_composite())
+        self.inner.entries(self.index.as_composite())
     }
 
     fn positions(&self) -> Box<dyn Iterator<Item = IndexPosition> + '_> {
@@ -178,7 +178,7 @@ impl EagerRevset {
 }
 
 impl InternalRevset for EagerRevset {
-    fn iter<'a, 'index: 'a>(
+    fn entries<'a, 'index: 'a>(
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
@@ -246,7 +246,7 @@ impl<F> InternalRevset for RevWalkRevset<F>
 where
     F: Fn(CompositeIndex<'_>) -> Box<dyn Iterator<Item = IndexEntry<'_>> + '_>,
 {
-    fn iter<'a, 'index: 'a>(
+    fn entries<'a, 'index: 'a>(
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
@@ -257,7 +257,7 @@ where
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexPosition> + 'a> {
-        Box::new(self.iter(index).map(|entry| entry.position()))
+        Box::new(self.entries(index).map(|entry| entry.position()))
     }
 
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
@@ -276,7 +276,7 @@ where
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn FnMut(&IndexEntry<'_>) -> bool + 'a> {
-        predicate_fn_from_entries(self.iter(index))
+        predicate_fn_from_entries(self.entries(index))
     }
 }
 
@@ -309,19 +309,19 @@ where
     S: InternalRevset,
     P: ToPredicateFn,
 {
-    fn iter<'a, 'index: 'a>(
+    fn entries<'a, 'index: 'a>(
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
         let p = self.predicate.to_predicate_fn(index);
-        Box::new(self.candidates.iter(index).filter(p))
+        Box::new(self.candidates.entries(index).filter(p))
     }
 
     fn positions<'a, 'index: 'a>(
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexPosition> + 'a> {
-        Box::new(self.iter(index).map(|entry| entry.position()))
+        Box::new(self.entries(index).map(|entry| entry.position()))
     }
 
     fn into_predicate<'a>(self: Box<Self>) -> Box<dyn ToPredicateFn + 'a>
@@ -371,13 +371,13 @@ where
     S1: InternalRevset,
     S2: InternalRevset,
 {
-    fn iter<'a, 'index: 'a>(
+    fn entries<'a, 'index: 'a>(
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
         Box::new(union_by(
-            self.set1.iter(index),
-            self.set2.iter(index),
+            self.set1.entries(index),
+            self.set2.entries(index),
             |entry1, entry2| entry1.position().cmp(&entry2.position()).reverse(),
         ))
     }
@@ -477,12 +477,12 @@ where
     S1: InternalRevset,
     S2: InternalRevset,
 {
-    fn iter<'a, 'index: 'a>(
+    fn entries<'a, 'index: 'a>(
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
         Box::new(intersection_by(
-            self.set1.iter(index),
+            self.set1.entries(index),
             self.set2.positions(index),
             |entry1, pos2| entry1.position().cmp(pos2).reverse(),
         ))
@@ -595,12 +595,12 @@ where
     S1: InternalRevset,
     S2: InternalRevset,
 {
-    fn iter<'a, 'index: 'a>(
+    fn entries<'a, 'index: 'a>(
         &'a self,
         index: CompositeIndex<'index>,
     ) -> Box<dyn Iterator<Item = IndexEntry<'index>> + 'a> {
         Box::new(difference_by(
-            self.set1.iter(index),
+            self.set1.entries(index),
             self.set2.positions(index),
             |entry1, pos2| entry1.position().cmp(pos2).reverse(),
         ))
@@ -843,7 +843,7 @@ impl<'index> EvaluationContext<'index> {
                 Ok(Box::new(EagerRevset { positions }))
             }
             ResolvedExpression::Roots(candidates) => {
-                let candidate_entries = self.evaluate(candidates)?.iter(index).collect_vec();
+                let candidate_entries = self.evaluate(candidates)?.entries(index).collect_vec();
                 let candidate_positions = candidate_entries
                     .iter()
                     .map(|entry| entry.position())
@@ -950,7 +950,7 @@ impl<'index> EvaluationContext<'index> {
         // Maintain min-heap containing the latest (greatest) count items. For small
         // count and large candidate set, this is probably cheaper than building vec
         // and applying selection algorithm.
-        let mut candidate_iter = candidate_set.iter(self.index).map(make_rev_item).fuse();
+        let mut candidate_iter = candidate_set.entries(self.index).map(make_rev_item).fuse();
         let mut latest_items = BinaryHeap::from_iter(candidate_iter.by_ref().take(count));
         for item in candidate_iter {
             let mut earliest = latest_items.peek_mut().unwrap();
@@ -1133,7 +1133,7 @@ mod tests {
             predicate: as_pure_predicate_fn(|_index, entry| entry.commit_id() != id_4),
         };
         assert_eq!(
-            set.iter(index.as_composite()).collect_vec(),
+            set.entries(index.as_composite()).collect_vec(),
             make_entries(&[&id_2, &id_0])
         );
         assert_eq!(
@@ -1153,7 +1153,7 @@ mod tests {
             predicate: make_set(&[&id_3, &id_2, &id_1]),
         };
         assert_eq!(
-            set.iter(index.as_composite()).collect_vec(),
+            set.entries(index.as_composite()).collect_vec(),
             make_entries(&[&id_2])
         );
         assert_eq!(
@@ -1172,7 +1172,7 @@ mod tests {
             set2: make_set(&[&id_3, &id_2, &id_1]),
         };
         assert_eq!(
-            set.iter(index.as_composite()).collect_vec(),
+            set.entries(index.as_composite()).collect_vec(),
             make_entries(&[&id_4, &id_3, &id_2, &id_1])
         );
         assert_eq!(
@@ -1191,7 +1191,7 @@ mod tests {
             set2: make_set(&[&id_3, &id_2, &id_1]),
         };
         assert_eq!(
-            set.iter(index.as_composite()).collect_vec(),
+            set.entries(index.as_composite()).collect_vec(),
             make_entries(&[&id_2])
         );
         assert_eq!(
@@ -1210,7 +1210,7 @@ mod tests {
             set2: make_set(&[&id_3, &id_2, &id_1]),
         };
         assert_eq!(
-            set.iter(index.as_composite()).collect_vec(),
+            set.entries(index.as_composite()).collect_vec(),
             make_entries(&[&id_4, &id_0])
         );
         assert_eq!(

From 2ec0fc99cc1f6e6ca082ae07b1e34f7503cbf9b2 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Mon, 19 Feb 2024 22:54:10 +0900
Subject: [PATCH 5/5] revset: ad-hoc optimization for range queries containing
 unwanted wanted heads
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In my linux stable mirror, this makes the default log revset evaluation super
fast. immutable_heads(), if configured properly, includes many historical
branch heads which are also the visible heads.

revsets/immutable_heads()..
---------------------------
0     12.27     117.1±0.77m
3      1.00       9.5±0.08m
---
 cli/testing/bench-revsets-git.txt      |  2 ++
 lib/src/default_index/revset_engine.rs | 10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/cli/testing/bench-revsets-git.txt b/cli/testing/bench-revsets-git.txt
index 602afa143e..ff474c4708 100644
--- a/cli/testing/bench-revsets-git.txt
+++ b/cli/testing/bench-revsets-git.txt
@@ -16,6 +16,8 @@ v2.39.0::v2.40.0
 # Tags and branches
 tags()
 branches()
+# Local changes
+(tags() | remote_branches())..
 # Intersection of range with a small subset
 tags() & ::v2.40.0
 v2.39.0 & ::v2.40.0
diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs
index e186e60fbd..f5644c6e9a 100644
--- a/lib/src/default_index/revset_engine.rs
+++ b/lib/src/default_index/revset_engine.rs
@@ -764,8 +764,16 @@ impl<'index> EvaluationContext<'index> {
             } => {
                 let root_set = self.evaluate(roots)?;
                 let root_positions = root_set.positions(index).collect_vec();
+                // Pre-filter heads so queries like 'immutable_heads()..' can
+                // terminate early. immutable_heads() usually includes some
+                // visible heads, which can be trivially rejected.
                 let head_set = self.evaluate(heads)?;
-                let head_positions = head_set.positions(index).collect_vec();
+                let head_positions = difference_by(
+                    head_set.positions(index),
+                    root_positions.iter().copied(),
+                    |pos1, pos2| pos1.cmp(pos2).reverse(),
+                )
+                .collect_vec();
                 if generation == &GENERATION_RANGE_FULL {
                     Ok(Box::new(RevWalkRevset::new(move |index| {
                         Box::new(index.walk_revs(&head_positions, &root_positions))