Skip to content

Commit

Permalink
revset: avoid merging whole parent trees by file()/diff_contains() query
Browse files Browse the repository at this point in the history
Perhaps, we should also cache merged trees, but this patch saves time until
we implement the bookkeeping. Even if we had a cache, it wouldn't be ideal to
calculate uncached merged trees during revset evaluation.

```
% hyperfine --sort command --warmup 3 --runs 10 -L bin jj-1,jj-2 \
  'target/release-with-debug/{bin} -R ~/mirrors/git --ignore-working-copy \
   log -r "::@ & file(root:builtin)" --no-graph -n50'
Benchmark 1: target/release-with-debug/jj-1 ..
  Time (mean ± σ):      3.512 s ±  0.014 s    [User: 3.391 s, System: 0.119 s]
  Range (min … max):    3.489 s …  3.528 s    10 runs

Benchmark 2: target/release-with-debug/jj-2 ..
  Time (mean ± σ):      1.351 s ±  0.010 s    [User: 1.275 s, System: 0.074 s]
  Range (min … max):    1.332 s …  1.366 s    10 runs

Relative speed comparison
        2.60 ±  0.02  target/release-with-debug/jj-1 ..
        1.00          target/release-with-debug/jj-2 ..
```
  • Loading branch information
yuja committed Aug 13, 2024
1 parent 13f0a2f commit a609580
Showing 1 changed file with 18 additions and 11 deletions.
29 changes: 18 additions & 11 deletions lib/src/default_index/revset_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use crate::conflicts::{materialize_tree_value, MaterializedTreeValue};
use crate::default_index::{AsCompositeIndex, CompositeIndex, IndexPosition};
use crate::graph::GraphEdge;
use crate::matchers::{Matcher, Visit};
use crate::merged_tree::TreeDiffEntry;
use crate::merged_tree::resolve_file_values;
use crate::repo_path::RepoPath;
use crate::revset::{
ResolvedExpression, ResolvedPredicateExpression, Revset, RevsetEvaluationError,
Expand Down Expand Up @@ -1143,19 +1143,22 @@ fn has_diff_from_parent(
return Ok(false);
}
}
let from_tree =
rewrite::merge_commit_trees_no_resolve_without_repo(store, &index, &parents)?.resolve()?;

// Conflict resolution is expensive, try that only for matched files.
let from_tree = rewrite::merge_commit_trees_no_resolve_without_repo(store, &index, &parents)?;
let to_tree = commit.tree()?;
let copy_records = Default::default();
let mut tree_diff = from_tree.diff_stream(&to_tree, matcher, &copy_records);
async {
match tree_diff.next().await {
Some(TreeDiffEntry { value: Ok(_), .. }) => Ok(true),
Some(TreeDiffEntry {
value: Err(err), ..
}) => Err(err),
None => Ok(false),
while let Some(entry) = tree_diff.next().await {
let (from_value, to_value) = entry.value?;
let from_value = resolve_file_values(store, &entry.source, from_value)?;
if from_value == to_value {
continue;
}
return Ok(true);
}
Ok(false)
}
.block_on()
}
Expand All @@ -1169,13 +1172,17 @@ fn matches_diff_from_parent(
) -> BackendResult<bool> {
let copy_records = Default::default(); // TODO handle copy tracking
let parents: Vec<_> = commit.parents().try_collect()?;
let from_tree =
rewrite::merge_commit_trees_no_resolve_without_repo(store, &index, &parents)?.resolve()?;
// Conflict resolution is expensive, try that only for matched files.
let from_tree = rewrite::merge_commit_trees_no_resolve_without_repo(store, &index, &parents)?;
let to_tree = commit.tree()?;
let mut tree_diff = from_tree.diff_stream(&to_tree, files_matcher, &copy_records);
async {
while let Some(entry) = tree_diff.next().await {
let (left_value, right_value) = entry.value?;
let left_value = resolve_file_values(store, &entry.source, left_value)?;
if left_value == right_value {
continue;
}
// Conflicts are compared in materialized form. Alternatively,
// conflict pairs can be compared one by one. #4062
let left_future = materialize_tree_value(store, &entry.source, left_value);
Expand Down

0 comments on commit a609580

Please sign in to comment.