This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7717

rust-discovery: restoring add_missing cheap early return
Needs RevisionPublic

Authored by gracinet on Dec 24 2019, 8:49 AM.

Details

Reviewers
Alphare
baymax
Group Reviewers
hg-reviewers
Summary

In case the iterator of missing revisions argument turns out
to be empty, we need to refrain from computing the undecided
set. This way, we'll benefit from subsequent add_common_revisions
until there is an actually meaningful add_missing_revisions.

To do this with the typestate pattern, that means the early
return has to happen before potential transition into an
WithUndecided

Diff Detail

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

Event Timeline

gracinet created this revision.Dec 24 2019, 8:49 AM
Alphare accepted this revision.Jan 3 2020, 4:46 AM
baymax requested changes to this revision.Apr 22 2020, 11:53 AM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Apr 22 2020, 11:53 AM

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/discovery.rs (23 lines)

Diff 18925

rust/hg-core/src/discovery.rs

} }
} }
/// Register revisions known as being missing in remote /// Register revisions known as being missing in remote
pub fn add_missing_revisions( pub fn add_missing_revisions(
&mut self, &mut self,
missing: impl IntoIterator<Item = Revision>, missing: impl IntoIterator<Item = Revision>,
) -> Result<(), GraphError> { ) -> Result<(), GraphError> {
let missing: VecDeque<Revision> = missing.into_iter().collect();
if missing.is_empty() {
return Ok(());
}
self.mutate_undecided( self.mutate_undecided(
|oc| oc.compute_undecided(), |oc| oc.compute_undecided(),
|wu| wu.add_missing_revisions(missing), |wu| wu.add_missing_revisions(missing),
) )
} }
/// Mutate into a `WithUndecided` and apply the `mutator` closure. /// Mutate into a `WithUndecided` and apply the `mutator` closure.
/// ///
} }
self.common.remove_ancestors_from(&mut self.undecided) self.common.remove_ancestors_from(&mut self.undecided)
} }
/// Register revisions known as being missing /// Register revisions known as being missing
/// ///
/// # Performance note /// # Performance note
/// ///
/// Except in the most trivial case, the first call of this method has /// `tovisit` is provided as a `VecDeque` already so that a potential
/// the side effect of computing `self.undecided` set for the first time, /// caller from `PartialDiscovery` can avoid the undecided set in case
/// and the related caches it might need for efficiency of its internal /// it turns out to be empty (hence restrain
/// computation. This is typically faster if more information is /// from performing the transition to `WithUndecided`)
/// available in `self.common`. Therefore, for good performance, the
/// caller should avoid calling this too early.
pub fn add_missing_revisions( pub fn add_missing_revisions(
&mut self, &mut self,
missing: impl IntoIterator<Item = Revision>, mut tovisit: VecDeque<Revision>,
) -> Result<(), GraphError> { ) -> Result<(), GraphError> {
let mut tovisit: VecDeque<Revision> = missing.into_iter().collect();
if tovisit.is_empty() {
return Ok(());
}
self.ensure_children_cache()?; self.ensure_children_cache()?;
let children = self.children_cache.as_ref().unwrap(); let children = self.children_cache.as_ref().unwrap();
let mut seen: HashSet<Revision> = HashSet::new(); let mut seen: HashSet<Revision> = HashSet::new();
let undecided_mut = &mut self.undecided; let undecided_mut = &mut self.undecided;
while let Some(rev) = tovisit.pop_front() { while let Some(rev) = tovisit.pop_front() {
if !self.missing.insert(rev) { if !self.missing.insert(rev) {
// either it's known to be missing from a previous // either it's known to be missing from a previous
// invocation, and there's no need to iterate on its // invocation, and there's no need to iterate on its
disco.add_common_revisions(vec![11, 12])?; disco.add_common_revisions(vec![11, 12])?;
assert!(disco.has_info()); assert!(disco.has_info());
assert!(!disco.is_complete()); assert!(!disco.is_complete());
// add_common_revisions did not trigger a premature computation // add_common_revisions did not trigger a premature computation
// of `undecided`, let's check that, force the mutation and // of `undecided`, let's check that, force the mutation and
// ask for them // ask for them
assert_disco_is_only_commons(&disco); assert_disco_is_only_commons(&disco);
disco.add_missing_revisions(Vec::new())?; disco.mutate_undecided(|oc| oc.compute_undecided(), |_| Ok(()))?;
assert_disco_is_b(&disco); assert_disco_is_b(&disco);
assert_eq!(sorted_undecided(&disco), vec![5, 8, 10, 13]); assert_eq!(sorted_undecided(&disco), vec![5, 8, 10, 13]);
assert_eq!(disco.stats().undecided, Some(4)); assert_eq!(disco.stats().undecided, Some(4));
Ok(()) Ok(())
} }
/// in this test, we pretend that our peer misses exactly (8+10):: /// in this test, we pretend that our peer misses exactly (8+10)::
/// and we're comparing all our repo to it (as in a bare push) /// and we're comparing all our repo to it (as in a bare push)