From 4c3d73ff3b32ef3865f77652b4b5e518dffda140 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 17 Mar 2021 15:58:35 -0700 Subject: [PATCH] evolution: walk orphans using index This actually seems to make it slightly slower, but it fixes an important bug (we used to evolve only one topological branch per `jj evolve` call). The slowdown seemed to be on the order of 5% when evolving 100 commits on git.git's "what's cooking" branch. --- lib/src/evolution.rs | 38 ++++++++---------------------- lib/src/index.rs | 46 +++++++++++++++++++++++++++++++++++++ lib/tests/test_evolution.rs | 37 +++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 28 deletions(-) diff --git a/lib/src/evolution.rs b/lib/src/evolution.rs index 40241cd70..cfd825a33 100644 --- a/lib/src/evolution.rs +++ b/lib/src/evolution.rs @@ -484,42 +484,23 @@ pub fn evolve( evolve_divergent_change(user_settings, &store, mut_repo, listener, &commits); } - // Dom't reuse the state from above, since the divergence-resolution may have + // Don't reuse the state from above, since the divergence-resolution may have // created new orphans, or resolved existing orphans. - let orphans: HashSet = mut_repo - .evolution() - .state - .orphan_commits + let orphans_topo_order: Vec<_> = mut_repo + .index() + .topo_order(mut_repo.evolution().state.orphan_commits.iter()) .iter() - .map(|id| store.get_commit(&id).unwrap()) + .map(|entry| entry.position()) .collect(); - let non_heads: HashSet = orphans.iter().flat_map(|commit| commit.parents()).collect(); - let orphan_heads: HashSet = orphans.difference(&non_heads).cloned().collect(); - let mut orphans_topo_order = vec![]; - for commit in bfs( - orphan_heads, - Box::new(|commit| commit.id().clone()), - Box::new(|commit| { - commit - .parents() - .iter() - .filter(|commit| orphans.contains(commit)) - .cloned() - .collect::>() - }), - ) { - orphans_topo_order.push(commit); - } - while !orphans_topo_order.is_empty() { - let orphan = orphans_topo_order.pop().unwrap(); - let old_parents = orphan.parents(); + for orphan_pos in orphans_topo_order { + let orphan_entry = mut_repo.index().entry_by_pos(orphan_pos); let mut new_parents = vec![]; let mut ambiguous_new_parents = false; let evolution = mut_repo.evolution(); - for old_parent in &old_parents { + for old_parent in orphan_entry.parents() { let new_parent_candidates = - evolution.new_parent(mut_repo.as_repo_ref(), old_parent.id()); + evolution.new_parent(mut_repo.as_repo_ref(), &old_parent.commit_id()); if new_parent_candidates.len() > 1 { ambiguous_new_parents = true; break; @@ -530,6 +511,7 @@ pub fn evolve( .unwrap(), ); } + let orphan = store.get_commit(&orphan_entry.commit_id()).unwrap(); if ambiguous_new_parents { listener.orphan_target_ambiguous(mut_repo, &orphan); } else { diff --git a/lib/src/index.rs b/lib/src/index.rs index 5380ece0f..f100661d0 100644 --- a/lib/src/index.rs +++ b/lib/src/index.rs @@ -654,6 +654,13 @@ impl MutableIndex { ) -> Vec { CompositeIndex(self).heads(candidates) } + + pub fn topo_order<'candidates>( + &self, + input: impl IntoIterator, + ) -> Vec { + CompositeIndex(self).topo_order(input) + } } trait IndexSegment { @@ -932,6 +939,19 @@ impl<'a> CompositeIndex<'a> { } candidate_positions } + + pub fn topo_order<'input>( + &self, + input: impl IntoIterator, + ) -> Vec> { + let mut entries_by_generation: Vec<_> = input + .into_iter() + .map(|id| IndexEntryByPosition(self.entry_by_id(id).unwrap())) + .collect(); + entries_by_generation.sort(); + let entries: Vec<_> = entries_by_generation.into_iter().map(|key| key.0).collect(); + entries + } } pub struct IndexLevelStats { @@ -949,6 +969,21 @@ pub struct IndexStats { pub levels: Vec, } +#[derive(Eq, PartialEq)] +struct IndexEntryByPosition<'a>(IndexEntry<'a>); + +impl Ord for IndexEntryByPosition<'_> { + fn cmp(&self, other: &Self) -> Ordering { + self.0.pos.cmp(&other.0.pos) + } +} + +impl PartialOrd for IndexEntryByPosition<'_> { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + #[derive(Eq, PartialEq)] struct IndexEntryByGeneration<'a>(IndexEntry<'a>); @@ -1292,6 +1327,10 @@ impl PartialEq for IndexEntry<'_> { impl Eq for IndexEntry<'_> {} impl<'a> IndexEntry<'a> { + pub fn position(&self) -> u32 { + self.pos + } + pub fn generation_number(&self) -> u32 { self.source.segment_generation_number(self.local_pos) } @@ -1449,6 +1488,13 @@ impl ReadonlyIndex { CompositeIndex(self).heads(candidates) } + pub fn topo_order<'candidates>( + &self, + input: impl IntoIterator, + ) -> Vec { + CompositeIndex(self).topo_order(input) + } + fn graph_entry(&self, local_pos: u32) -> CommitGraphEntry { let offset = (local_pos as usize) * self.commit_graph_entry_size; CommitGraphEntry { diff --git a/lib/tests/test_evolution.rs b/lib/tests/test_evolution.rs index ca3897b7f..075b4d69b 100644 --- a/lib/tests/test_evolution.rs +++ b/lib/tests/test_evolution.rs @@ -616,6 +616,43 @@ fn test_evolve_pruned_orphan(use_git: bool) { tx.discard(); } +#[test_case(false ; "local store")] +// #[test_case(true ; "git store")] +fn test_evolve_multiple_orphans(use_git: bool) { + let settings = testutils::user_settings(); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); + let root_commit = repo.store().root_commit(); + + let mut tx = repo.start_transaction("test"); + let mut_repo = tx.mut_repo(); + let initial = child_commit(&settings, &repo, &root_commit).write_to_repo(mut_repo); + let child = child_commit(&settings, &repo, &initial).write_to_repo(mut_repo); + let grandchild = child_commit(&settings, &repo, &child).write_to_repo(mut_repo); + let grandchild2 = child_commit(&settings, &repo, &child).write_to_repo(mut_repo); + + let rewritten = CommitBuilder::for_rewrite_from(&settings, repo.store(), &initial) + .set_description("rewritten".to_string()) + .write_to_repo(mut_repo); + + let mut listener = RecordingEvolveListener::default(); + evolve(&settings, mut_repo, &mut listener); + assert_eq!(listener.evolved_divergents.len(), 0); + assert_eq!(listener.evolved_orphans.len(), 3); + assert_eq!(&listener.evolved_orphans[0].0, &child); + assert_eq!(&listener.evolved_orphans[0].1.parents(), &vec![rewritten]); + assert_eq!(&listener.evolved_orphans[1].0, &grandchild); + assert_eq!( + &listener.evolved_orphans[1].1.parents(), + &vec![listener.evolved_orphans[0].1.clone()] + ); + assert_eq!(&listener.evolved_orphans[2].0, &grandchild2); + assert_eq!( + &listener.evolved_orphans[2].1.parents(), + &vec![listener.evolved_orphans[0].1.clone()] + ); + tx.discard(); +} + #[test_case(false ; "local store")] // #[test_case(true ; "git store")] fn test_evolve_divergent(use_git: bool) {