Patchwork D6312: branchcache: update the filteredhash if we update the tiprev

login
register
mail settings
Submitter phabricator
Date April 26, 2019, 11:22 p.m.
Message ID <differential-rev-PHID-DREV-zkpyhlnli7a3hfqzci3o-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39837/
State New
Headers show

Comments

phabricator - April 26, 2019, 11:22 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We update the tiprev and in next if statement we check whether the branchcache
  is valid or not. If the update of tiprev happens, then definitely
  self.validfor() will return False because filteredhash is old now.
  
  Let's update the filteredhash if we update the tiprev also. This prevents us
  from entering the loop where we iter all the heads, and find the tiprev.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/branchmap.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - April 28, 2019, 8:36 a.m.
>          if ntiprev > self.tiprev:
>              self.tiprev = ntiprev
>              self.tipnode = cl.node(ntiprev)
> +            self.filteredhash = scmutil.filteredhash(repo, self.tiprev)
>  
>          if not self.validfor(repo):
>              # cache key are not valid anymore

and update `self.filteredhash` later again, which smells. Instead, shouldn't
we check the validity first, and take the fast path only if it was valid?

```
if self.validfor(repo):  # was valid, can take fast path
    self.tiprev = ntiprev
    self.tipnode = cl.node(ntiprev)
else:  # bad luck, recompute tiprev/tipnode
    ...
self.filteredhash = scmutil.filteredhash(repo, self.tiprev)
```
phabricator - April 28, 2019, 8:38 a.m.
yuja added a comment.


  >   if ntiprev > self.tiprev:
  >       self.tiprev = ntiprev
  >       self.tipnode = cl.node(ntiprev)
  > 
  > +            self.filteredhash = scmutil.filteredhash(repo, self.tiprev)
  > 
  >   if not self.validfor(repo):
  >       # cache key are not valid anymore
  
  and update `self.filteredhash` later again, which smells. Instead, shouldn't
  we check the validity first, and take the fast path only if it was valid?
  
    if self.validfor(repo):  # was valid, can take fast path
        self.tiprev = ntiprev
        self.tipnode = cl.node(ntiprev)
    else:  # bad luck, recompute tiprev/tipnode
        ...
    self.filteredhash = scmutil.filteredhash(repo, self.tiprev)

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/branchmap.py b/mercurial/branchmap.py
--- a/mercurial/branchmap.py
+++ b/mercurial/branchmap.py
@@ -411,6 +411,7 @@ 
         if ntiprev > self.tiprev:
             self.tiprev = ntiprev
             self.tipnode = cl.node(ntiprev)
+            self.filteredhash = scmutil.filteredhash(repo, self.tiprev)
 
         if not self.validfor(repo):
             # cache key are not valid anymore