Patchwork [4,of,4,STABLE] manifest: make treemanifestctx store the repo

login
register
mail settings
Submitter Durham Goode
Date Oct. 19, 2016, 12:50 a.m.
Message ID <d5bc7048144e6c967513.1476838217@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/17173/
State Accepted
Headers show

Comments

Durham Goode - Oct. 19, 2016, 12:50 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1476837882 25200
#      Tue Oct 18 17:44:42 2016 -0700
# Branch stable
# Node ID d5bc7048144e6c9675134b85aadb9d7b69d406aa
# Parent  3efece5c59853908d65de53635488361dbf20c49
manifest: make treemanifestctx store the repo

Same as in the last commit, the old treemanifestctx stored a reference to the
revlog.  If the inmemory revlog became invalid, the ctx now held an old copy and
would be incorrect. To fix this, we need the ctx to go through the manifestlog
for each access.

This is the same pattern that changectx already uses (it stores the repo, and
accesses commit data through self._repo.changelog).
Durham Goode - Oct. 19, 2016, 12:54 a.m.
Foozy pointed this out to me at the sprint and I didn't manage to get it 
done in time for the freeze.  This series fixes a pretty notable 
regression from the old manifest code in that the manifestlog is being 
recreated every time repo.manifestlog is called.


On 10/18/16 5:50 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1476837882 25200
> #      Tue Oct 18 17:44:42 2016 -0700
> # Branch stable
> # Node ID d5bc7048144e6c9675134b85aadb9d7b69d406aa
> # Parent  3efece5c59853908d65de53635488361dbf20c49
> manifest: make treemanifestctx store the repo
>
> Same as in the last commit, the old treemanifestctx stored a reference to the
> revlog.  If the inmemory revlog became invalid, the ctx now held an old copy and
> would be incorrect. To fix this, we need the ctx to go through the manifestlog
> for each access.
>
> This is the same pattern that changectx already uses (it stores the repo, and
> accesses commit data through self._repo.changelog).
>
> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
> --- a/mercurial/manifest.py
> +++ b/mercurial/manifest.py
> @@ -1274,7 +1274,7 @@ class manifestlog(object):
>                   return cachemf
>   
>           if self._treeinmem:
> -            m = treemanifestctx(self._revlog, '', node)
> +            m = treemanifestctx(self._repo, '', node)
>           else:
>               m = manifestctx(self._repo, node)
>           if node != revlog.nullid:
> @@ -1344,9 +1344,8 @@ class manifestctx(object):
>           return manifestdict(d)
>   
>   class treemanifestctx(object):
> -    def __init__(self, revlog, dir, node):
> -        revlog = revlog.dirlog(dir)
> -        self._revlog = revlog
> +    def __init__(self, repo, dir, node):
> +        self._repo = repo
>           self._dir = dir
>           self._data = None
>   
> @@ -1359,23 +1358,27 @@ class treemanifestctx(object):
>           #rev = revlog.rev(node)
>           #self.linkrev = revlog.linkrev(rev)
>   
> +    def _revlog(self):
> +        return self._repo.manifestlog._revlog.dirlog(self._dir)
> +
>       def read(self):
>           if not self._data:
> +            rl = self._revlog()
>               if self._node == revlog.nullid:
>                   self._data = treemanifest()
> -            elif self._revlog._treeondisk:
> +            elif rl._treeondisk:
>                   m = treemanifest(dir=self._dir)
>                   def gettext():
> -                    return self._revlog.revision(self._node)
> +                    return rl.revision(self._node)
>                   def readsubtree(dir, subm):
> -                    return treemanifestctx(self._revlog, dir, subm).read()
> +                    return treemanifestctx(self._repo, dir, subm).read()
>                   m.read(gettext, readsubtree)
>                   m.setnode(self._node)
>                   self._data = m
>               else:
> -                text = self._revlog.revision(self._node)
> +                text = revlog.revision(self._node)
>                   arraytext = array.array('c', text)
> -                self._revlog.fulltextcache[self._node] = arraytext
> +                rl.fulltextcache[self._node] = arraytext
>                   self._data = treemanifest(dir=self._dir, text=text)
>   
>           return self._data
> @@ -1385,9 +1388,9 @@ class treemanifestctx(object):
>   
>       def readdelta(self):
>           # Need to perform a slow delta
> -        revlog = self._revlog
> +        revlog = self._revlog()
>           r0 = revlog.deltaparent(revlog.rev(self._node))
> -        m0 = treemanifestctx(revlog, self._dir, revlog.node(r0)).read()
> +        m0 = treemanifestctx(self._repo, self._dir, revlog.node(r0)).read()
>           m1 = self.read()
>           md = treemanifest(dir=self._dir)
>           for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():
> @@ -1398,7 +1401,7 @@ class treemanifestctx(object):
>           return md
>   
>       def readfast(self):
> -        rl = self._revlog
> +        rl = self._revlog()
>           r = rl.rev(self._node)
>           deltaparent = rl.deltaparent(r)
>           if deltaparent != revlog.nullrev and deltaparent in rl.parentrevs(r):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DQIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=OIS_lb8Kz_3P14_jz49eK8NtZN8h_4KBUiwQwg-aGE0&s=ERR5QLZxx3JCSTEzVblfNLFdAGeJFkFj1ag-Hp79DaI&e=
via Mercurial-devel - Oct. 19, 2016, 11:45 p.m.
Pushed to committed repo, thanks!

On Tue, Oct 18, 2016 at 5:50 PM, Durham Goode <durham@fb.com> wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1476837882 25200
> #      Tue Oct 18 17:44:42 2016 -0700
> # Branch stable
> # Node ID d5bc7048144e6c9675134b85aadb9d7b69d406aa
> # Parent  3efece5c59853908d65de53635488361dbf20c49
> manifest: make treemanifestctx store the repo
>
> Same as in the last commit, the old treemanifestctx stored a reference to the
> revlog.  If the inmemory revlog became invalid, the ctx now held an old copy and
> would be incorrect. To fix this, we need the ctx to go through the manifestlog
> for each access.
>
> This is the same pattern that changectx already uses (it stores the repo, and
> accesses commit data through self._repo.changelog).
>
> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
> --- a/mercurial/manifest.py
> +++ b/mercurial/manifest.py
> @@ -1274,7 +1274,7 @@ class manifestlog(object):
>                  return cachemf
>
>          if self._treeinmem:
> -            m = treemanifestctx(self._revlog, '', node)
> +            m = treemanifestctx(self._repo, '', node)
>          else:
>              m = manifestctx(self._repo, node)
>          if node != revlog.nullid:
> @@ -1344,9 +1344,8 @@ class manifestctx(object):
>          return manifestdict(d)
>
>  class treemanifestctx(object):
> -    def __init__(self, revlog, dir, node):
> -        revlog = revlog.dirlog(dir)
> -        self._revlog = revlog
> +    def __init__(self, repo, dir, node):
> +        self._repo = repo
>          self._dir = dir
>          self._data = None
>
> @@ -1359,23 +1358,27 @@ class treemanifestctx(object):
>          #rev = revlog.rev(node)
>          #self.linkrev = revlog.linkrev(rev)
>
> +    def _revlog(self):
> +        return self._repo.manifestlog._revlog.dirlog(self._dir)
> +
>      def read(self):
>          if not self._data:
> +            rl = self._revlog()
>              if self._node == revlog.nullid:
>                  self._data = treemanifest()
> -            elif self._revlog._treeondisk:
> +            elif rl._treeondisk:
>                  m = treemanifest(dir=self._dir)
>                  def gettext():
> -                    return self._revlog.revision(self._node)
> +                    return rl.revision(self._node)
>                  def readsubtree(dir, subm):
> -                    return treemanifestctx(self._revlog, dir, subm).read()
> +                    return treemanifestctx(self._repo, dir, subm).read()
>                  m.read(gettext, readsubtree)
>                  m.setnode(self._node)
>                  self._data = m
>              else:
> -                text = self._revlog.revision(self._node)
> +                text = revlog.revision(self._node)
>                  arraytext = array.array('c', text)
> -                self._revlog.fulltextcache[self._node] = arraytext
> +                rl.fulltextcache[self._node] = arraytext
>                  self._data = treemanifest(dir=self._dir, text=text)
>
>          return self._data
> @@ -1385,9 +1388,9 @@ class treemanifestctx(object):
>
>      def readdelta(self):
>          # Need to perform a slow delta
> -        revlog = self._revlog
> +        revlog = self._revlog()
>          r0 = revlog.deltaparent(revlog.rev(self._node))
> -        m0 = treemanifestctx(revlog, self._dir, revlog.node(r0)).read()
> +        m0 = treemanifestctx(self._repo, self._dir, revlog.node(r0)).read()
>          m1 = self.read()
>          md = treemanifest(dir=self._dir)
>          for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():
> @@ -1398,7 +1401,7 @@ class treemanifestctx(object):
>          return md
>
>      def readfast(self):
> -        rl = self._revlog
> +        rl = self._revlog()
>          r = rl.rev(self._node)
>          deltaparent = rl.deltaparent(r)
>          if deltaparent != revlog.nullrev and deltaparent in rl.parentrevs(r):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Katsunori FUJIWARA - Oct. 20, 2016, 9:43 p.m.
At Tue, 18 Oct 2016 17:54:33 -0700,
Durham Goode wrote:
> 
> Foozy pointed this out to me at the sprint and I didn't manage to get it 
> done in time for the freeze.  This series fixes a pretty notable 
> regression from the old manifest code in that the manifestlog is being 
> recreated every time repo.manifestlog is called.

Thank you for completing this work!

> On 10/18/16 5:50 PM, Durham Goode wrote:
> > # HG changeset patch
> > # User Durham Goode <durham@fb.com>
> > # Date 1476837882 25200
> > #      Tue Oct 18 17:44:42 2016 -0700
> > # Branch stable
> > # Node ID d5bc7048144e6c9675134b85aadb9d7b69d406aa
> > # Parent  3efece5c59853908d65de53635488361dbf20c49
> > manifest: make treemanifestctx store the repo
> >
> > Same as in the last commit, the old treemanifestctx stored a reference to the
> > revlog.  If the inmemory revlog became invalid, the ctx now held an old copy and
> > would be incorrect. To fix this, we need the ctx to go through the manifestlog
> > for each access.
> >
> > This is the same pattern that changectx already uses (it stores the repo, and
> > accesses commit data through self._repo.changelog).
> >
> > diff --git a/mercurial/manifest.py b/mercurial/manifest.py
> > --- a/mercurial/manifest.py
> > +++ b/mercurial/manifest.py
> > @@ -1274,7 +1274,7 @@ class manifestlog(object):
> >                   return cachemf
> >   
> >           if self._treeinmem:
> > -            m = treemanifestctx(self._revlog, '', node)
> > +            m = treemanifestctx(self._repo, '', node)
> >           else:
> >               m = manifestctx(self._repo, node)
> >           if node != revlog.nullid:
> > @@ -1344,9 +1344,8 @@ class manifestctx(object):
> >           return manifestdict(d)
> >   
> >   class treemanifestctx(object):
> > -    def __init__(self, revlog, dir, node):
> > -        revlog = revlog.dirlog(dir)
> > -        self._revlog = revlog
> > +    def __init__(self, repo, dir, node):
> > +        self._repo = repo
> >           self._dir = dir
> >           self._data = None
> >   
> > @@ -1359,23 +1358,27 @@ class treemanifestctx(object):
> >           #rev = revlog.rev(node)
> >           #self.linkrev = revlog.linkrev(rev)
> >   
> > +    def _revlog(self):
> > +        return self._repo.manifestlog._revlog.dirlog(self._dir)
> > +
> >       def read(self):
> >           if not self._data:
> > +            rl = self._revlog()
> >               if self._node == revlog.nullid:
> >                   self._data = treemanifest()
> > -            elif self._revlog._treeondisk:
> > +            elif rl._treeondisk:
> >                   m = treemanifest(dir=self._dir)
> >                   def gettext():
> > -                    return self._revlog.revision(self._node)
> > +                    return rl.revision(self._node)
> >                   def readsubtree(dir, subm):
> > -                    return treemanifestctx(self._revlog, dir, subm).read()
> > +                    return treemanifestctx(self._repo, dir, subm).read()
> >                   m.read(gettext, readsubtree)
> >                   m.setnode(self._node)
> >                   self._data = m
> >               else:
> > -                text = self._revlog.revision(self._node)
> > +                text = revlog.revision(self._node)
> >                   arraytext = array.array('c', text)
> > -                self._revlog.fulltextcache[self._node] = arraytext
> > +                rl.fulltextcache[self._node] = arraytext
> >                   self._data = treemanifest(dir=self._dir, text=text)
> >   
> >           return self._data
> > @@ -1385,9 +1388,9 @@ class treemanifestctx(object):
> >   
> >       def readdelta(self):
> >           # Need to perform a slow delta
> > -        revlog = self._revlog
> > +        revlog = self._revlog()
> >           r0 = revlog.deltaparent(revlog.rev(self._node))
> > -        m0 = treemanifestctx(revlog, self._dir, revlog.node(r0)).read()
> > +        m0 = treemanifestctx(self._repo, self._dir, revlog.node(r0)).read()
> >           m1 = self.read()
> >           md = treemanifest(dir=self._dir)
> >           for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():
> > @@ -1398,7 +1401,7 @@ class treemanifestctx(object):
> >           return md
> >   
> >       def readfast(self):
> > -        rl = self._revlog
> > +        rl = self._revlog()
> >           r = rl.rev(self._node)
> >           deltaparent = rl.deltaparent(r)
> >           if deltaparent != revlog.nullrev and deltaparent in rl.parentrevs(r):
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DQIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=OIS_lb8Kz_3P14_jz49eK8NtZN8h_4KBUiwQwg-aGE0&s=ERR5QLZxx3JCSTEzVblfNLFdAGeJFkFj1ag-Hp79DaI&e=
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/mercurial/manifest.py b/mercurial/manifest.py
--- a/mercurial/manifest.py
+++ b/mercurial/manifest.py
@@ -1274,7 +1274,7 @@  class manifestlog(object):
                 return cachemf
 
         if self._treeinmem:
-            m = treemanifestctx(self._revlog, '', node)
+            m = treemanifestctx(self._repo, '', node)
         else:
             m = manifestctx(self._repo, node)
         if node != revlog.nullid:
@@ -1344,9 +1344,8 @@  class manifestctx(object):
         return manifestdict(d)
 
 class treemanifestctx(object):
-    def __init__(self, revlog, dir, node):
-        revlog = revlog.dirlog(dir)
-        self._revlog = revlog
+    def __init__(self, repo, dir, node):
+        self._repo = repo
         self._dir = dir
         self._data = None
 
@@ -1359,23 +1358,27 @@  class treemanifestctx(object):
         #rev = revlog.rev(node)
         #self.linkrev = revlog.linkrev(rev)
 
+    def _revlog(self):
+        return self._repo.manifestlog._revlog.dirlog(self._dir)
+
     def read(self):
         if not self._data:
+            rl = self._revlog()
             if self._node == revlog.nullid:
                 self._data = treemanifest()
-            elif self._revlog._treeondisk:
+            elif rl._treeondisk:
                 m = treemanifest(dir=self._dir)
                 def gettext():
-                    return self._revlog.revision(self._node)
+                    return rl.revision(self._node)
                 def readsubtree(dir, subm):
-                    return treemanifestctx(self._revlog, dir, subm).read()
+                    return treemanifestctx(self._repo, dir, subm).read()
                 m.read(gettext, readsubtree)
                 m.setnode(self._node)
                 self._data = m
             else:
-                text = self._revlog.revision(self._node)
+                text = revlog.revision(self._node)
                 arraytext = array.array('c', text)
-                self._revlog.fulltextcache[self._node] = arraytext
+                rl.fulltextcache[self._node] = arraytext
                 self._data = treemanifest(dir=self._dir, text=text)
 
         return self._data
@@ -1385,9 +1388,9 @@  class treemanifestctx(object):
 
     def readdelta(self):
         # Need to perform a slow delta
-        revlog = self._revlog
+        revlog = self._revlog()
         r0 = revlog.deltaparent(revlog.rev(self._node))
-        m0 = treemanifestctx(revlog, self._dir, revlog.node(r0)).read()
+        m0 = treemanifestctx(self._repo, self._dir, revlog.node(r0)).read()
         m1 = self.read()
         md = treemanifest(dir=self._dir)
         for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():
@@ -1398,7 +1401,7 @@  class treemanifestctx(object):
         return md
 
     def readfast(self):
-        rl = self._revlog
+        rl = self._revlog()
         r = rl.rev(self._node)
         deltaparent = rl.deltaparent(r)
         if deltaparent != revlog.nullrev and deltaparent in rl.parentrevs(r):