Patchwork D7717: rust-discovery: restoring add_missing cheap early return

login
register
mail settings
Submitter phabricator
Date Dec. 24, 2019, 1:50 p.m.
Message ID <differential-rev-PHID-DREV-7byqwdwvuzcf7wzkwiy5-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44038/
State New
Headers show

Comments

phabricator - Dec. 24, 2019, 1:50 p.m.
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




To: gracinet, #hg-reviewers
Cc: durin42, kevincox, mjpieters, mercurial-devel
phabricator - April 22, 2020, 3:53 p.m.
Herald added a subscriber: mercurial-patches.
This revision now requires changes to proceed.
baymax added a comment.
baymax requested changes to this revision.


  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:

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7717/new/

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

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

Patch

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));