Patchwork manifest: move manifestctx creation into manifestlog.get()

login
register
mail settings
Submitter Durham Goode
Date Nov. 17, 2016, 11:36 p.m.
Message ID <916fbbd3d9a628dcd4a6.1479425808@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/17632/
State Accepted
Headers show

Comments

Durham Goode - Nov. 17, 2016, 11:36 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1479425479 28800
#      Thu Nov 17 15:31:19 2016 -0800
# Node ID 916fbbd3d9a628dcd4a63c352d4d70b678e92a68
# Parent  c27614f2dec1405db606d1ef871dfabf72cc0737
manifest: move manifestctx creation into manifestlog.get()

Most manifestctx creation already happened in manifestlog.get(), but there was
one spot in the manifestctx class itself that created an instance manually. This
patch makes that one instance go through the manifestlog. This means extensions
can just wrap manifestlog.get() and it will cover all manifestctx creations. It
also means this code path now hits the manifestlog cache.
via Mercurial-devel - Nov. 18, 2016, 5:47 p.m.
And of course that wasn't just for Durham.

---------- Forwarded message ----------
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Fri, Nov 18, 2016 at 9:46 AM
Subject: Re: [PATCH] manifest: move manifestctx creation into manifestlog.get()
To: Durham Goode <durham@fb.com>


On Thu, Nov 17, 2016 at 3:36 PM, Durham Goode <durham@fb.com> wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1479425479 28800
> #      Thu Nov 17 15:31:19 2016 -0800
> # Node ID 916fbbd3d9a628dcd4a63c352d4d70b678e92a68
> # Parent  c27614f2dec1405db606d1ef871dfabf72cc0737
> manifest: move manifestctx creation into manifestlog.get()
>
> Most manifestctx creation already happened in manifestlog.get(), but there was
> one spot in the manifestctx class itself that created an instance manually. This
> patch makes that one instance go through the manifestlog. This means extensions
> can just wrap manifestlog.get() and it will cover all manifestctx creations. It
> also means this code path now hits the manifestlog cache.
>
> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
> --- a/mercurial/manifest.py
> +++ b/mercurial/manifest.py
> @@ -1418,7 +1418,7 @@ class manifestctx(object):
>          if revlog._usemanifestv2:

Looks like the below is only used with manifestv2, which no one is
using and no one will probably be using.

That reminded me to update the wiki for manfestv2. Please take a look
at the "Space savings" section of
https://www.mercurial-scm.org/wiki/ManifestV2Plan and see how it's a
actually a space increase. I reported that a long time ago, but redid
the tests the other week (repo conversion took a week, I think) and
finally updated the wiki. Let me know if you (anyone) think we should
still keep the experimental manifestv2 code in there.

>              # Need to perform a slow delta
>              r0 = revlog.deltaparent(revlog.rev(self._node))
> -            m0 = manifestctx(self._repo, revlog.node(r0)).read()
> +            m0 = self._repo.manifestlog[revlog.node(r0)].read()
>              m1 = self.read()
>              md = manifestdict()
>              for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Durham Goode - Nov. 18, 2016, 6:56 p.m.
I’m fine with getting rid of manifestv2.  In the meantime, it’d be good to get this patch in since it’s the last ctx constructor outside of manifestlog.get().

On 11/18/16, 9:47 AM, "Martin von Zweigbergk" <martinvonz@google.com> wrote:

    And of course that wasn't just for Durham.
    
    ---------- Forwarded message ----------
    From: Martin von Zweigbergk <martinvonz@google.com>

    Date: Fri, Nov 18, 2016 at 9:46 AM
    Subject: Re: [PATCH] manifest: move manifestctx creation into manifestlog.get()
    To: Durham Goode <durham@fb.com>
    
    
    On Thu, Nov 17, 2016 at 3:36 PM, Durham Goode <durham@fb.com> wrote:
    > # HG changeset patch

    > # User Durham Goode <durham@fb.com>

    > # Date 1479425479 28800

    > #      Thu Nov 17 15:31:19 2016 -0800

    > # Node ID 916fbbd3d9a628dcd4a63c352d4d70b678e92a68

    > # Parent  c27614f2dec1405db606d1ef871dfabf72cc0737

    > manifest: move manifestctx creation into manifestlog.get()

    >

    > Most manifestctx creation already happened in manifestlog.get(), but there was

    > one spot in the manifestctx class itself that created an instance manually. This

    > patch makes that one instance go through the manifestlog. This means extensions

    > can just wrap manifestlog.get() and it will cover all manifestctx creations. It

    > also means this code path now hits the manifestlog cache.

    >

    > diff --git a/mercurial/manifest.py b/mercurial/manifest.py

    > --- a/mercurial/manifest.py

    > +++ b/mercurial/manifest.py

    > @@ -1418,7 +1418,7 @@ class manifestctx(object):

    >          if revlog._usemanifestv2:

    
    Looks like the below is only used with manifestv2, which no one is
    using and no one will probably be using.
    
    That reminded me to update the wiki for manfestv2. Please take a look
    at the "Space savings" section of
    https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_wiki_ManifestV2Plan&d=DgIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=UGs0X2NyCTmbnJsMiQ_o3RWDnXgfqn3Y7J0uRnz-5p4&s=wvTFQc3KF1krxTB5k8hm9PkVcrvIWEANNelYw2mKouE&e=  and see how it's a
    actually a space increase. I reported that a long time ago, but redid
    the tests the other week (repo conversion took a week, I think) and
    finally updated the wiki. Let me know if you (anyone) think we should
    still keep the experimental manifestv2 code in there.
    
    >              # Need to perform a slow delta

    >              r0 = revlog.deltaparent(revlog.rev(self._node))

    > -            m0 = manifestctx(self._repo, revlog.node(r0)).read()

    > +            m0 = self._repo.manifestlog[revlog.node(r0)].read()

    >              m1 = self.read()

    >              md = manifestdict()

    >              for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():

    > _______________________________________________

    > 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=DgIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=UGs0X2NyCTmbnJsMiQ_o3RWDnXgfqn3Y7J0uRnz-5p4&s=2fnnUnE_7o6jsxXbyA5zjQqTK9blzFt57ry0FTJq1Pc&e=
via Mercurial-devel - Nov. 18, 2016, 7 p.m.
On Fri, Nov 18, 2016 at 10:56 AM, Durham Goode <durham@fb.com> wrote:
> I’m fine with getting rid of manifestv2.  In the meantime, it’d be good to get this patch in since it’s the last ctx constructor outside of manifestlog.get().

Oh, did I forget to say that I queued it? Looks like I did :-) Thanks!

>
> On 11/18/16, 9:47 AM, "Martin von Zweigbergk" <martinvonz@google.com> wrote:
>
>     And of course that wasn't just for Durham.
>
>     ---------- Forwarded message ----------
>     From: Martin von Zweigbergk <martinvonz@google.com>
>     Date: Fri, Nov 18, 2016 at 9:46 AM
>     Subject: Re: [PATCH] manifest: move manifestctx creation into manifestlog.get()
>     To: Durham Goode <durham@fb.com>
>
>
>     On Thu, Nov 17, 2016 at 3:36 PM, Durham Goode <durham@fb.com> wrote:
>     > # HG changeset patch
>     > # User Durham Goode <durham@fb.com>
>     > # Date 1479425479 28800
>     > #      Thu Nov 17 15:31:19 2016 -0800
>     > # Node ID 916fbbd3d9a628dcd4a63c352d4d70b678e92a68
>     > # Parent  c27614f2dec1405db606d1ef871dfabf72cc0737
>     > manifest: move manifestctx creation into manifestlog.get()
>     >
>     > Most manifestctx creation already happened in manifestlog.get(), but there was
>     > one spot in the manifestctx class itself that created an instance manually. This
>     > patch makes that one instance go through the manifestlog. This means extensions
>     > can just wrap manifestlog.get() and it will cover all manifestctx creations. It
>     > also means this code path now hits the manifestlog cache.
>     >
>     > diff --git a/mercurial/manifest.py b/mercurial/manifest.py
>     > --- a/mercurial/manifest.py
>     > +++ b/mercurial/manifest.py
>     > @@ -1418,7 +1418,7 @@ class manifestctx(object):
>     >          if revlog._usemanifestv2:
>
>     Looks like the below is only used with manifestv2, which no one is
>     using and no one will probably be using.
>
>     That reminded me to update the wiki for manfestv2. Please take a look
>     at the "Space savings" section of
>     https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_wiki_ManifestV2Plan&d=DgIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=UGs0X2NyCTmbnJsMiQ_o3RWDnXgfqn3Y7J0uRnz-5p4&s=wvTFQc3KF1krxTB5k8hm9PkVcrvIWEANNelYw2mKouE&e=  and see how it's a
>     actually a space increase. I reported that a long time ago, but redid
>     the tests the other week (repo conversion took a week, I think) and
>     finally updated the wiki. Let me know if you (anyone) think we should
>     still keep the experimental manifestv2 code in there.
>
>     >              # Need to perform a slow delta
>     >              r0 = revlog.deltaparent(revlog.rev(self._node))
>     > -            m0 = manifestctx(self._repo, revlog.node(r0)).read()
>     > +            m0 = self._repo.manifestlog[revlog.node(r0)].read()
>     >              m1 = self.read()
>     >              md = manifestdict()
>     >              for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():
>     > _______________________________________________
>     > 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=DgIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=UGs0X2NyCTmbnJsMiQ_o3RWDnXgfqn3Y7J0uRnz-5p4&s=2fnnUnE_7o6jsxXbyA5zjQqTK9blzFt57ry0FTJq1Pc&e=
>
>

Patch

diff --git a/mercurial/manifest.py b/mercurial/manifest.py
--- a/mercurial/manifest.py
+++ b/mercurial/manifest.py
@@ -1418,7 +1418,7 @@  class manifestctx(object):
         if revlog._usemanifestv2:
             # Need to perform a slow delta
             r0 = revlog.deltaparent(revlog.rev(self._node))
-            m0 = manifestctx(self._repo, revlog.node(r0)).read()
+            m0 = self._repo.manifestlog[revlog.node(r0)].read()
             m1 = self.read()
             md = manifestdict()
             for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():