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