From 8480ee9e05103c37c82878b73ce309f1a34728d0 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 8 Mar 2024 17:54:23 +0900 Subject: [PATCH] index: migrate RevWalk constructors to builder API The current RevWalk constructors insert intermediate items to BinaryHeap and convert them as needed. This is redundant, and I'm going to add another parameter that should be applied to the queue first. That's why I decided to factor out a builder type. I considered adding a few set of factory functions that receive all parameters, but they looked messy because most of the parameters are of [IndexPosition] type. This patch also adds must_use to the builder and its return types, which are all iterator-like. --- lib/src/default_index/composite.rs | 13 +-- lib/src/default_index/rev_walk.rs | 112 +++++++++++++++---------- lib/src/default_index/revset_engine.rs | 51 ++++++----- 3 files changed, 102 insertions(+), 74 deletions(-) diff --git a/lib/src/default_index/composite.rs b/lib/src/default_index/composite.rs index c4e7c7f74..39fe1565a 100644 --- a/lib/src/default_index/composite.rs +++ b/lib/src/default_index/composite.rs @@ -26,7 +26,7 @@ use super::entry::{ SmallLocalPositionsVec, }; use super::readonly::ReadonlyIndexSegment; -use super::rev_walk::{AncestorsBitSet, RevWalkAncestors}; +use super::rev_walk::AncestorsBitSet; use super::revset_engine; use crate::backend::{ChangeId, CommitId}; use crate::hex_util; @@ -330,17 +330,6 @@ impl<'a> CompositeIndex<'a> { self.heads_pos(result) } - pub(super) fn walk_revs( - &self, - wanted: &[IndexPosition], - unwanted: &[IndexPosition], - ) -> RevWalkAncestors<'a> { - let mut rev_walk = RevWalkAncestors::new(*self); - rev_walk.extend_wanted(wanted.iter().copied()); - rev_walk.extend_unwanted(unwanted.iter().copied()); - rev_walk - } - pub(super) fn all_heads(self) -> impl Iterator + 'a { self.all_heads_pos() .map(move |pos| self.entry_by_pos(pos).commit_id()) diff --git a/lib/src/default_index/rev_walk.rs b/lib/src/default_index/rev_walk.rs index d9bb51cac..1333b86d9 100644 --- a/lib/src/default_index/rev_walk.rs +++ b/lib/src/default_index/rev_walk.rs @@ -199,32 +199,44 @@ impl RevWalkQueue { } } -pub(super) type RevWalkAncestors<'a> = RevWalkImpl<'a, CompositeIndex<'a>>; - #[derive(Clone)] -pub(super) struct RevWalkImpl<'a, I: RevWalkIndex<'a>> { - index: I, - queue: RevWalkQueue, +#[must_use] +pub(super) struct RevWalkBuilder<'a> { + index: CompositeIndex<'a>, + // TODO: Use Vec<_> until queued item type is settled. + queue: RevWalkQueue, } -impl<'a> RevWalkAncestors<'a> { - pub(super) fn new(index: CompositeIndex<'a>) -> Self { +impl<'a> RevWalkBuilder<'a> { + pub fn new(index: CompositeIndex<'a>) -> Self { let queue = RevWalkQueue::new(); - RevWalkImpl { index, queue } + RevWalkBuilder { index, queue } } - pub(super) fn extend_wanted(&mut self, positions: impl IntoIterator) { + /// Adds head positions to be included. + pub fn wanted_heads(mut self, positions: impl IntoIterator) -> Self { self.queue.extend_wanted(positions, ()); + self } - pub(super) fn extend_unwanted(&mut self, positions: impl IntoIterator) { + /// Adds root positions to be excluded. The roots precede the heads. + pub fn unwanted_roots(mut self, positions: impl IntoIterator) -> Self { self.queue.extend_unwanted(positions); + self } - /// Filters entries by generation (or depth from the current wanted set.) + /// Walks ancestors. + pub fn ancestors(self) -> RevWalkAncestors<'a> { + RevWalkImpl { + index: self.index, + queue: self.queue, + } + } + + /// Walks ancestors within the `generation_range`. /// - /// The generation of the current wanted entries starts from 0. - pub fn filter_by_generation( + /// A generation number counts from the heads. + pub fn ancestors_filtered_by_generation( self, generation_range: Range, ) -> RevWalkAncestorsGenerationRange<'a> { @@ -236,7 +248,7 @@ impl<'a> RevWalkAncestors<'a> { /// /// Use this if you are only interested in descendants of the given roots. /// The caller still needs to filter out unwanted entries. - pub fn take_until_roots( + pub fn ancestors_until_roots( self, root_positions: &[IndexPosition], ) -> impl Iterator> + Clone + 'a { @@ -244,36 +256,45 @@ impl<'a> RevWalkAncestors<'a> { // it will perform better for unbalanced branchy history. // https://github.com/martinvonz/jj/pull/1492#discussion_r1160678325 let bottom_position = *root_positions.iter().min().unwrap_or(&IndexPosition::MAX); - self.take_while(move |entry| entry.position() >= bottom_position) + self.ancestors() + .take_while(move |entry| entry.position() >= bottom_position) } - /// Fully consumes the ancestors and walks back from `root_positions`. + /// Fully consumes ancestors and walks back from the `root_positions`. /// /// The returned iterator yields entries in order of ascending index /// position. - pub fn descendants(self, root_positions: &[IndexPosition]) -> RevWalkDescendants<'a> { + pub fn descendants( + self, + root_positions: impl IntoIterator, + ) -> RevWalkDescendants<'a> { + // TODO: collect HashSet<_> directly + let root_positions = Vec::from_iter(root_positions); RevWalkDescendants { - candidate_entries: self.take_until_roots(root_positions).collect(), - root_positions: root_positions.iter().copied().collect(), + candidate_entries: self.ancestors_until_roots(&root_positions).collect(), + root_positions: root_positions.into_iter().collect(), reachable_positions: HashSet::new(), } } - /// Fully consumes the ancestors and walks back from `root_positions` within - /// `generation_range`. + /// Fully consumes ancestors and walks back from the `root_positions` within + /// the `generation_range`. + /// + /// A generation number counts from the roots. /// /// The returned iterator yields entries in order of ascending index /// position. pub fn descendants_filtered_by_generation( self, - root_positions: &[IndexPosition], + root_positions: impl IntoIterator, generation_range: Range, ) -> RevWalkDescendantsGenerationRange<'a> { let index = self.index; - let entries = self.take_until_roots(root_positions); + let root_positions = Vec::from_iter(root_positions); + let entries = self.ancestors_until_roots(&root_positions); let descendants_index = RevWalkDescendantsIndex::build(index, entries); let mut queue = RevWalkQueue::new(); - for &pos in root_positions { + for pos in root_positions { // Do not add unreachable roots which shouldn't be visited if descendants_index.contains_pos(pos) { queue.push_wanted(Reverse(pos), ()); @@ -283,6 +304,15 @@ impl<'a> RevWalkAncestors<'a> { } } +pub(super) type RevWalkAncestors<'a> = RevWalkImpl<'a, CompositeIndex<'a>>; + +#[derive(Clone)] +#[must_use] +pub(super) struct RevWalkImpl<'a, I: RevWalkIndex<'a>> { + index: I, + queue: RevWalkQueue, +} + impl<'a, I: RevWalkIndex<'a>> Iterator for RevWalkImpl<'a, I> { type Item = IndexEntry<'a>; @@ -319,6 +349,7 @@ pub(super) type RevWalkDescendantsGenerationRange<'a> = RevWalkGenerationRangeImpl<'a, RevWalkDescendantsIndex<'a>>; #[derive(Clone)] +#[must_use] pub(super) struct RevWalkGenerationRangeImpl<'a, I: RevWalkIndex<'a>> { index: I, // Sort item generations in ascending order @@ -438,6 +469,7 @@ impl RevWalkItemGenerationRange { /// Walks descendants from the roots, in order of ascending index position. #[derive(Clone)] +#[must_use] pub(super) struct RevWalkDescendants<'a> { candidate_entries: Vec>, root_positions: HashSet, @@ -602,10 +634,10 @@ mod tests { 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 - .walk_revs(&wanted_positions, &unwanted_positions) + RevWalkBuilder::new(index) + .wanted_heads(to_positions_vec(index, wanted)) + .unwanted_roots(to_positions_vec(index, unwanted)) + .ancestors() .map(|entry| entry.commit_id()) .collect_vec() }; @@ -694,11 +726,10 @@ mod tests { let walk_commit_ids = |wanted: &[CommitId], unwanted: &[CommitId], range: Range| { let index = index.as_composite(); - let wanted_positions = to_positions_vec(index, wanted); - let unwanted_positions = to_positions_vec(index, unwanted); - index - .walk_revs(&wanted_positions, &unwanted_positions) - .filter_by_generation(range) + RevWalkBuilder::new(index) + .wanted_heads(to_positions_vec(index, wanted)) + .unwanted_roots(to_positions_vec(index, unwanted)) + .ancestors_filtered_by_generation(range) .map(|entry| entry.commit_id()) .collect_vec() }; @@ -773,10 +804,9 @@ mod tests { let walk_commit_ids = |wanted: &[CommitId], range: Range| { let index = index.as_composite(); - let wanted_positions = to_positions_vec(index, wanted); - index - .walk_revs(&wanted_positions, &[]) - .filter_by_generation(range) + RevWalkBuilder::new(index) + .wanted_heads(to_positions_vec(index, wanted)) + .ancestors_filtered_by_generation(range) .map(|entry| entry.commit_id()) .collect_vec() }; @@ -844,11 +874,9 @@ mod tests { let visible_heads = [&id_6, &id_8].map(Clone::clone); let walk_commit_ids = |roots: &[CommitId], heads: &[CommitId], range: Range| { let index = index.as_composite(); - let root_positions = to_positions_vec(index, roots); - let head_positions = to_positions_vec(index, heads); - index - .walk_revs(&head_positions, &[]) - .descendants_filtered_by_generation(&root_positions, range) + RevWalkBuilder::new(index) + .wanted_heads(to_positions_vec(index, heads)) + .descendants_filtered_by_generation(to_positions_vec(index, roots), range) .map(|entry| entry.commit_id()) .collect_vec() }; diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs index d277516c5..8ed5bad8d 100644 --- a/lib/src/default_index/revset_engine.rs +++ b/lib/src/default_index/revset_engine.rs @@ -23,6 +23,7 @@ use std::sync::Arc; use itertools::Itertools; +use super::rev_walk::RevWalkBuilder; use super::revset_graph_iterator::RevsetGraphIterator; use crate::backend::{ChangeId, CommitId, MillisSinceEpoch}; use crate::default_index::{AsCompositeIndex, CompositeIndex, IndexEntry, IndexPosition}; @@ -744,15 +745,19 @@ impl<'index> EvaluationContext<'index> { 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, &[])) + Box::new( + RevWalkBuilder::new(index) + .wanted_heads(head_positions.iter().copied()) + .ancestors(), + ) }))) } else { let generation = to_u32_generation_range(generation)?; Ok(Box::new(RevWalkRevset::new(move |index| { Box::new( - index - .walk_revs(&head_positions, &[]) - .filter_by_generation(generation.clone()), + RevWalkBuilder::new(index) + .wanted_heads(head_positions.iter().copied()) + .ancestors_filtered_by_generation(generation.clone()), ) }))) } @@ -776,15 +781,21 @@ impl<'index> EvaluationContext<'index> { .collect_vec(); if generation == &GENERATION_RANGE_FULL { Ok(Box::new(RevWalkRevset::new(move |index| { - Box::new(index.walk_revs(&head_positions, &root_positions)) + Box::new( + RevWalkBuilder::new(index) + .wanted_heads(head_positions.iter().copied()) + .unwanted_roots(root_positions.iter().copied()) + .ancestors(), + ) }))) } else { let generation = to_u32_generation_range(generation)?; Ok(Box::new(RevWalkRevset::new(move |index| { Box::new( - index - .walk_revs(&head_positions, &root_positions) - .filter_by_generation(generation.clone()), + RevWalkBuilder::new(index) + .wanted_heads(head_positions.iter().copied()) + .unwanted_roots(root_positions.iter().copied()) + .ancestors_filtered_by_generation(generation.clone()), ) }))) } @@ -802,9 +813,9 @@ impl<'index> EvaluationContext<'index> { let root_positions_set: HashSet<_> = root_positions.iter().copied().collect(); let candidates = RevWalkRevset::new(move |index| { Box::new( - index - .walk_revs(&head_positions, &[]) - .take_until_roots(&root_positions), + RevWalkBuilder::new(index) + .wanted_heads(head_positions.iter().copied()) + .ancestors_until_roots(&root_positions), ) }); let predicate = as_pure_predicate_fn(move |_index, entry| { @@ -820,9 +831,9 @@ impl<'index> EvaluationContext<'index> { predicate, })) } else if generation_from_roots == &GENERATION_RANGE_FULL { - let mut positions = index - .walk_revs(&head_positions, &[]) - .descendants(&root_positions) + let mut positions = RevWalkBuilder::new(index) + .wanted_heads(head_positions) + .descendants(root_positions) .map(|entry| entry.position()) .collect_vec(); positions.reverse(); @@ -831,10 +842,10 @@ impl<'index> EvaluationContext<'index> { // For small generation range, it might be better to build a reachable map // with generation bit set, which can be calculated incrementally from roots: // reachable[pos] = (reachable[parent_pos] | ...) << 1 - let mut positions = index - .walk_revs(&head_positions, &[]) + let mut positions = RevWalkBuilder::new(index) + .wanted_heads(head_positions) .descendants_filtered_by_generation( - &root_positions, + root_positions, to_u32_generation_range(generation_from_roots)?, ) .map(|entry| entry.position()) @@ -856,9 +867,9 @@ impl<'index> EvaluationContext<'index> { .iter() .map(|entry| entry.position()) .collect_vec(); - let filled = index - .walk_revs(&candidate_positions, &[]) - .descendants(&candidate_positions) + let filled = RevWalkBuilder::new(index) + .wanted_heads(candidate_positions.iter().copied()) + .descendants(candidate_positions) .collect_positions_set(); let mut positions = vec![]; for candidate in candidate_entries {