This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8048

rust-dirstatemap: cache non normal and other parent set
ClosedPublic

Authored by Alphare on Jan 30 2020, 9:09 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHG83b2b829c94e: rust-dirstatemap: cache non normal and other parent set
Summary

Performance of hg update was significantly worse since the introduction of
the Rust dirstatemap. This regression was noticed by Valentin Gatien-Baron
when working on a large repository, as it goes unnoticed for smaller
repositories like Mercurial itself.

This fix introduces the same getter/setter mechanism at hg-core level as
for set/get_dirs.

While this technique is, as previously discussed, quite suboptimal, it fixes an
important enough problem. Refactoring hg-core to use the typestate
pattern could be a good approach to improving code quality in a future patch.

Diff Detail

Repository
rHG Mercurial
Branch
stable
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Jan 30 2020, 9:09 AM
marmoute accepted this revision.Feb 7 2020, 3:14 PM
marmoute added a subscriber: marmoute.

As you warned, this is not pretty. However that seems failry straightforward and does the job.

Alphare added a commit: rHG83b2b829c94e: rust-dirstatemap: cache non normal and other parent set.Feb 10 2020, 4:27 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG83b2b829c94e: rust-dirstatemap: cache non normal and other parent set (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.
Alphare updated this revision to Diff 20060.Feb 10 2020, 4:44 PM
kevincox added inline comments.Feb 12 2020, 5:01 AM
rust/hg-core/src/dirstate/dirstate_map.rs
251

If we have just set the fields to Some(..) in the previous line can't we do the unwrap here where it is obviously correct?

Revision Contents
Changeset List

Diff 19712

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

#[derive(Default)] #[derive(Default)]
pub struct DirstateMap { pub struct DirstateMap {
state_map: StateMap, state_map: StateMap,
pub copy_map: CopyMap, pub copy_map: CopyMap,
file_fold_map: Option<FileFoldMap>, file_fold_map: Option<FileFoldMap>,
pub dirs: Option<DirsMultiset>, pub dirs: Option<DirsMultiset>,
pub all_dirs: Option<DirsMultiset>, pub all_dirs: Option<DirsMultiset>,
non_normal_set: HashSet<HgPathBuf>, non_normal_set: Option<HashSet<HgPathBuf>>,
other_parent_set: HashSet<HgPathBuf>, other_parent_set: Option<HashSet<HgPathBuf>>,
parents: Option<DirstateParents>, parents: Option<DirstateParents>,
dirty_parents: bool, dirty_parents: bool,
} }
/// Should only really be used in python interface code, for clarity /// Should only really be used in python interface code, for clarity
impl Deref for DirstateMap { impl Deref for DirstateMap {
type Target = StateMap; type Target = StateMap;
pub fn new() -> Self { pub fn new() -> Self {
Self::default() Self::default()
} }
pub fn clear(&mut self) { pub fn clear(&mut self) {
self.state_map.clear(); self.state_map.clear();
self.copy_map.clear(); self.copy_map.clear();
self.file_fold_map = None; self.file_fold_map = None;
self.non_normal_set.clear(); self.non_normal_set = None;
self.other_parent_set.clear(); self.other_parent_set = None;
self.set_parents(&DirstateParents { self.set_parents(&DirstateParents {
p1: NULL_ID, p1: NULL_ID,
p2: NULL_ID, p2: NULL_ID,
}) })
} }
/// Add a tracked file to the dirstate /// Add a tracked file to the dirstate
pub fn add_file( pub fn add_file(
if old_state == EntryState::Unknown { if old_state == EntryState::Unknown {
if let Some(ref mut all_dirs) = self.all_dirs { if let Some(ref mut all_dirs) = self.all_dirs {
all_dirs.add_path(filename)?; all_dirs.add_path(filename)?;
} }
} }
self.state_map.insert(filename.to_owned(), entry.to_owned()); self.state_map.insert(filename.to_owned(), entry.to_owned());
if entry.state != EntryState::Normal || entry.mtime == MTIME_UNSET { if entry.state != EntryState::Normal || entry.mtime == MTIME_UNSET {
self.non_normal_set.insert(filename.to_owned()); self.get_non_normal_other_parent_entries()
.0
.as_mut()
.unwrap()
.insert(filename.to_owned());
} }
if entry.size == SIZE_FROM_OTHER_PARENT { if entry.size == SIZE_FROM_OTHER_PARENT {
self.other_parent_set.insert(filename.to_owned()); self.get_non_normal_other_parent_entries()
.1
.as_mut()
.unwrap()
.insert(filename.to_owned());
} }
Ok(()) Ok(())
} }
/// Mark a file as removed in the dirstate. /// Mark a file as removed in the dirstate.
/// ///
/// The `size` parameter is used to store sentinel values that indicate /// The `size` parameter is used to store sentinel values that indicate
/// the file's previous state. In the future, we should refactor this /// the file's previous state. In the future, we should refactor this
filename.to_owned(), filename.to_owned(),
DirstateEntry { DirstateEntry {
state: EntryState::Removed, state: EntryState::Removed,
mode: 0, mode: 0,
size, size,
mtime: 0, mtime: 0,
}, },
); );
self.non_normal_set.insert(filename.to_owned()); self.get_non_normal_other_parent_entries()
.0
.as_mut()
.unwrap()
.insert(filename.to_owned());
Ok(()) Ok(())
} }
/// Remove a file from the dirstate. /// Remove a file from the dirstate.
/// Returns `true` if the file was previously recorded. /// Returns `true` if the file was previously recorded.
pub fn drop_file( pub fn drop_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
} }
if let Some(ref mut all_dirs) = self.all_dirs { if let Some(ref mut all_dirs) = self.all_dirs {
all_dirs.delete_path(filename)?; all_dirs.delete_path(filename)?;
} }
} }
if let Some(ref mut file_fold_map) = self.file_fold_map { if let Some(ref mut file_fold_map) = self.file_fold_map {
file_fold_map.remove(&normalize_case(filename)); file_fold_map.remove(&normalize_case(filename));
} }
self.non_normal_set.remove(filename); self.get_non_normal_other_parent_entries()
.0
.as_mut()
.unwrap()
.remove(filename);
Ok(exists) Ok(exists)
} }
pub fn clear_ambiguous_times( pub fn clear_ambiguous_times(
&mut self, &mut self,
filenames: Vec<HgPathBuf>, filenames: Vec<HgPathBuf>,
now: i32, now: i32,
) { ) {
for filename in filenames { for filename in filenames {
let mut changed = false; let mut changed = false;
self.state_map self.state_map
.entry(filename.to_owned()) .entry(filename.to_owned())
.and_modify(|entry| { .and_modify(|entry| {
if entry.state == EntryState::Normal && entry.mtime == now if entry.state == EntryState::Normal && entry.mtime == now
{ {
changed = true; changed = true;
*entry = DirstateEntry { *entry = DirstateEntry {
mtime: MTIME_UNSET, mtime: MTIME_UNSET,
..*entry ..*entry
}; };
} }
}); });
if changed { if changed {
self.non_normal_set.insert(filename.to_owned()); self.get_non_normal_other_parent_entries()
.0
.as_mut()
.unwrap()
.insert(filename.to_owned());
} }
} }
} }
pub fn non_normal_other_parent_entries( pub fn non_normal_entries_remove(
&self, &mut self,
) -> (HashSet<HgPathBuf>, HashSet<HgPathBuf>) { key: impl AsRef<HgPath>,
) -> bool {
self.get_non_normal_other_parent_entries()
.0
.as_mut()
.unwrap()
.remove(key.as_ref())
}
pub fn non_normal_entries_union(
&mut self,
other: HashSet<HgPathBuf>,
) -> Vec<HgPathBuf> {
self.get_non_normal_other_parent_entries()
.0
.as_mut()
.unwrap()
.union(&other)
.map(|e| e.to_owned())
.collect()
}
pub fn get_non_normal_other_parent_entries(
&mut self,
) -> (
&mut Option<HashSet<HgPathBuf>>,
&mut Option<HashSet<HgPathBuf>>,
) {
self.set_non_normal_other_parent_entries(false);
(&mut self.non_normal_set, &mut self.other_parent_set)
kevincoxUnsubmitted
Not Done

If we have just set the fields to Some(..) in the previous line can't we do the unwrap here where it is obviously correct?

kevincox: If we have just set the fields to `Some(..)` in the previous line can't we do the unwrap here…
}
pub fn set_non_normal_other_parent_entries(&mut self, force: bool) {
if !force
&& self.non_normal_set.is_some()
&& self.other_parent_set.is_some()
{
return;
}
let mut non_normal = HashSet::new(); let mut non_normal = HashSet::new();
let mut other_parent = HashSet::new(); let mut other_parent = HashSet::new();
for ( for (
filename, filename,
DirstateEntry { DirstateEntry {
state, size, mtime, .. state, size, mtime, ..
}, },
) in self.state_map.iter() ) in self.state_map.iter()
{ {
if *state != EntryState::Normal || *mtime == MTIME_UNSET { if *state != EntryState::Normal || *mtime == MTIME_UNSET {
non_normal.insert(filename.to_owned()); non_normal.insert(filename.to_owned());
} }
if *state == EntryState::Normal && *size == SIZE_FROM_OTHER_PARENT if *state == EntryState::Normal && *size == SIZE_FROM_OTHER_PARENT
{ {
other_parent.insert(filename.to_owned()); other_parent.insert(filename.to_owned());
} }
} }
self.non_normal_set = Some(non_normal);
(non_normal, other_parent) self.other_parent_set = Some(other_parent);
} }
/// Both of these setters and their uses appear to be the simplest way to /// Both of these setters and their uses appear to be the simplest way to
/// emulate a Python lazy property, but it is ugly and unidiomatic. /// emulate a Python lazy property, but it is ugly and unidiomatic.
/// TODO One day, rewriting this struct using the typestate might be a /// TODO One day, rewriting this struct using the typestate might be a
/// good idea. /// good idea.
pub fn set_all_dirs(&mut self) -> Result<(), DirstateMapError> { pub fn set_all_dirs(&mut self) -> Result<(), DirstateMapError> {
if self.all_dirs.is_none() { if self.all_dirs.is_none() {
parents: DirstateParents, parents: DirstateParents,
now: Duration, now: Duration,
) -> Result<Vec<u8>, DirstateError> { ) -> Result<Vec<u8>, DirstateError> {
let packed = let packed =
pack_dirstate(&mut self.state_map, &self.copy_map, parents, now)?; pack_dirstate(&mut self.state_map, &self.copy_map, parents, now)?;
self.dirty_parents = false; self.dirty_parents = false;
let result = self.non_normal_other_parent_entries(); self.set_non_normal_other_parent_entries(true);
self.non_normal_set = result.0;
self.other_parent_set = result.1;
Ok(packed) Ok(packed)
} }
pub fn build_file_fold_map(&mut self) -> &FileFoldMap { pub fn build_file_fold_map(&mut self) -> &FileFoldMap {
if let Some(ref file_fold_map) = self.file_fold_map { if let Some(ref file_fold_map) = self.file_fold_map {
return file_fold_map; return file_fold_map;
} }
let mut new_file_fold_map = FileFoldMap::default(); let mut new_file_fold_map = FileFoldMap::default();
mode: 1337, mode: 1337,
mtime: 1337, mtime: 1337,
size: 1337, size: 1337,
}, },
) )
.unwrap(); .unwrap();
assert_eq!(1, map.len()); assert_eq!(1, map.len());
assert_eq!(0, map.non_normal_set.len()); assert_eq!(
assert_eq!(0, map.other_parent_set.len()); 0,
map.get_non_normal_other_parent_entries()
.0
.as_ref()
.unwrap()
.len()
);
assert_eq!(
0,
map.get_non_normal_other_parent_entries()
.1
.as_ref()
.unwrap()
.len()
);
} }
#[test] #[test]
fn test_non_normal_other_parent_entries() { fn test_non_normal_other_parent_entries() {
let map: DirstateMap = [ let mut map: DirstateMap = [
(b"f1", (EntryState::Removed, 1337, 1337, 1337)), (b"f1", (EntryState::Removed, 1337, 1337, 1337)),
(b"f2", (EntryState::Normal, 1337, 1337, -1)), (b"f2", (EntryState::Normal, 1337, 1337, -1)),
(b"f3", (EntryState::Normal, 1337, 1337, 1337)), (b"f3", (EntryState::Normal, 1337, 1337, 1337)),
(b"f4", (EntryState::Normal, 1337, -2, 1337)), (b"f4", (EntryState::Normal, 1337, -2, 1337)),
(b"f5", (EntryState::Added, 1337, 1337, 1337)), (b"f5", (EntryState::Added, 1337, 1337, 1337)),
(b"f6", (EntryState::Added, 1337, 1337, -1)), (b"f6", (EntryState::Added, 1337, 1337, -1)),
(b"f7", (EntryState::Merged, 1337, 1337, -1)), (b"f7", (EntryState::Merged, 1337, 1337, -1)),
(b"f8", (EntryState::Merged, 1337, 1337, 1337)), (b"f8", (EntryState::Merged, 1337, 1337, 1337)),
b"f1", b"f2", b"f5", b"f6", b"f7", b"f8", b"f9", b"fa", b"fb", b"f1", b"f2", b"f5", b"f6", b"f7", b"f8", b"f9", b"fa", b"fb",
] ]
.iter() .iter()
.map(|x| HgPathBuf::from_bytes(x.as_ref())) .map(|x| HgPathBuf::from_bytes(x.as_ref()))
.collect(); .collect();
let mut other_parent = HashSet::new(); let mut other_parent = HashSet::new();
other_parent.insert(HgPathBuf::from_bytes(b"f4")); other_parent.insert(HgPathBuf::from_bytes(b"f4"));
let entries = map.get_non_normal_other_parent_entries();
assert_eq!( assert_eq!(
(non_normal, other_parent), (Some(non_normal), Some(other_parent)),
map.non_normal_other_parent_entries() (entries.0.to_owned(), entries.1.to_owned())
); );
} }
} }