From c98b0d76af8e262de6cac63087eee5d214a8f4fc Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 6 Jan 2024 16:54:31 -0800 Subject: [PATCH] index: move Revset::change_id_index() to Index We current have `Revset::change_id_index()` for creating a `ChangeIdIndex` for a given revset. I think it will be hard to make it performant for general revsets, especially in very large repos and with custom index implementations, like the one we have at Google. If we instead restrict it to including all ancestors of a set of heads, I think it will be much easier to implement. We only use `Revset::change_id_index()` with revsets including all visible commits today, so we won't lose any current functionality by making it more restricted. --- lib/src/default_index/composite.rs | 18 ++++++++++++++++-- lib/src/default_index/mutable.rs | 9 ++++++++- lib/src/default_index/readonly.rs | 8 ++++++++ lib/src/default_index/revset_engine.rs | 15 --------------- lib/src/index.rs | 5 +++++ lib/src/repo.rs | 11 ++++++----- lib/src/revset.rs | 3 --- lib/tests/test_index.rs | 9 +++------ 8 files changed, 46 insertions(+), 32 deletions(-) diff --git a/lib/src/default_index/composite.rs b/lib/src/default_index/composite.rs index 1d9c16288..5db8121c4 100644 --- a/lib/src/default_index/composite.rs +++ b/lib/src/default_index/composite.rs @@ -296,6 +296,13 @@ impl<'a> CompositeIndex<'a> { candidate_positions } + pub(super) fn change_id_index( + &self, + heads: &mut dyn Iterator, + ) -> Box { + Box::new(ChangeIdIndexImpl::new(*self, heads)) + } + pub(super) fn evaluate_revset( &self, expression: &ResolvedExpression, @@ -382,6 +389,13 @@ impl Index for CompositeIndex<'_> { ids } + fn change_id_index( + &self, + heads: &mut dyn Iterator, + ) -> Box { + CompositeIndex::change_id_index(self, heads) + } + fn evaluate_revset<'index>( &'index self, expression: &ResolvedExpression, @@ -392,8 +406,8 @@ impl Index for CompositeIndex<'_> { } pub(super) struct ChangeIdIndexImpl { - pub index: I, - pub pos_by_change: IdIndex, + index: I, + pos_by_change: IdIndex, } impl ChangeIdIndexImpl { diff --git a/lib/src/default_index/mutable.rs b/lib/src/default_index/mutable.rs index ba99b2adb..0e2f6002d 100644 --- a/lib/src/default_index/mutable.rs +++ b/lib/src/default_index/mutable.rs @@ -35,7 +35,7 @@ use super::readonly::{DefaultReadonlyIndex, ReadonlyIndexSegment}; use crate::backend::{ChangeId, CommitId}; use crate::commit::Commit; use crate::file_util::persist_content_addressed_temp_file; -use crate::index::{Index, MutableIndex, ReadonlyIndex}; +use crate::index::{ChangeIdIndex, Index, MutableIndex, ReadonlyIndex}; use crate::object_id::{HexPrefix, ObjectId, PrefixResolution}; use crate::revset::{ResolvedExpression, Revset, RevsetEvaluationError}; use crate::store::Store; @@ -442,6 +442,13 @@ impl Index for DefaultMutableIndex { self.as_composite().topo_order(input) } + fn change_id_index( + &self, + heads: &mut dyn Iterator, + ) -> Box { + self.as_composite().change_id_index(heads) + } + fn evaluate_revset<'index>( &'index self, expression: &ResolvedExpression, diff --git a/lib/src/default_index/readonly.rs b/lib/src/default_index/readonly.rs index b95d6e320..e305adb6f 100644 --- a/lib/src/default_index/readonly.rs +++ b/lib/src/default_index/readonly.rs @@ -509,6 +509,14 @@ impl Index for DefaultReadonlyIndex { self.as_composite().topo_order(input) } + // TODO: Create a persistent lookup from change id to commit ids. + fn change_id_index( + &self, + heads: &mut dyn Iterator, + ) -> Box { + self.as_composite().change_id_index(heads) + } + fn evaluate_revset<'index>( &'index self, expression: &ResolvedExpression, diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs index a2e605848..c2cf31a9e 100644 --- a/lib/src/default_index/revset_engine.rs +++ b/lib/src/default_index/revset_engine.rs @@ -23,12 +23,9 @@ use std::sync::Arc; use itertools::Itertools; -use super::composite::ChangeIdIndexImpl; use super::revset_graph_iterator::RevsetGraphIterator; use crate::backend::{ChangeId, CommitId, MillisSinceEpoch}; use crate::default_index::{AsCompositeIndex, CompositeIndex, IndexEntry, IndexPosition}; -use crate::id_prefix::IdIndex; -use crate::index::ChangeIdIndex; use crate::matchers::{EverythingMatcher, Matcher, PrefixMatcher, Visit}; use crate::repo_path::RepoPath; use crate::revset::{ @@ -133,18 +130,6 @@ where Box::new(self.iter_graph_impl()) } - fn change_id_index(&self) -> Box { - // TODO: Create a persistent lookup from change id to commit ids. - let mut pos_by_change = IdIndex::builder(); - for entry in self.entries() { - pos_by_change.insert(&entry.change_id(), entry.position()); - } - Box::new(ChangeIdIndexImpl { - index: self.index.clone(), - pos_by_change: pos_by_change.build(), - }) - } - fn is_empty(&self) -> bool { self.entries().next().is_none() } diff --git a/lib/src/index.rs b/lib/src/index.rs index bb554cc16..8e0014c64 100644 --- a/lib/src/index.rs +++ b/lib/src/index.rs @@ -72,6 +72,11 @@ pub trait Index: Send + Sync { /// Parents before children fn topo_order(&self, input: &mut dyn Iterator) -> Vec; + fn change_id_index( + &self, + heads: &mut dyn Iterator, + ) -> Box; + fn evaluate_revset<'index>( &'index self, expression: &ResolvedExpression, diff --git a/lib/src/repo.rs b/lib/src/repo.rs index de1ae748d..39ed1485c 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -49,7 +49,6 @@ use crate::operation::Operation; use crate::refs::{ diff_named_ref_targets, diff_named_remote_refs, merge_ref_targets, merge_remote_refs, }; -use crate::revset::RevsetExpression; use crate::rewrite::{DescendantRebaser, RebaseOptions}; use crate::settings::{RepoSettings, UserSettings}; use crate::signing::{SignInitError, Signer}; @@ -1402,14 +1401,16 @@ impl Repo for MutableRepo { } fn resolve_change_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution> { - let revset = RevsetExpression::all().evaluate_programmatic(self).unwrap(); - let change_id_index = revset.change_id_index(); + let change_id_index = self + .index() + .change_id_index(&mut self.view().heads().iter()); change_id_index.resolve_prefix(prefix) } fn shortest_unique_change_id_prefix_len(&self, target_id: &ChangeId) -> usize { - let revset = RevsetExpression::all().evaluate_programmatic(self).unwrap(); - let change_id_index = revset.change_id_index(); + let change_id_index = self + .index() + .change_id_index(&mut self.view().heads().iter()); change_id_index.shortest_unique_prefix_len(target_id) } } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index f517fc9d1..35cec1e9d 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -35,7 +35,6 @@ use crate::backend::{BackendError, BackendResult, ChangeId, CommitId}; use crate::commit::Commit; use crate::git; use crate::hex_util::to_forward_hex; -use crate::index::ChangeIdIndex; use crate::object_id::{HexPrefix, PrefixResolution}; use crate::op_store::WorkspaceId; use crate::repo::Repo; @@ -2414,8 +2413,6 @@ pub trait Revset<'index>: fmt::Debug { fn iter_graph(&self) -> Box)> + '_>; - fn change_id_index(&self) -> Box; - fn is_empty(&self) -> bool; fn count(&self) -> usize; diff --git a/lib/tests/test_index.rs b/lib/tests/test_index.rs index c6f7ef696..60c12bcda 100644 --- a/lib/tests/test_index.rs +++ b/lib/tests/test_index.rs @@ -26,7 +26,6 @@ use jj_lib::default_index::{ use jj_lib::index::Index as _; use jj_lib::object_id::{HexPrefix, ObjectId as _, PrefixResolution}; use jj_lib::repo::{MutableRepo, ReadonlyRepo, Repo}; -use jj_lib::revset::RevsetExpression; use jj_lib::settings::UserSettings; use testutils::test_backend::TestBackend; use testutils::{ @@ -674,11 +673,9 @@ fn test_change_id_index() { let commit_5 = commit_with_change_id("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"); let index_for_heads = |commits: &[&Commit]| { - RevsetExpression::commits(commits.iter().map(|commit| commit.id().clone()).collect()) - .ancestors() - .evaluate_programmatic(tx.repo()) - .unwrap() - .change_id_index() + tx.repo() + .index() + .change_id_index(&mut commits.iter().map(|commit| commit.id())) }; let change_id_index = index_for_heads(&[&commit_1, &commit_2, &commit_3, &commit_4, &commit_5]); let prefix_len =