This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10550

dirstate-tree: Avoid BTreeMap double-lookup when inserting a dirstate entry
ClosedPublic

Authored by SimonSapin on May 3 2021, 6:25 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG04bcba539c96: dirstate-tree: Avoid BTreeMap double-lookup when inserting a dirstate entry
rHG660a9301073e: dirstate-tree: Avoid BTreeMap double-lookup when inserting a dirstate entry
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

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Revision Contents
Changeset List

Diff 27660

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

) -> &'tree mut Node { ) -> &'tree mut Node {
let mut child_nodes = root; let mut child_nodes = root;
let mut inclusive_ancestor_paths = let mut inclusive_ancestor_paths =
WithBasename::inclusive_ancestors_of(path); WithBasename::inclusive_ancestors_of(path);
let mut ancestor_path = inclusive_ancestor_paths let mut ancestor_path = inclusive_ancestor_paths
.next() .next()
.expect("expected at least one inclusive ancestor"); .expect("expected at least one inclusive ancestor");
loop { loop {
// TODO: can we avoid double lookup in all cases without allocating // TODO: can we avoid allocating an owned key in cases where the
// an owned key in cases where the map already contains that key? // map already contains that key, without introducing double
// lookup?
let child_node = let child_node =
if child_nodes.contains_key(ancestor_path.base_name()) { child_nodes.entry(ancestor_path.to_owned()).or_default();
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()
};
if let Some(next) = inclusive_ancestor_paths.next() { if let Some(next) = inclusive_ancestor_paths.next() {
ancestor_path = next; ancestor_path = next;
child_nodes = &mut child_node.children; child_nodes = &mut child_node.children;
} else { } else {
return child_node; return child_node;
} }
} }
} }