From a609580204a4c8290a0c286a7d29974d781a93d6 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Fri, 9 Aug 2024 19:44:42 +0900
Subject: [PATCH] revset: avoid merging whole parent trees by
 file()/diff_contains() query
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 ..
```
---
 lib/src/default_index/revset_engine.rs | 29 ++++++++++++++++----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs
index 238be00b32..7eb6d5baac 100644
--- a/lib/src/default_index/revset_engine.rs
+++ b/lib/src/default_index/revset_engine.rs
@@ -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,
@@ -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()
 }
@@ -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);