Patchwork [4,of,5] phases: calculate the maxphase and minphaserev

login
register
mail settings
Submitter Durham Goode
Date Oct. 9, 2014, 7:22 p.m.
Message ID <80710688335b3e647bbf.1412882562@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/6171/
State Changes Requested
Headers show

Comments

Durham Goode - Oct. 9, 2014, 7:22 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1412707606 25200
#      Tue Oct 07 11:46:46 2014 -0700
# Node ID 80710688335b3e647bbf74700cb6e1a464b909b2
# Parent  9291d6a89c9b9bf93cbb920c98795fa13fbd6fa0
phases: calculate the maxphase and minphaserev

This calculates and stores the maxphase in the repo, and the minimum phased rev.
These will be used in future patches to optimize the phase computation. It's
already used in this patch to optimize returning public (0) for revs below the
minimum phased rev.
Pierre-Yves David - Oct. 13, 2014, 6:33 a.m.
On 10/09/2014 12:22 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1412707606 25200
> #      Tue Oct 07 11:46:46 2014 -0700
> # Node ID 80710688335b3e647bbf74700cb6e1a464b909b2
> # Parent  9291d6a89c9b9bf93cbb920c98795fa13fbd6fa0
> phases: calculate the maxphase and minphaserev

Patch idea looks fine. But I've a couple of technical question.

 From a patchbombing perspective it would have been nice to group it 
with something actually using all the new concept. (for example, could 
have been swaped with the next ones.)

> This calculates and stores the maxphase in the repo, and the minimum phased rev.
> These will be used in future patches to optimize the phase computation. It's
> already used in this patch to optimize returning public (0) for revs below the
> minimum phased rev.
>
> diff --git a/mercurial/phases.py b/mercurial/phases.py
> --- a/mercurial/phases.py
> +++ b/mercurial/phases.py
> @@ -146,6 +146,10 @@ class phasecache(object):
>               # Cheap trick to allow shallow-copy without copy module
>               self.phaseroots, self.dirty = _readroots(repo, phasedefaults)
>               self._phaserevs = None
> +            # The maximum phase in this repository.
> +            self._maxphase = 0
> +            # The minimum rev in the repo that is not phase 0.
> +            self._minphaserev = 0

Looks like you are using "0" as "not-initialized value" while 0 is a 
valid value in both case. Could we use None here instead?

>               self.filterunknown(repo)
>               self.opener = repo.sopener
>
> @@ -157,10 +161,14 @@ class phasecache(object):
>           ph.dirty = self.dirty
>           ph.opener = self.opener
>           ph._phaserevs = self._phaserevs
> +        ph._maxphase = self._maxphase
> +        ph._minphaserev = self._minphaserev
>           return ph
>
>       def replace(self, phcache):
> -        for a in 'phaseroots dirty opener _phaserevs'.split():
> +        attributes = ['phaseroots', 'dirty', 'opener', '_phaserevs',
> +            '_maxphase', '_minphaserev']

bad indentation. should be

 > +        attributes = ['phaseroots', 'dirty', 'opener', '_phaserevs',
 > +                      '_maxphase', '_minphaserev']


> +        for a in attributes:
>               setattr(self, a, getattr(phcache, a))
>
>       def getphaserevs(self, repo):
> @@ -180,14 +188,19 @@ class phasecache(object):
>
>       def invalidate(self):
>           self._phaserevs = None
> +        self._maxphase = 0
> +        self._minphaserev = 0

same feedback about using 0 as default value.

>
>       def _populatephaseroots(self, repo):
>           """Fills the _phaserevs cache with phases for the roots.
>           """
> +        self._minphaserev = len(repo)
>           cl = repo.changelog
>           phaserevs = self._phaserevs
>           for phase in trackedphases:
>               roots = map(cl.rev, self.phaseroots[phase])
> +            if roots:
> +                self._maxphase = max(self._maxphase, phase)
>               for root in roots:
>                   phaserevs[root] = phase
>                   # We know that the parent of a root has a phase less than
> @@ -196,6 +209,7 @@ class phasecache(object):
>                       for parent in cl.parentrevs(root):
>                           if parent != -1:
>                               phaserevs[parent] = 0
> +                self._minphaserev = min(self._minphaserev, root)
>
>       def phase(self, repo, rev):
>           # We need a repo argument here to be able to build _phaserevs
> @@ -207,6 +221,8 @@ class phasecache(object):
>               return public
>           if rev < nullrev:
>               raise ValueError(_('cannot lookup negative revision'))
> +        if rev < self._minphaserev or rev <= nullrev:
> +            return 0
>           if self._phaserevs is None or rev >= len(self._phaserevs):
>               self.invalidate()
>               self._phaserevs = self.getphaserevs(repo)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/phases.py b/mercurial/phases.py
--- a/mercurial/phases.py
+++ b/mercurial/phases.py
@@ -146,6 +146,10 @@  class phasecache(object):
             # Cheap trick to allow shallow-copy without copy module
             self.phaseroots, self.dirty = _readroots(repo, phasedefaults)
             self._phaserevs = None
+            # The maximum phase in this repository.
+            self._maxphase = 0
+            # The minimum rev in the repo that is not phase 0.
+            self._minphaserev = 0
             self.filterunknown(repo)
             self.opener = repo.sopener
 
@@ -157,10 +161,14 @@  class phasecache(object):
         ph.dirty = self.dirty
         ph.opener = self.opener
         ph._phaserevs = self._phaserevs
+        ph._maxphase = self._maxphase
+        ph._minphaserev = self._minphaserev
         return ph
 
     def replace(self, phcache):
-        for a in 'phaseroots dirty opener _phaserevs'.split():
+        attributes = ['phaseroots', 'dirty', 'opener', '_phaserevs',
+            '_maxphase', '_minphaserev']
+        for a in attributes:
             setattr(self, a, getattr(phcache, a))
 
     def getphaserevs(self, repo):
@@ -180,14 +188,19 @@  class phasecache(object):
 
     def invalidate(self):
         self._phaserevs = None
+        self._maxphase = 0
+        self._minphaserev = 0
 
     def _populatephaseroots(self, repo):
         """Fills the _phaserevs cache with phases for the roots.
         """
+        self._minphaserev = len(repo)
         cl = repo.changelog
         phaserevs = self._phaserevs
         for phase in trackedphases:
             roots = map(cl.rev, self.phaseroots[phase])
+            if roots:
+                self._maxphase = max(self._maxphase, phase)
             for root in roots:
                 phaserevs[root] = phase
                 # We know that the parent of a root has a phase less than
@@ -196,6 +209,7 @@  class phasecache(object):
                     for parent in cl.parentrevs(root):
                         if parent != -1:
                             phaserevs[parent] = 0
+                self._minphaserev = min(self._minphaserev, root)
 
     def phase(self, repo, rev):
         # We need a repo argument here to be able to build _phaserevs
@@ -207,6 +221,8 @@  class phasecache(object):
             return public
         if rev < nullrev:
             raise ValueError(_('cannot lookup negative revision'))
+        if rev < self._minphaserev or rev <= nullrev:
+            return 0
         if self._phaserevs is None or rev >= len(self._phaserevs):
             self.invalidate()
             self._phaserevs = self.getphaserevs(repo)