ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

merged_tree: make TreeEntriesIterator not self-referential

Browse source 
While importing the `ouroboros` crate and the `aliasable` crate it
depends on, the "unsafe Rust reviewer" expressed some concern that
they contain a lot of unsafe code that's hard to review. We can avoid
the unsafe code altogether by making `TreeEntriesIterator` not
self-refential. Instead, we can collect the matching entries in an
individual tree up front. It does have some performance cost:


```
❯ hyperfine --warmup 3 --runs 30 \
      '/tmp/jj-before --ignore-working-copy files -r v6.0' \
      '/tmp/jj-after --ignore-working-copy files -r v6.0'
Benchmark 1: /tmp/jj-before --ignore-working-copy files -r v6.0
  Time (mean ± σ):     461.4 ms ±  14.3 ms    [User: 232.1 ms, System: 229.4 ms]
  Range (min … max):   443.4 ms … 496.3 ms    30 runs

Benchmark 2: /tmp/jj-after --ignore-working-copy files -r v6.0
  Time (mean ± σ):     482.0 ms ±  14.3 ms    [User: 257.2 ms, System: 224.9 ms]
  Range (min … max):   461.8 ms … 513.3 ms    30 runs

Summary
  '/tmp/jj-before --ignore-working-copy files -r v6.0' ran
    1.04 ± 0.04 times faster than '/tmp/jj-after --ignore-working-copy files -r v6.0'
```

I think that's acceptable.
This commit is contained in:
Martin von Zweigbergk 2023-11-13 17:13:42 -08:00 committed by Martin von Zweigbergk
parent c2d5e3d343
commit 61d87fe296
1 changed files with 29 additions and 20 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

49
lib/src/merged_tree.rs
View file

@ -575,25 +575,37 @@ pub struct TreeEntriesIterator<'matcher> {
matcher: &'matcher dyn Matcher,
}
#[ouroboros::self_referencing]
struct TreeEntriesDirItem {
tree: MergedTree,
#[borrows(tree)]
#[not_covariant]
entry_iterator: TreeEntriesNonRecursiveIterator<'this>,
entries: Vec<(RepoPath, MergedTreeValue)>,
}
impl From<MergedTree> for TreeEntriesDirItem {
fn from(tree: MergedTree) -> Self {
TreeEntriesDirItem::new(tree, |tree| tree.entries_non_recursive())
impl TreeEntriesDirItem {
fn new(tree: MergedTree, matcher: &dyn Matcher) -> Self {
let mut entries = vec![];
let dir = tree.dir();
for (name, value) in tree.entries_non_recursive() {
let path = dir.join(name);
let value = value.to_merge();
if value.is_tree() {
// TODO: Handle the other cases (specific files and trees)
if matcher.visit(&path).is_nothing() {
continue;
}
} else if !matcher.matches(&path) {
continue;
}
entries.push((path, value));
}
entries.reverse();
TreeEntriesDirItem { tree, entries }
}
}
impl<'matcher> TreeEntriesIterator<'matcher> {
fn new(tree: MergedTree, matcher: &'matcher dyn Matcher) -> Self {
// TODO: Restrict walk according to Matcher::visit()
Self {
stack: vec![TreeEntriesDirItem::from(tree)],
stack: vec![TreeEntriesDirItem::new(tree, matcher)],
matcher,
}
}
@ -604,19 +616,16 @@ impl Iterator for TreeEntriesIterator<'_> {
fn next(&mut self) -> Option<Self::Item> {
while let Some(top) = self.stack.last_mut() {
if let (tree, Some((name, value))) = top.with_mut(|x| (x.tree, x.entry_iterator.next()))
{
let path = tree.dir().join(name);
let value = value.to_merge();
if let Some((path, value)) = top.entries.pop() {
if value.is_tree() {
// TODO: Handle the other cases (specific files and trees)
if self.matcher.visit(&path).is_nothing() {
continue;
}
let tree_merge = value.to_tree_merge(tree.store(), &path).unwrap().unwrap();
let tree_merge = value
.to_tree_merge(top.tree.store(), &path)
.unwrap()
.unwrap();
let merged_tree = MergedTree::Merge(tree_merge);
self.stack.push(TreeEntriesDirItem::from(merged_tree));
} else if self.matcher.matches(&path) {
self.stack
.push(TreeEntriesDirItem::new(merged_tree, self.matcher));
} else {
return Some((path, value));
}
} else {