index: turn CompositeIndex::walk_revs() into position-based API

This gets rid of round-trip conversion from queries like "(main..)-". I have
such expression in my default log/disambiguation revset, and the query could
take ~150ms to convert head positions back and forth if the repository had
tons of unmerged commits.

lib/src/default_index_store.rs

@@ -888,7 +888,7 @@ impl<'a> CompositeIndex<'a> {
         }
     }
 
-    fn commit_id_to_pos(&self, commit_id: &CommitId) -> Option<IndexPosition> {
+    pub fn commit_id_to_pos(&self, commit_id: &CommitId) -> Option<IndexPosition> {
         self.ancestor_index_segments()
             .find_map(|segment| segment.segment_commit_id_to_pos(commit_id))
     }
@@ -991,12 +991,12 @@ impl<'a> CompositeIndex<'a> {
         self.heads_pos(result)
     }
 
-    pub fn walk_revs(&self, wanted: &[CommitId], unwanted: &[CommitId]) -> RevWalk<'a> {
+    pub fn walk_revs(&self, wanted: &[IndexPosition], unwanted: &[IndexPosition]) -> RevWalk<'a> {
         let mut rev_walk = RevWalk::new(*self);
-        for pos in wanted.iter().map(|id| self.commit_id_to_pos(id).unwrap()) {
+        for &pos in wanted {
             rev_walk.add_wanted(pos);
         }
-        for pos in unwanted.iter().map(|id| self.commit_id_to_pos(id).unwrap()) {
+        for &pos in unwanted {
             rev_walk.add_unwanted(pos);
         }
         rev_walk
@@ -2041,6 +2041,13 @@ mod tests {
         move || iter.next().unwrap()
     }
 
+    fn to_positions_vec(index: CompositeIndex<'_>, commit_ids: &[CommitId]) -> Vec<IndexPosition> {
+        commit_ids
+            .iter()
+            .map(|id| index.commit_id_to_pos(id).unwrap())
+            .collect()
+    }
+
     #[test_case(false; "memory")]
     #[test_case(true; "file")]
     fn index_empty(on_disk: bool) {
@@ -2743,9 +2750,11 @@ mod tests {
         index.add_commit_data(id_5.clone(), new_change_id(), &[id_4.clone(), id_2.clone()]);
 
         let walk_commit_ids = |wanted: &[CommitId], unwanted: &[CommitId]| {
+            let index = index.as_composite();
+            let wanted_positions = to_positions_vec(index, wanted);
+            let unwanted_positions = to_positions_vec(index, unwanted);
             index
-                .as_composite()
-                .walk_revs(wanted, unwanted)
+                .walk_revs(&wanted_positions, &unwanted_positions)
                 .map(|entry| entry.commit_id())
                 .collect_vec()
         };
@@ -2833,9 +2842,11 @@ mod tests {
         index.add_commit_data(id_8.clone(), new_change_id(), &[id_7.clone()]);
 
         let walk_commit_ids = |wanted: &[CommitId], unwanted: &[CommitId], range: Range<u32>| {
+            let index = index.as_composite();
+            let wanted_positions = to_positions_vec(index, wanted);
+            let unwanted_positions = to_positions_vec(index, unwanted);
             index
-                .as_composite()
-                .walk_revs(wanted, unwanted)
+                .walk_revs(&wanted_positions, &unwanted_positions)
                 .filter_by_generation(range)
                 .map(|entry| entry.commit_id())
                 .collect_vec()
@@ -2910,9 +2921,10 @@ mod tests {
         );
 
         let walk_commit_ids = |wanted: &[CommitId], range: Range<u32>| {
+            let index = index.as_composite();
+            let wanted_positions = to_positions_vec(index, wanted);
             index
-                .as_composite()
-                .walk_revs(wanted, &[])
+                .walk_revs(&wanted_positions, &[])
                 .filter_by_generation(range)
                 .map(|entry| entry.commit_id())
                 .collect_vec()
@@ -2980,13 +2992,11 @@ mod tests {
 
         let visible_heads = [&id_6, &id_8].map(Clone::clone);
         let walk_commit_ids = |roots: &[CommitId], heads: &[CommitId], range: Range<u32>| {
-            let root_positions = roots
-                .iter()
-                .map(|id| index.as_composite().commit_id_to_pos(id).unwrap())
-                .collect_vec();
+            let index = index.as_composite();
+            let root_positions = to_positions_vec(index, roots);
+            let head_positions = to_positions_vec(index, heads);
             index
-                .as_composite()
-                .walk_revs(heads, &[])
+                .walk_revs(&head_positions, &[])
                 .descendants_filtered_by_generation(&root_positions, range)
                 .map(|entry| entry.commit_id())
                 .collect_vec()

lib/src/default_revset_engine.rs

@@ -535,10 +535,10 @@ impl<'index> EvaluationContext<'index> {
                 generation,
             } => {
                 let root_set = self.evaluate(roots)?;
-                let root_ids = root_set.iter().map(|entry| entry.commit_id()).collect_vec();
+                let root_positions = root_set.iter().map(|entry| entry.position()).collect_vec();
                 let head_set = self.evaluate(heads)?;
-                let head_ids = head_set.iter().map(|entry| entry.commit_id()).collect_vec();
-                let walk = self.index.walk_revs(&head_ids, &root_ids);
+                let head_positions = head_set.iter().map(|entry| entry.position()).collect_vec();
+                let walk = self.index.walk_revs(&head_positions, &root_positions);
                 if generation == &GENERATION_RANGE_FULL {
                     Ok(Box::new(RevWalkRevset { walk }))
                 } else {
@@ -662,8 +662,8 @@ impl<'index> EvaluationContext<'index> {
     where
         S: InternalRevset<'a> + ?Sized,
     {
-        let head_ids = head_set.iter().map(|entry| entry.commit_id()).collect_vec();
-        self.index.walk_revs(&head_ids, &[])
+        let head_positions = head_set.iter().map(|entry| entry.position()).collect_vec();
+        self.index.walk_revs(&head_positions, &[])
     }
 
     fn walk_children<'a, 'b, S, T>(

lib/tests/test_index.rs

@@ -17,7 +17,9 @@ use std::sync::Arc;
 use jujutsu_lib::backend::CommitId;
 use jujutsu_lib::commit::Commit;
 use jujutsu_lib::commit_builder::CommitBuilder;
-use jujutsu_lib::default_index_store::{CompositeIndex, MutableIndexImpl, ReadonlyIndexWrapper};
+use jujutsu_lib::default_index_store::{
+    CompositeIndex, IndexPosition, MutableIndexImpl, ReadonlyIndexWrapper,
+};
 use jujutsu_lib::index::Index as _;
 use jujutsu_lib::repo::{MutableRepo, ReadonlyRepo, Repo};
 use jujutsu_lib::settings::UserSettings;
@@ -39,6 +41,13 @@ fn generation_number(index: CompositeIndex, commit_id: &CommitId) -> u32 {
     index.entry_by_id(commit_id).unwrap().generation_number()
 }
 
+fn to_positions_vec(index: CompositeIndex<'_>, commit_ids: &[CommitId]) -> Vec<IndexPosition> {
+    commit_ids
+        .iter()
+        .map(|id| index.commit_id_to_pos(id).unwrap())
+        .collect()
+}
+
 #[test_case(false ; "local backend")]
 #[test_case(true ; "git backend")]
 fn test_index_commits_empty_repo(use_git: bool) {
@@ -188,72 +197,68 @@ fn test_index_commits_criss_cross(use_git: bool) {
         }
     }
 
+    let walk_revs = |wanted: &[CommitId], unwanted: &[CommitId]| {
+        let wanted_positions = to_positions_vec(index, wanted);
+        let unwanted_positions = to_positions_vec(index, unwanted);
+        index.walk_revs(&wanted_positions, &unwanted_positions)
+    };
+
     // RevWalk deduplicates chains by entry.
     assert_eq!(
-        index
-            .walk_revs(&[left_commits[num_generations - 1].id().clone()], &[])
-            .count(),
+        walk_revs(&[left_commits[num_generations - 1].id().clone()], &[]).count(),
         2 * num_generations
     );
     assert_eq!(
-        index
-            .walk_revs(&[right_commits[num_generations - 1].id().clone()], &[])
-            .count(),
+        walk_revs(&[right_commits[num_generations - 1].id().clone()], &[]).count(),
         2 * num_generations
     );
     assert_eq!(
-        index
-            .walk_revs(
-                &[left_commits[num_generations - 1].id().clone()],
-                &[left_commits[num_generations - 2].id().clone()]
-            )
-            .count(),
+        walk_revs(
+            &[left_commits[num_generations - 1].id().clone()],
+            &[left_commits[num_generations - 2].id().clone()]
+        )
+        .count(),
         2
     );
     assert_eq!(
-        index
-            .walk_revs(
-                &[right_commits[num_generations - 1].id().clone()],
-                &[right_commits[num_generations - 2].id().clone()]
-            )
-            .count(),
+        walk_revs(
+            &[right_commits[num_generations - 1].id().clone()],
+            &[right_commits[num_generations - 2].id().clone()]
+        )
+        .count(),
         2
     );
 
     // RevWalkGenerationRange deduplicates chains by (entry, generation), which may
     // be more expensive than RevWalk, but should still finish in reasonable time.
     assert_eq!(
-        index
-            .walk_revs(&[left_commits[num_generations - 1].id().clone()], &[])
+        walk_revs(&[left_commits[num_generations - 1].id().clone()], &[])
             .filter_by_generation(0..(num_generations + 1) as u32)
             .count(),
         2 * num_generations
     );
     assert_eq!(
-        index
-            .walk_revs(&[right_commits[num_generations - 1].id().clone()], &[])
+        walk_revs(&[right_commits[num_generations - 1].id().clone()], &[])
             .filter_by_generation(0..(num_generations + 1) as u32)
             .count(),
         2 * num_generations
     );
     assert_eq!(
-        index
-            .walk_revs(
-                &[left_commits[num_generations - 1].id().clone()],
-                &[left_commits[num_generations - 2].id().clone()]
-            )
-            .filter_by_generation(0..(num_generations + 1) as u32)
-            .count(),
+        walk_revs(
+            &[left_commits[num_generations - 1].id().clone()],
+            &[left_commits[num_generations - 2].id().clone()]
+        )
+        .filter_by_generation(0..(num_generations + 1) as u32)
+        .count(),
         2
     );
     assert_eq!(
-        index
-            .walk_revs(
-                &[right_commits[num_generations - 1].id().clone()],
-                &[right_commits[num_generations - 2].id().clone()]
-            )
-            .filter_by_generation(0..(num_generations + 1) as u32)
-            .count(),
+        walk_revs(
+            &[right_commits[num_generations - 1].id().clone()],
+            &[right_commits[num_generations - 2].id().clone()]
+        )
+        .filter_by_generation(0..(num_generations + 1) as u32)
+        .count(),
         2
     );
 }