From f0619c07acd64b29ee9ae881b1a7df4eaaa1553f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 15 Mar 2021 04:15:35 -0700 Subject: [PATCH] MutableEvolution: make MutableRepo responsible for lazy calculation This patch continues the work from the previous pathc. From this patch, we no longer calculate the evolution state just because a transaction starts. We still unnecessarily calculate it when adding a commit within the transaction, however. I'll fix that next. --- lib/src/evolution.rs | 35 +++++++++---------- lib/src/repo.rs | 67 +++++++++++++++++++++++-------------- lib/tests/test_evolution.rs | 20 +++++------ src/commands.rs | 2 +- 4 files changed, 68 insertions(+), 56 deletions(-) diff --git a/lib/src/evolution.rs b/lib/src/evolution.rs index c9122dd55..d6008ef32 100644 --- a/lib/src/evolution.rs +++ b/lib/src/evolution.rs @@ -315,12 +315,12 @@ impl State { } } -pub enum EvolutionRef<'a, 'm: 'a, 'r: 'm> { +pub enum EvolutionRef<'a> { Readonly(Arc), - Mutable(&'a MutableEvolution<'m, 'r>), + Mutable(&'a MutableEvolution), } -impl EvolutionRef<'_, '_, '_> { +impl EvolutionRef<'_> { pub fn successors(&self, commit_id: &CommitId) -> HashSet { match self { EvolutionRef::Readonly(evolution) => evolution.successors(commit_id), @@ -367,7 +367,7 @@ impl EvolutionRef<'_, '_, '_> { pub fn new_parent(&self, store: &StoreWrapper, old_parent_id: &CommitId) -> HashSet { match self { EvolutionRef::Readonly(evolution) => evolution.new_parent(store, old_parent_id), - EvolutionRef::Mutable(evolution) => evolution.new_parent(old_parent_id), + EvolutionRef::Mutable(evolution) => evolution.new_parent(store, old_parent_id), } } } @@ -400,12 +400,8 @@ impl ReadonlyEvolution { } } - pub fn start_modification<'r: 'm, 'm>( - &self, - repo: &'m MutableRepo<'r>, - ) -> MutableEvolution<'r, 'm> { + pub fn start_modification(&self) -> MutableEvolution { MutableEvolution { - repo, state: self.state.clone(), } } @@ -431,12 +427,17 @@ impl ReadonlyEvolution { } } -pub struct MutableEvolution<'r: 'm, 'm> { - repo: &'m MutableRepo<'r>, +pub struct MutableEvolution { state: State, } -impl MutableEvolution<'_, '_> { +impl MutableEvolution { + pub fn new(repo: &MutableRepo) -> MutableEvolution { + MutableEvolution { + state: State::calculate(repo.as_repo_ref()), + } + } + pub fn successors(&self, commit_id: &CommitId) -> HashSet { self.state.successors(commit_id) } @@ -453,17 +454,13 @@ impl MutableEvolution<'_, '_> { self.state.is_divergent(change_id) } - pub fn new_parent(&self, old_parent_id: &CommitId) -> HashSet { - self.state.new_parent(self.repo.store(), old_parent_id) + pub fn new_parent(&self, store: &StoreWrapper, old_parent_id: &CommitId) -> HashSet { + self.state.new_parent(store, old_parent_id) } pub fn add_commit(&mut self, commit: &Commit) { self.state.add_commit(commit); } - - pub fn invalidate(&mut self) { - self.state = State::calculate(self.repo.as_repo_ref()); - } } pub fn evolve( @@ -527,7 +524,7 @@ pub fn evolve( let mut ambiguous_new_parents = false; let evolution = tx.mut_repo().evolution(); for old_parent in &old_parents { - let new_parent_candidates = evolution.new_parent(old_parent.id()); + let new_parent_candidates = evolution.new_parent(&store, old_parent.id()); if new_parent_candidates.len() > 1 { ambiguous_new_parents = true; break; diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 201087bca..b4bec4fb6 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -97,7 +97,7 @@ impl<'a, 'r> RepoRef<'a, 'r> { } } - pub fn evolution(&self) -> EvolutionRef<'a, 'a, 'r> { + pub fn evolution(&self) -> EvolutionRef { match self { RepoRef::Readonly(repo) => EvolutionRef::Readonly(repo.evolution()), RepoRef::Mutable(repo) => EvolutionRef::Mutable(repo.evolution()), @@ -346,7 +346,8 @@ impl ReadonlyRepo { } pub fn start_transaction(&self, description: &str) -> Transaction { - let mut_repo = MutableRepo::new(self, self.index(), &self.view, self.evolution()); + let locked_evolution = self.evolution.lock().unwrap(); + let mut_repo = MutableRepo::new(self, self.index(), &self.view, locked_evolution.as_ref()); Transaction::new(mut_repo, description) } @@ -482,7 +483,7 @@ pub struct MutableRepo<'r> { repo: &'r ReadonlyRepo, index: MutableIndex, view: MutableView, - evolution: Option>, + evolution: Mutex>, } impl<'r> MutableRepo<'r> { @@ -490,24 +491,17 @@ impl<'r> MutableRepo<'r> { repo: &'r ReadonlyRepo, index: Arc, view: &ReadonlyView, - evolution: Arc, + evolution: Option<&Arc>, ) -> Arc> { let mut_view = view.start_modification(); let mut_index = MutableIndex::incremental(index); - let mut mut_repo = Arc::new(MutableRepo { + let mut_evolution = evolution.map(|evolution| evolution.start_modification()); + Arc::new(MutableRepo { repo, index: mut_index, view: mut_view, - evolution: None, - }); - let repo_ref: &MutableRepo = mut_repo.as_ref(); - let static_lifetime_repo: &'static MutableRepo = unsafe { std::mem::transmute(repo_ref) }; - let mut_evolution: MutableEvolution<'_, '_> = - evolution.start_modification(static_lifetime_repo); - let static_lifetime_mut_evolution: MutableEvolution<'static, 'static> = - unsafe { std::mem::transmute(mut_evolution) }; - Arc::get_mut(&mut mut_repo).unwrap().evolution = Some(static_lifetime_mut_evolution); - mut_repo + evolution: Mutex::new(mut_evolution), + }) } pub fn as_repo_ref(&self) -> RepoRef { @@ -538,10 +532,31 @@ impl<'r> MutableRepo<'r> { (self.index, self.view) } - pub fn evolution<'m>(&'m self) -> &MutableEvolution<'r, 'm> { - let evolution: &MutableEvolution<'static, 'static> = self.evolution.as_ref().unwrap(); - let evolution: &MutableEvolution<'r, 'm> = unsafe { std::mem::transmute(evolution) }; - evolution + pub fn evolution(&self) -> &MutableEvolution { + let mut locked_evolution = self.evolution.lock().unwrap(); + if locked_evolution.is_none() { + locked_evolution.replace(MutableEvolution::new(self)); + } + let evolution = locked_evolution.as_ref().unwrap(); + // Extend lifetime from lifetime of MutexGuard to lifetime of self. Safe because + // the value won't change again except for by invalidate_evolution(), which + // requires a mutable reference. + unsafe { std::mem::transmute(evolution) } + } + + pub fn evolution_mut(&mut self) -> &mut MutableEvolution { + let mut locked_evolution = self.evolution.lock().unwrap(); + if locked_evolution.is_none() { + locked_evolution.replace(MutableEvolution::new(self)); + } + // Extend lifetime from lifetime of MutexGuard to lifetime of self. Safe because + // the value won't change again except for by invalidate_evolution(), which + // requires a mutable reference. + unsafe { std::mem::transmute(locked_evolution.as_mut().unwrap()) } + } + + pub fn invalidate_evolution(&mut self) { + self.evolution.lock().unwrap().take(); } pub fn write_commit(&mut self, commit: store::Commit) -> Commit { @@ -610,7 +625,7 @@ impl<'r> MutableRepo<'r> { { self.index.add_commit(head); self.view.add_head(head); - self.evolution.as_mut().unwrap().add_commit(head); + self.evolution_mut().add_commit(head); } else { let missing_commits = topo_order_reverse( vec![head.clone()], @@ -627,23 +642,23 @@ impl<'r> MutableRepo<'r> { self.index.add_commit(missing_commit); } self.view.add_head(head); - self.evolution.as_mut().unwrap().invalidate(); + self.invalidate_evolution(); } } pub fn remove_head(&mut self, head: &Commit) { self.view.remove_head(head); - self.evolution.as_mut().unwrap().invalidate(); + self.invalidate_evolution(); } pub fn add_public_head(&mut self, head: &Commit) { self.view.add_public_head(head); - self.evolution.as_mut().unwrap().add_commit(head); + self.evolution_mut().add_commit(head); } pub fn remove_public_head(&mut self, head: &Commit) { self.view.remove_public_head(head); - self.evolution.as_mut().unwrap().invalidate(); + self.invalidate_evolution(); } pub fn insert_git_ref(&mut self, name: String, commit_id: CommitId) { @@ -656,7 +671,7 @@ impl<'r> MutableRepo<'r> { pub fn set_view(&mut self, data: op_store::View) { self.view.set_view(data); - self.evolution.as_mut().unwrap().invalidate(); + self.invalidate_evolution(); } pub fn merge(&mut self, base_repo: &ReadonlyRepo, other_repo: &ReadonlyRepo) { @@ -674,6 +689,6 @@ impl<'r> MutableRepo<'r> { ); self.view.set_view(merged_view); - self.evolution.as_mut().unwrap().invalidate(); + self.invalidate_evolution(); } } diff --git a/lib/tests/test_evolution.rs b/lib/tests/test_evolution.rs index a12aec0fb..24b54589c 100644 --- a/lib/tests/test_evolution.rs +++ b/lib/tests/test_evolution.rs @@ -164,7 +164,7 @@ fn test_new_parent_rewritten(use_git: bool) { .set_change_id(original.change_id().clone()) .write_to_transaction(&mut tx); assert_eq!( - tx.evolution().new_parent(original.id()), + tx.evolution().new_parent(tx.store(), original.id()), vec![rewritten.id().clone()].into_iter().collect() ); tx.discard(); @@ -184,7 +184,7 @@ fn test_new_parent_cherry_picked(use_git: bool) { .set_predecessors(vec![original.id().clone()]) .write_to_transaction(&mut tx); assert_eq!( - tx.evolution().new_parent(original.id()), + tx.evolution().new_parent(tx.store(), original.id()), vec![original.id().clone()].into_iter().collect() ); tx.discard(); @@ -208,7 +208,7 @@ fn test_new_parent_is_pruned(use_git: bool) { .set_change_id(original.change_id().clone()) .write_to_transaction(&mut tx); assert_eq!( - tx.evolution().new_parent(original.id()), + tx.evolution().new_parent(tx.store(), original.id()), vec![new_parent.id().clone()].into_iter().collect() ); tx.discard(); @@ -237,7 +237,7 @@ fn test_new_parent_divergent(use_git: bool) { .set_change_id(original.change_id().clone()) .write_to_transaction(&mut tx); assert_eq!( - tx.evolution().new_parent(original.id()), + tx.evolution().new_parent(tx.store(), original.id()), vec![ rewritten1.id().clone(), rewritten2.id().clone(), @@ -278,7 +278,7 @@ fn test_new_parent_divergent_one_not_pruned(use_git: bool) { .set_pruned(true) .write_to_transaction(&mut tx); assert_eq!( - tx.evolution().new_parent(original.id()), + tx.evolution().new_parent(tx.store(), original.id()), vec![ rewritten1.id().clone(), parent2.id().clone(), @@ -321,7 +321,7 @@ fn test_new_parent_divergent_all_pruned(use_git: bool) { .set_pruned(true) .write_to_transaction(&mut tx); assert_eq!( - tx.evolution().new_parent(original.id()), + tx.evolution().new_parent(tx.store(), original.id()), vec![ parent1.id().clone(), parent2.id().clone(), @@ -357,7 +357,7 @@ fn test_new_parent_split(use_git: bool) { .set_predecessors(vec![original.id().clone()]) .write_to_transaction(&mut tx); assert_eq!( - tx.evolution().new_parent(original.id()), + tx.evolution().new_parent(tx.store(), original.id()), vec![rewritten3.id().clone()].into_iter().collect() ); tx.discard(); @@ -391,7 +391,7 @@ fn test_new_parent_split_pruned_descendant(use_git: bool) { .set_predecessors(vec![original.id().clone()]) .write_to_transaction(&mut tx); assert_eq!( - tx.evolution().new_parent(original.id()), + tx.evolution().new_parent(tx.store(), original.id()), vec![rewritten2.id().clone()].into_iter().collect() ); tx.discard(); @@ -425,7 +425,7 @@ fn test_new_parent_split_forked(use_git: bool) { .set_predecessors(vec![original.id().clone()]) .write_to_transaction(&mut tx); assert_eq!( - tx.evolution().new_parent(original.id()), + tx.evolution().new_parent(tx.store(), original.id()), vec![rewritten2.id().clone(), rewritten3.id().clone()] .into_iter() .collect() @@ -461,7 +461,7 @@ fn test_new_parent_split_forked_pruned(use_git: bool) { .set_predecessors(vec![original.id().clone()]) .write_to_transaction(&mut tx); assert_eq!( - tx.evolution().new_parent(original.id()), + tx.evolution().new_parent(tx.store(), original.id()), vec![rewritten3.id().clone()].into_iter().collect() ); tx.discard(); diff --git a/src/commands.rs b/src/commands.rs index 6c2dc8c15..a8be6b996 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -257,7 +257,7 @@ fn update_working_copy( fn update_checkout_after_rewrite(ui: &mut Ui, tx: &mut Transaction) { // TODO: Perhaps this method should be in Transaction. - let new_checkout_candidates = tx.evolution().new_parent(tx.view().checkout()); + let new_checkout_candidates = tx.evolution().new_parent(tx.store(), tx.view().checkout()); if new_checkout_candidates.is_empty() { return; }