Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Not performance improvements :) #4989

Merged
merged 3 commits into from
Nov 28, 2024
Merged

Not performance improvements :) #4989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e6b513
merged_tree: avoid two unnecessary clones
martinvonz Nov 27, 2024
ad6b5cf
store: make `get_tree()` functions take owned repo path
martinvonz Nov 27, 2024
3ca2ef2
test_merged_tree: avoid a temporary lifetime extension
martinvonz Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/src/commands/debug/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub fn cmd_debug_tree(
RepoPathBuf::root()
};
let store = workspace_command.repo().store();
let tree = store.get_tree(&dir, &tree_id)?;
let tree = store.get_tree(dir, &tree_id)?;
MergedTree::resolved(tree)
} else {
let commit = workspace_command
Expand Down
2 changes: 1 addition & 1 deletion lib/src/merge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ where
if let Some(tree_id_merge) = tree_id_merge {
let get_tree = |id: &Option<&TreeId>| -> BackendResult<Tree> {
if let Some(id) = id {
store.get_tree(dir, id)
store.get_tree(dir.to_owned(), id)
} else {
Ok(Tree::empty(store.clone(), dir.to_owned()))
}
Expand Down
18 changes: 9 additions & 9 deletions lib/src/merged_tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ impl MergedTree {
.into_iter()
.map(|builder| {
let tree_id = builder.write_tree()?;
store.get_tree(RepoPath::root(), &tree_id)
store.get_tree(RepoPathBuf::root(), &tree_id)
})
.try_collect()?;
Ok(MergedTree {
Expand Down Expand Up @@ -199,19 +199,19 @@ impl MergedTree {
Ok(Some(TreeValue::Tree(sub_tree_id))) => {
let subdir = self.dir().join(name);
Ok(Some(MergedTree::resolved(
self.store().get_tree(&subdir, sub_tree_id)?,
self.store().get_tree(subdir, sub_tree_id)?,
)))
}
Ok(_) => Ok(None),
Err(merge) => {
let trees = merge.try_map(|value| match value {
Some(TreeValue::Tree(sub_tree_id)) => {
let subdir = self.dir().join(name);
self.store().get_tree(&subdir, sub_tree_id)
self.store().get_tree(subdir, sub_tree_id)
}
_ => {
let subdir = self.dir().join(name);
Ok(Tree::empty(self.store().clone(), subdir.clone()))
Ok(Tree::empty(self.store().clone(), subdir))
}
})?;
Ok(Some(MergedTree { trees }))
Expand Down Expand Up @@ -939,12 +939,12 @@ impl<'matcher> TreeDiffStreamImpl<'matcher> {

async fn single_tree(
store: &Arc<Store>,
dir: &RepoPath,
dir: RepoPathBuf,
value: Option<&TreeValue>,
) -> BackendResult<Tree> {
match value {
Some(TreeValue::Tree(tree_id)) => store.get_tree_async(dir, tree_id).await,
_ => Ok(Tree::empty(store.clone(), dir.to_owned())),
_ => Ok(Tree::empty(store.clone(), dir.clone())),
}
}

Expand All @@ -956,12 +956,12 @@ impl<'matcher> TreeDiffStreamImpl<'matcher> {
) -> BackendResult<MergedTree> {
let trees = if values.is_tree() {
let builder: MergeBuilder<Tree> = futures::stream::iter(values.iter())
.then(|value| Self::single_tree(&store, &dir, value.as_ref()))
.then(|value| Self::single_tree(&store, dir.clone(), value.as_ref()))
.try_collect()
.await?;
builder.build()
} else {
Merge::resolved(Tree::empty(store, dir.clone()))
Merge::resolved(Tree::empty(store, dir))
};
Ok(MergedTree { trees })
}
Expand Down Expand Up @@ -1147,7 +1147,7 @@ impl MergedTreeBuilder {
pub fn write_tree(self, store: &Arc<Store>) -> BackendResult<MergedTreeId> {
let base_tree_ids = match self.base_tree_id.clone() {
MergedTreeId::Legacy(base_tree_id) => {
let legacy_base_tree = store.get_tree(RepoPath::root(), &base_tree_id)?;
let legacy_base_tree = store.get_tree(RepoPathBuf::root(), &base_tree_id)?;
let base_tree = MergedTree::from_legacy_tree(legacy_base_tree)?;
base_tree.id().to_merge()
}
Expand Down
12 changes: 6 additions & 6 deletions lib/src/store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,17 +170,17 @@ impl Store {
Ok(Commit::new(self.clone(), commit_id, data))
}

pub fn get_tree(self: &Arc<Self>, dir: &RepoPath, id: &TreeId) -> BackendResult<Tree> {
pub fn get_tree(self: &Arc<Self>, dir: RepoPathBuf, id: &TreeId) -> BackendResult<Tree> {
self.get_tree_async(dir, id).block_on()
}

pub async fn get_tree_async(
self: &Arc<Self>,
dir: &RepoPath,
dir: RepoPathBuf,
id: &TreeId,
) -> BackendResult<Tree> {
let data = self.get_backend_tree(dir, id).await?;
Ok(Tree::new(self.clone(), dir.to_owned(), id.clone(), data))
let data = self.get_backend_tree(&dir, id).await?;
Ok(Tree::new(self.clone(), dir, id.clone(), data))
}

async fn get_backend_tree(
Expand All @@ -205,11 +205,11 @@ impl Store {
pub fn get_root_tree(self: &Arc<Self>, id: &MergedTreeId) -> BackendResult<MergedTree> {
match &id {
MergedTreeId::Legacy(id) => {
let tree = self.get_tree(RepoPath::root(), id)?;
let tree = self.get_tree(RepoPathBuf::root(), id)?;
MergedTree::from_legacy_tree(tree)
}
MergedTreeId::Merge(ids) => {
let trees = ids.try_map(|id| self.get_tree(RepoPath::root(), id))?;
let trees = ids.try_map(|id| self.get_tree(RepoPathBuf::root(), id))?;
Ok(MergedTree::new(trees))
}
}
Expand Down
12 changes: 6 additions & 6 deletions lib/src/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ impl Tree {
match sub_tree {
TreeValue::Tree(sub_tree_id) => {
let subdir = self.dir.join(name);
let sub_tree = self.store.get_tree(&subdir, sub_tree_id)?;
let sub_tree = self.store.get_tree(subdir, sub_tree_id)?;
Ok(Some(sub_tree))
}
_ => Ok(None),
Expand All @@ -165,7 +165,7 @@ impl Tree {
}
}

fn known_sub_tree(&self, subdir: &RepoPath, id: &TreeId) -> Tree {
fn known_sub_tree(&self, subdir: RepoPathBuf, id: &TreeId) -> Tree {
self.store.get_tree(subdir, id).unwrap()
}

Expand Down Expand Up @@ -251,7 +251,7 @@ impl Iterator for TreeEntriesIterator<'_> {
if self.matcher.visit(&path).is_nothing() {
continue;
}
let subtree = top.tree.known_sub_tree(&path, &id);
let subtree = top.tree.known_sub_tree(path, &id);
self.stack.push(TreeEntriesDirItem::from(subtree));
}
value => {
Expand Down Expand Up @@ -369,9 +369,9 @@ fn merge_tree_value(
Ok(match (base_tree_id, side1_tree_id, side2_tree_id) {
(Some(base_id), Some(side1_id), Some(side2_id)) => {
let subdir = dir.join(basename);
let base_tree = store.get_tree(&subdir, base_id)?;
let side1_tree = store.get_tree(&subdir, side1_id)?;
let side2_tree = store.get_tree(&subdir, side2_id)?;
let base_tree = store.get_tree(subdir.clone(), base_id)?;
let side1_tree = store.get_tree(subdir.clone(), side1_id)?;
let side2_tree = store.get_tree(subdir, side2_id)?;
let merged_tree = merge_trees(&side1_tree, &base_tree, &side2_tree)?;
if merged_tree.id() == empty_tree_id {
None
Expand Down
2 changes: 1 addition & 1 deletion lib/src/tree_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ impl TreeBuilder {
let store = &self.store;
let mut tree_cache = {
let dir = RepoPathBuf::root();
let tree = store.get_tree(&dir, &self.base_tree_id)?;
let tree = store.get_tree(dir.clone(), &self.base_tree_id)?;
BTreeMap::from([(dir, tree)])
};

Expand Down
4 changes: 2 additions & 2 deletions lib/tests/test_local_working_copy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ fn test_tree_builder_file_directory_transition() {
let mut ws = test_workspace.workspace;
let workspace_root = ws.workspace_root().to_owned();
let mut check_out_tree = |tree_id: &TreeId| {
let tree = repo.store().get_tree(RepoPath::root(), tree_id).unwrap();
let tree = repo.store().get_tree(RepoPathBuf::root(), tree_id).unwrap();
let commit = commit_with_tree(repo.store(), MergedTreeId::Legacy(tree.id().clone()));
ws.check_out(
repo.op_id().clone(),
Expand Down Expand Up @@ -1140,7 +1140,7 @@ fn test_gitignores_ignored_directory_already_tracked() {
}
}
let id = tree_builder.write_tree().unwrap();
MergedTree::resolved(store.get_tree(RepoPath::root(), &id).unwrap())
MergedTree::resolved(store.get_tree(RepoPathBuf::root(), &id).unwrap())
};

let gitignore_path = RepoPath::from_internal_string(".gitignore");
Expand Down
17 changes: 9 additions & 8 deletions lib/tests/test_merge_trees.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use itertools::Itertools;
use jj_lib::backend::TreeValue;
use jj_lib::repo::Repo;
use jj_lib::repo_path::RepoPath;
use jj_lib::repo_path::RepoPathBuf;
use jj_lib::repo_path::RepoPathComponent;
use jj_lib::rewrite::rebase_commit;
use jj_lib::tree::merge_trees;
Expand Down Expand Up @@ -65,7 +66,7 @@ fn test_same_type() {
}
}
let tree_id = tree_builder.write_tree().unwrap();
store.get_tree(RepoPath::root(), &tree_id).unwrap()
store.get_tree(RepoPathBuf::root(), &tree_id).unwrap()
};

let base_tree = write_tree(0);
Expand Down Expand Up @@ -207,7 +208,7 @@ fn test_executable() {
}
}
let tree_id = tree_builder.write_tree().unwrap();
store.get_tree(RepoPath::root(), &tree_id).unwrap()
store.get_tree(RepoPathBuf::root(), &tree_id).unwrap()
};

fn contents_in_tree<'a>(files: &[&'a str], index: usize) -> Vec<(&'a str, bool)> {
Expand Down Expand Up @@ -255,7 +256,7 @@ fn test_subtrees() {
);
}
let tree_id = tree_builder.write_tree().unwrap();
store.get_tree(RepoPath::root(), &tree_id).unwrap()
store.get_tree(RepoPathBuf::root(), &tree_id).unwrap()
};

let base_tree = write_tree(vec!["f1", "d1/f1", "d1/d1/f1", "d1/d1/d1/f1"]);
Expand Down Expand Up @@ -309,7 +310,7 @@ fn test_subtree_becomes_empty() {
);
}
let tree_id = tree_builder.write_tree().unwrap();
store.get_tree(RepoPath::root(), &tree_id).unwrap()
store.get_tree(RepoPathBuf::root(), &tree_id).unwrap()
};

let base_tree = write_tree(vec!["f1", "d1/f1", "d1/d1/d1/f1", "d1/d1/d1/f2"]);
Expand Down Expand Up @@ -338,7 +339,7 @@ fn test_subtree_one_missing() {
);
}
let tree_id = tree_builder.write_tree().unwrap();
store.get_tree(RepoPath::root(), &tree_id).unwrap()
store.get_tree(RepoPathBuf::root(), &tree_id).unwrap()
};

let tree1 = write_tree(vec![]);
Expand Down Expand Up @@ -405,11 +406,11 @@ fn test_types() {
"contents",
);
let base_tree_id = base_tree_builder.write_tree().unwrap();
let base_tree = store.get_tree(RepoPath::root(), &base_tree_id).unwrap();
let base_tree = store.get_tree(RepoPathBuf::root(), &base_tree_id).unwrap();
let side1_tree_id = side1_tree_builder.write_tree().unwrap();
let side1_tree = store.get_tree(RepoPath::root(), &side1_tree_id).unwrap();
let side1_tree = store.get_tree(RepoPathBuf::root(), &side1_tree_id).unwrap();
let side2_tree_id = side2_tree_builder.write_tree().unwrap();
let side2_tree = store.get_tree(RepoPath::root(), &side2_tree_id).unwrap();
let side2_tree = store.get_tree(RepoPathBuf::root(), &side2_tree_id).unwrap();

// Created the merged tree
let merged_tree = merge_trees(&side1_tree, &base_tree, &side2_tree).unwrap();
Expand Down
10 changes: 5 additions & 5 deletions lib/tests/test_merged_tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,14 @@ fn test_from_legacy_tree() {

// dir1: directory without conflicts
let dir1_basename = RepoPathComponent::new("dir1");
let dir1_filename = &RepoPath::root()
let dir1_filename = RepoPath::root()
.join(dir1_basename)
.join(RepoPathComponent::new("file"));
let dir1_filename_id = write_file(store.as_ref(), dir1_filename, "file5_v2");
tree_builder.set(dir1_filename.to_owned(), file_value(&dir1_filename_id));
let dir1_filename_id = write_file(store.as_ref(), &dir1_filename, "file5_v2");
tree_builder.set(dir1_filename.clone(), file_value(&dir1_filename_id));

let tree_id = tree_builder.write_tree().unwrap();
let tree = store.get_tree(RepoPath::root(), &tree_id).unwrap();
let tree = store.get_tree(RepoPathBuf::root(), &tree_id).unwrap();

let merged_tree = MergedTree::from_legacy_tree(tree.clone()).unwrap();
assert_eq!(
Expand Down Expand Up @@ -269,7 +269,7 @@ fn test_from_legacy_tree() {
tree_builder.set_or_remove(file3_path.to_owned(), file3_conflict);
tree_builder.set_or_remove(file4_path.to_owned(), file4_conflict);
tree_builder.set_or_remove(
dir1_filename.to_owned(),
dir1_filename.clone(),
Merge::normal(file_value(&dir1_filename_id)),
);
let recreated_merged_id = tree_builder.write_tree(store).unwrap();
Expand Down
2 changes: 1 addition & 1 deletion lib/testutils/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ pub fn create_single_tree(repo: &Arc<ReadonlyRepo>, path_contents: &[(&RepoPath,
write_normal_file(&mut tree_builder, path, contents);
}
let id = tree_builder.write_tree().unwrap();
store.get_tree(RepoPath::root(), &id).unwrap()
store.get_tree(RepoPathBuf::root(), &id).unwrap()
}

pub fn create_tree(repo: &Arc<ReadonlyRepo>, path_contents: &[(&RepoPath, &str)]) -> MergedTree {
Expand Down