Speed up implementation of tree::overlay (#1294)

By using only a single treebuilder and no intermediate
trees a lot of time and io is saved.
Workspace rebuilds are much faster with this change.

Change: faster-overlay

Author: christian-schilling

josh-core/src/cache.rs

@@ -1,7 +1,7 @@
 use super::*;
 use std::collections::HashMap;
 
-const CACHE_VERSION: u64 = 16;
+const CACHE_VERSION: u64 = 17;
 
 lazy_static! {
     static ref DB: std::sync::Mutex<Option<sled::Db>> = std::sync::Mutex::new(None);
@@ -54,6 +54,7 @@ struct Transaction2 {
     commit_map: HashMap<git2::Oid, HashMap<git2::Oid, git2::Oid>>,
     apply_map: HashMap<git2::Oid, HashMap<git2::Oid, git2::Oid>>,
     subtract_map: HashMap<(git2::Oid, git2::Oid), git2::Oid>,
+    overlay_map: HashMap<(git2::Oid, git2::Oid), git2::Oid>,
     unapply_map: HashMap<git2::Oid, HashMap<git2::Oid, git2::Oid>>,
     sled_trees: HashMap<git2::Oid, sled::Tree>,
     path_tree: sled::Tree,
@@ -139,6 +140,7 @@ impl Transaction {
                 commit_map: HashMap::new(),
                 apply_map: HashMap::new(),
                 subtract_map: HashMap::new(),
+                overlay_map: HashMap::new(),
                 unapply_map: HashMap::new(),
                 sled_trees: HashMap::new(),
                 path_tree,
@@ -210,6 +212,16 @@ impl Transaction {
         return t2.subtract_map.get(&from).cloned();
     }
 
+    pub fn insert_overlay(&self, from: (git2::Oid, git2::Oid), to: git2::Oid) {
+        let mut t2 = self.t2.borrow_mut();
+        t2.overlay_map.insert(from, to);
+    }
+
+    pub fn get_overlay(&self, from: (git2::Oid, git2::Oid)) -> Option<git2::Oid> {
+        let t2 = self.t2.borrow_mut();
+        return t2.overlay_map.get(&from).cloned();
+    }
+
     pub fn insert_unapply(&self, filter: filter::Filter, from: git2::Oid, to: git2::Oid) {
         let mut t2 = self.t2.borrow_mut();
         t2.unapply_map

josh-core/src/filter/mod.rs

@@ -652,7 +652,7 @@ fn apply_to_commit2(
             let mut filtered_tree = commit.tree_id();
 
             for t in trees {
-                filtered_tree = tree::overlay(repo, filtered_tree, t)?;
+                filtered_tree = tree::overlay(transaction, filtered_tree, t)?;
             }
 
             repo.find_tree(filtered_tree)?
@@ -856,7 +856,7 @@ pub fn unapply<'a>(
         let new_tree = apply(transaction, inverted, tree)?;
 
         return Ok(transaction.repo().find_tree(tree::overlay(
-            transaction.repo(),
+            transaction,
             new_tree.id(),
             stripped,
         )?)?);
@@ -911,7 +911,7 @@ fn unapply_workspace<'a>(
             let new_tree = apply(transaction, invert(filter)?, tree)?;
 
             let result = transaction.repo().find_tree(tree::overlay(
-                transaction.repo(),
+                transaction,
                 new_tree.id(),
                 stripped,
             )?)?;

josh-core/src/filter/tree.rs

@@ -340,11 +340,14 @@ pub fn diff_paths(
 }
 
 pub fn overlay(
-    repo: &git2::Repository,
+    transaction: &cache::Transaction,
     input1: git2::Oid,
     input2: git2::Oid,
 ) -> JoshResult<git2::Oid> {
-    rs_tracing::trace_scoped!("overlay");
+    if let Some(cached) = transaction.get_overlay((input1, input2)) {
+        return Ok(cached);
+    }
+    let repo = transaction.repo();
     if input1 == input2 {
         return Ok(input1);
     }
@@ -356,29 +359,35 @@ pub fn overlay(
     }
 
     if let (Ok(tree1), Ok(tree2)) = (repo.find_tree(input1), repo.find_tree(input2)) {
-        let mut result_tree = tree1.clone();
+        rs_tracing::trace_begin!( "overlay",
+            "overlay_a": format!("{}", input1),
+            "overlay_b": format!("{}", input2),
+            "overlay_ab": format!("{} - {}", input1, input2));
+        let mut builder = repo.treebuilder(Some(&tree1))?;
 
+        let mut i = 0;
         for entry in tree2.iter() {
-            if let Some(e) = tree1.get_name(entry.name().ok_or_else(|| josh_error("no name"))?) {
-                result_tree = replace_child(
-                    repo,
-                    Path::new(entry.name().ok_or_else(|| josh_error("no name"))?),
-                    overlay(repo, e.id(), entry.id())?,
-                    e.filemode(),
-                    &result_tree,
-                )?;
+            i += 1;
+            let (id, mode) = if let Some(e) =
+                tree1.get_name(entry.name().ok_or_else(|| josh_error("no name"))?)
+            {
+                (overlay(transaction, e.id(), entry.id())?, e.filemode())
             } else {
-                result_tree = replace_child(
-                    repo,
-                    Path::new(entry.name().ok_or_else(|| josh_error("no name"))?),
-                    entry.id(),
-                    entry.filemode(),
-                    &result_tree,
-                )?;
-            }
+                (entry.id(), entry.filemode())
+            };
+
+            builder.insert(
+                Path::new(entry.name().ok_or_else(|| josh_error("no name"))?),
+                id,
+                mode,
+            )?;
         }
 
-        return Ok(result_tree.id());
+        let rid = builder.write()?;
+        rs_tracing::trace_end!( "overlay", "count":i);
+
+        transaction.insert_overlay((input1, input2), rid);
+        return Ok(rid);
     }
 
     Ok(input1)
@@ -837,7 +846,7 @@ pub fn invert_paths<'a>(
                 &format!("{}{}{}", root, if root.is_empty() { "" } else { "/" }, name),
                 repo.find_tree(entry.id())?,
             )?;
-            result = repo.find_tree(overlay(repo, result.id(), s.id())?)?;
+            result = repo.find_tree(overlay(transaction, result.id(), s.id())?)?;
         }
     }
 
@@ -897,7 +906,7 @@ fn populate(
         for entry in content.iter() {
             if let Some(e) = paths.get_name(entry.name().ok_or_else(|| josh_error("no name"))?) {
                 result_tree = overlay(
-                    repo,
+                    transaction,
                     result_tree,
                     populate(transaction, e.id(), entry.id())?,
                 )?;
@@ -918,7 +927,7 @@ pub fn compose_fast(
     let repo = transaction.repo();
     let mut result = empty_id();
     for tree in trees {
-        result = overlay(repo, tree, result)?;
+        result = overlay(transaction, tree, result)?;
     }
 
     Ok(repo.find_tree(result)?)
@@ -950,8 +959,8 @@ pub fn compose<'a>(
             apply(transaction, invert(*f)?, applied)?.id()
         };
         transaction.insert_unapply(*f, aid, unapplied);
-        taken = repo.find_tree(overlay(repo, taken.id(), unapplied)?)?;
-        result = repo.find_tree(overlay(repo, subtracted.id(), result.id())?)?;
+        taken = repo.find_tree(overlay(transaction, taken.id(), unapplied)?)?;
+        result = repo.find_tree(overlay(transaction, subtracted.id(), result.id())?)?;
     }
 
     Ok(result)

tests/proxy/amend_patchset.t

@@ -124,7 +124,7 @@
   ]
   .
   |-- josh
-  |   `-- 16
+  |   `-- 17
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/authentication.t

@@ -124,7 +124,7 @@
   "real_repo.git" = ["::sub1/"]
   .
   |-- josh
-  |   `-- 16
+  |   `-- 17
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/caching.t

@@ -51,7 +51,7 @@
   ]
   .
   |-- josh
-  |   `-- 16
+  |   `-- 17
   |       `-- sled
   |           |-- blobs
   |           |-- conf
@@ -162,7 +162,7 @@
   ]
   .
   |-- josh
-  |   `-- 16
+  |   `-- 17
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/clone_absent_head.t

@@ -85,7 +85,7 @@
   $ bash ${TESTDIR}/destroy_test_env.sh
   .
   |-- josh
-  |   `-- 16
+  |   `-- 17
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/clone_all.t

@@ -53,7 +53,7 @@
   "real_repo.git" = ["::sub1/"]
   .
   |-- josh
-  |   `-- 16
+  |   `-- 17
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/clone_blocked.t

@@ -9,7 +9,7 @@
   $ bash ${TESTDIR}/destroy_test_env.sh
   .
   |-- josh
-  |   `-- 16
+  |   `-- 17
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/clone_invalid_url.t

@@ -32,7 +32,7 @@
   $ bash ${TESTDIR}/destroy_test_env.sh
   .
   |-- josh
-  |   `-- 16
+  |   `-- 17
   |       `-- sled
   |           |-- blobs
   |           |-- conf