D7717: rust-discovery: restoring add_missing cheap early return

[gracinet (Georges Racinet)]

gracinet created this revision.
Herald added subscribers: mercurial-devel, mjpieters, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION 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

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-core/src/discovery.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/discovery.rs b/rust/hg-core/src/discovery.rs
--- a/rust/hg-core/src/discovery.rs
+++ b/rust/hg-core/src/discovery.rs
@@ -256,6 +256,11 @@
         &mut self,
         missing: impl IntoIterator<Item = Revision>,
     ) -> Result<(), GraphError> {
+        let missing: VecDeque<Revision> = missing.into_iter().collect();
+        if missing.is_empty() {
+            return Ok(());
+        }
+
         self.mutate_undecided(
             |oc| oc.compute_undecided(),
             |wu| wu.add_missing_revisions(missing),
@@ -431,20 +436,14 @@
     ///
     /// # Performance note
     ///
-    /// Except in the most trivial case, the first call of this method has
-    /// the side effect of computing `self.undecided` set for the first time,
-    /// and the related caches it might need for efficiency of its internal
-    /// computation. This is typically faster if more information is
-    /// available in `self.common`. Therefore, for good performance, the
-    /// caller should avoid calling this too early.
+    /// `tovisit` is provided as a `VecDeque` already so that a potential
+    /// caller from `PartialDiscovery` can avoid the undecided set in case
+    /// it turns out to be empty (hence restrain
+    /// from performing the transition to `WithUndecided`)
     pub fn add_missing_revisions(
         &mut self,
-        missing: impl IntoIterator<Item = Revision>,
+        mut tovisit: VecDeque<Revision>,
     ) -> Result<(), GraphError> {
-        let mut tovisit: VecDeque<Revision> = missing.into_iter().collect();
-        if tovisit.is_empty() {
-            return Ok(());
-        }
         self.ensure_children_cache()?;
         let children = self.children_cache.as_ref().unwrap();
         let mut seen: HashSet<Revision> = HashSet::new();
@@ -773,7 +772,7 @@
         // of `undecided`, let's check that, force the mutation and
         // ask for them
         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_eq!(sorted_undecided(&disco), vec![5, 8, 10, 13]);
         assert_eq!(disco.stats().undecided, Some(4));



To: gracinet, #hg-reviewers
Cc: durin42, kevincox, mjpieters, mercurial-devel