[Request] [+ ] D10550: dirstate-tree: Avoid BTreeMap double-lookup when inserting a dirstate entry

[SimonSapin]

SimonSapin created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  The child nodes of a given node in the tree-shaped dirstate are kept in a
  `BTreeMap` where keys are file names as strings. Finding or inserting a value
  in the map takes `O(log(n))` string comparisons, which adds up when constructing
  the tree.
  
  The `entry` API allows finding a "spot" in the map that may or may not be
  occupied and then access that value or insert a new one without doing map
  lookup again. However the current API is limited in that calling `entry`
  requires an owned key (and so a memory allocation), even if it ends up not
  being used in the case where the map already has a value with an equal key.
  
  This is still a win, with 4% better end-to-end time for `hg status` measured
  here with hyperfine:
  
    Benchmark #1: ../hg2/hg status -R $REPO --config=experimental.dirstate-tree.in-memory=1
      Time (mean ± σ):      1.337 s ±  0.018 s    [User: 892.9 ms, System: 437.5 ms]
      Range (min … max):    1.316 s …  1.373 s    10 runs
    
    Benchmark #2: ./hg status -R $REPO --config=experimental.dirstate-tree.in-memory=1
      Time (mean ± σ):      1.291 s ±  0.008 s    [User: 853.4 ms, System: 431.1 ms]
      Range (min … max):    1.283 s …  1.309 s    10 runs
    
    Summary
      './hg status -R $REPO --config=experimental.dirstate-tree.in-memory=1' ran
        1.04 ± 0.02 times faster than '../hg2/hg status -R $REPO --config=experimental.dirstate-tree.in-memory=1'
  
  
  
  - ./hg is this revision
  - ../hg2/hg is its parent
  - $REPO is an old snapshot of mozilla-central

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D10550

AFFECTED FILES
  rust/hg-core/src/dirstate_tree/dirstate_map.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/dirstate_tree/dirstate_map.rs b/rust/hg-core/src/dirstate_tree/dirstate_map.rs
--- a/rust/hg-core/src/dirstate_tree/dirstate_map.rs
+++ b/rust/hg-core/src/dirstate_tree/dirstate_map.rs
@@ -169,17 +169,11 @@
             .next()
             .expect("expected at least one inclusive ancestor");
         loop {
-            // TODO: can we avoid double lookup in all cases without allocating
-            // an owned key in cases where the map already contains that key?
+            // TODO: can we avoid allocating an owned key in cases where the
+            // map already contains that key, without introducing double
+            // lookup?
             let child_node =
-                if child_nodes.contains_key(ancestor_path.base_name()) {
-                    child_nodes.get_mut(ancestor_path.base_name()).unwrap()
-                } else {
-                    // This is always a vacant entry, using `.entry()` lets us
-                    // return a `&mut Node` of the newly-inserted node without
-                    // yet another lookup. `BTreeMap::insert` doesn’t do this.
-                    child_nodes.entry(ancestor_path.to_owned()).or_default()
-                };
+                child_nodes.entry(ancestor_path.to_owned()).or_default();
             if let Some(next) = inclusive_ancestor_paths.next() {
                 ancestor_path = next;
                 child_nodes = &mut child_node.children;



To: SimonSapin, #hg-reviewers
Cc: mercurial-patches, mercurial-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mercurial-scm.org/pipermail/mercurial-patches/attachments/20210503/def75757/attachment.html>