Patchwork [memctx-cache,V2] memctx: always use cache for filectxfn

login
register
mail settings
Submitter Sean Farley
Date June 12, 2017, 1:20 a.m.
Message ID <df6f0855d09ede751ec9.1497230426@1.0.0.127.in-addr.arpa>
Download mbox | patch
Permalink /patch/21337/
State Accepted
Headers show

Comments

Sean Farley - June 12, 2017, 1:20 a.m.
# HG changeset patch
# User Sean Farley <sean@farley.io>
# Date 1497135618 25200
#      Sat Jun 10 16:00:18 2017 -0700
# Branch memctx-cache
# Node ID df6f0855d09ede751ec95e98fa840291f03c69e9
# Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
memctx: always use cache for filectxfn

I don't see a downside to doing this unless I'm missing something.
Thanks to foozy for correcting my previous bad logic.
Katsunori FUJIWARA - June 12, 2017, 7:52 a.m.
At Sun, 11 Jun 2017 18:20:26 -0700,
Sean Farley wrote:
> 
> # HG changeset patch
> # User Sean Farley <sean@farley.io>
> # Date 1497135618 25200
> #      Sat Jun 10 16:00:18 2017 -0700
> # Branch memctx-cache
> # Node ID df6f0855d09ede751ec95e98fa840291f03c69e9
> # Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
> memctx: always use cache for filectxfn
> 
> I don't see a downside to doing this unless I'm missing something.
> Thanks to foozy for correcting my previous bad logic.

LGTM :-)

> diff --git a/mercurial/context.py b/mercurial/context.py
> index 7ce34af..818f173 100644
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -2104,17 +2104,17 @@ class memctx(committablectx):
>          if branch is not None:
>              self._extra['branch'] = encoding.fromlocal(branch)
>          self.substate = {}
>  
>          if isinstance(filectxfn, patch.filestore):
> -            self._filectxfn = memfilefrompatch(filectxfn)
> +            filectxfn = memfilefrompatch(filectxfn)
>          elif not callable(filectxfn):
>              # if store is not callable, wrap it in a function
> -            self._filectxfn = memfilefromctx(filectxfn)
> -        else:
> -            # memoizing increases performance for e.g. vcs convert scenarios.
> -            self._filectxfn = makecachingfilectxfn(filectxfn)
> +            filectxfn = memfilefromctx(filectxfn)
> +
> +        # memoizing increases performance for e.g. vcs convert scenarios.
> +        self._filectxfn = makecachingfilectxfn(filectxfn)
>  
>          if editor:
>              self._text = editor(self._repo, self, [])
>              self._repo.savecommitmessage(self._text)
>  
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - June 12, 2017, 1:01 p.m.
On Mon, 12 Jun 2017 16:52:36 +0900, FUJIWARA Katsunori wrote:
> At Sun, 11 Jun 2017 18:20:26 -0700,
> Sean Farley wrote:
> > 
> > # HG changeset patch
> > # User Sean Farley <sean@farley.io>
> > # Date 1497135618 25200
> > #      Sat Jun 10 16:00:18 2017 -0700
> > # Branch memctx-cache
> > # Node ID df6f0855d09ede751ec95e98fa840291f03c69e9
> > # Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
> > memctx: always use cache for filectxfn
> > 
> > I don't see a downside to doing this unless I'm missing something.
> > Thanks to foozy for correcting my previous bad logic.
> 
> LGTM :-)

Yeah. Queued per review, thanks.

> > diff --git a/mercurial/context.py b/mercurial/context.py
> > index 7ce34af..818f173 100644
> > --- a/mercurial/context.py
> > +++ b/mercurial/context.py
> > @@ -2104,17 +2104,17 @@ class memctx(committablectx):
> >          if branch is not None:
> >              self._extra['branch'] = encoding.fromlocal(branch)
> >          self.substate = {}
> >  
> >          if isinstance(filectxfn, patch.filestore):
> > -            self._filectxfn = memfilefrompatch(filectxfn)
> > +            filectxfn = memfilefrompatch(filectxfn)
> >          elif not callable(filectxfn):
> >              # if store is not callable, wrap it in a function
> > -            self._filectxfn = memfilefromctx(filectxfn)

One possible downside would be there might be a cache leak by e.g. ctx->p1/p2
chain, but I couldn't come up with a real example.
Sean Farley - June 12, 2017, 9:58 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> On Mon, 12 Jun 2017 16:52:36 +0900, FUJIWARA Katsunori wrote:
>> At Sun, 11 Jun 2017 18:20:26 -0700,
>> Sean Farley wrote:
>> > 
>> > # HG changeset patch
>> > # User Sean Farley <sean@farley.io>
>> > # Date 1497135618 25200
>> > #      Sat Jun 10 16:00:18 2017 -0700
>> > # Branch memctx-cache
>> > # Node ID df6f0855d09ede751ec95e98fa840291f03c69e9
>> > # Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
>> > memctx: always use cache for filectxfn
>> > 
>> > I don't see a downside to doing this unless I'm missing something.
>> > Thanks to foozy for correcting my previous bad logic.
>> 
>> LGTM :-)
>
> Yeah. Queued per review, thanks.
>
>> > diff --git a/mercurial/context.py b/mercurial/context.py
>> > index 7ce34af..818f173 100644
>> > --- a/mercurial/context.py
>> > +++ b/mercurial/context.py
>> > @@ -2104,17 +2104,17 @@ class memctx(committablectx):
>> >          if branch is not None:
>> >              self._extra['branch'] = encoding.fromlocal(branch)
>> >          self.substate = {}
>> >  
>> >          if isinstance(filectxfn, patch.filestore):
>> > -            self._filectxfn = memfilefrompatch(filectxfn)
>> > +            filectxfn = memfilefrompatch(filectxfn)
>> >          elif not callable(filectxfn):
>> >              # if store is not callable, wrap it in a function
>> > -            self._filectxfn = memfilefromctx(filectxfn)
>
> One possible downside would be there might be a cache leak by e.g. ctx->p1/p2
> chain, but I couldn't come up with a real example.

My guess is that Phil and I might find something while doing the
in-memory merge stuff. But same as you: nothing concrete yet (and it's
easy enough to back out if we need to).

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
index 7ce34af..818f173 100644
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -2104,17 +2104,17 @@  class memctx(committablectx):
         if branch is not None:
             self._extra['branch'] = encoding.fromlocal(branch)
         self.substate = {}
 
         if isinstance(filectxfn, patch.filestore):
-            self._filectxfn = memfilefrompatch(filectxfn)
+            filectxfn = memfilefrompatch(filectxfn)
         elif not callable(filectxfn):
             # if store is not callable, wrap it in a function
-            self._filectxfn = memfilefromctx(filectxfn)
-        else:
-            # memoizing increases performance for e.g. vcs convert scenarios.
-            self._filectxfn = makecachingfilectxfn(filectxfn)
+            filectxfn = memfilefromctx(filectxfn)
+
+        # memoizing increases performance for e.g. vcs convert scenarios.
+        self._filectxfn = makecachingfilectxfn(filectxfn)
 
         if editor:
             self._text = editor(self._repo, self, [])
             self._repo.savecommitmessage(self._text)