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

login
register
mail settings
Submitter Sean Farley
Date June 10, 2017, 11:01 p.m.
Message ID <12f2faa6019f497ba089.1497135683@1.0.0.127.in-addr.arpa>
Download mbox | patch
Permalink /patch/21323/
State Superseded
Headers show

Comments

Sean Farley - June 10, 2017, 11:01 p.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 12f2faa6019f497ba08960564cfceff80250e75a
# Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
memctx: always use cache for filectxfn

I don't see a downside to doing this unless I'm missing something.
Katsunori FUJIWARA - June 11, 2017, 9:53 p.m.
At Sat, 10 Jun 2017 16:01:23 -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 12f2faa6019f497ba08960564cfceff80250e75a
> # Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
> memctx: always use cache for filectxfn
> 
> I don't see a downside to doing this unless I'm missing something.
> 
> diff --git a/mercurial/context.py b/mercurial/context.py
> index 7ce34af..348c13c 100644
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -2103,18 +2103,19 @@ class memctx(committablectx):
>          self._files = files
>          if branch is not None:
>              self._extra['branch'] = encoding.fromlocal(branch)
>          self.substate = {}
>  
> +        self._filectxfn = filectxfn
>          if isinstance(filectxfn, patch.filestore):
>              self._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)
> +
> +        # memoizing increases performance for e.g. vcs convert scenarios.
> +        self._filectxfn = makecachingfilectxfn(filectxfn)

This change seems to cache also results of memfilefrompatch() and
makecachingfilectxfn(). If so, should makecachingfilectxfn() be
applied on self._filectxfn ?  Or:

================
         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)
>  
>
Sean Farley - June 11, 2017, 10:20 p.m.
FUJIWARA Katsunori <foozy@lares.dti.ne.jp> writes:

> At Sat, 10 Jun 2017 16:01:23 -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 12f2faa6019f497ba08960564cfceff80250e75a
>> # Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
>> memctx: always use cache for filectxfn
>> 
>> I don't see a downside to doing this unless I'm missing something.
>> 
>> diff --git a/mercurial/context.py b/mercurial/context.py
>> index 7ce34af..348c13c 100644
>> --- a/mercurial/context.py
>> +++ b/mercurial/context.py
>> @@ -2103,18 +2103,19 @@ class memctx(committablectx):
>>          self._files = files
>>          if branch is not None:
>>              self._extra['branch'] = encoding.fromlocal(branch)
>>          self.substate = {}
>>  
>> +        self._filectxfn = filectxfn
>>          if isinstance(filectxfn, patch.filestore):
>>              self._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)
>> +
>> +        # memoizing increases performance for e.g. vcs convert scenarios.
>> +        self._filectxfn = makecachingfilectxfn(filectxfn)
>
> This change seems to cache also results of memfilefrompatch() and
> makecachingfilectxfn(). If so, should makecachingfilectxfn() be
> applied on self._filectxfn ?  Or:
>
> ================
>          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:

The third type of object that can be sent is a function that takes
(repo, memctx, path), so we can't get rid of the 'else:' like this. My
main question in this patch is: can we cache everything from filectxfn?
If not, that's fine (and I would personally document the reason in the
code).
Katsunori FUJIWARA - June 12, 2017, 12:06 a.m.
At Sun, 11 Jun 2017 15:20:25 -0700,
Sean Farley wrote:
> 
> FUJIWARA Katsunori <foozy@lares.dti.ne.jp> writes:
> 
> > At Sat, 10 Jun 2017 16:01:23 -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 12f2faa6019f497ba08960564cfceff80250e75a
> >> # Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
> >> memctx: always use cache for filectxfn
> >> 
> >> I don't see a downside to doing this unless I'm missing something.
> >> 
> >> diff --git a/mercurial/context.py b/mercurial/context.py
> >> index 7ce34af..348c13c 100644
> >> --- a/mercurial/context.py
> >> +++ b/mercurial/context.py
> >> @@ -2103,18 +2103,19 @@ class memctx(committablectx):
> >>          self._files = files
> >>          if branch is not None:
> >>              self._extra['branch'] = encoding.fromlocal(branch)
> >>          self.substate = {}
> >>  
> >> +        self._filectxfn = filectxfn
> >>          if isinstance(filectxfn, patch.filestore):
> >>              self._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)
> >> +
> >> +        # memoizing increases performance for e.g. vcs convert scenarios.
> >> +        self._filectxfn = makecachingfilectxfn(filectxfn)
> >
> > This change seems to cache also results of memfilefrompatch() and
> > makecachingfilectxfn(). If so, should makecachingfilectxfn() be
> > applied on self._filectxfn ?  Or:
> >
> > ================
> >          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:
> 
> The third type of object that can be sent is a function that takes
> (repo, memctx, path), so we can't get rid of the 'else:' like this.

With your change, wrapping original "filectxfn" by memfilefrompatch()
and memfilefromctx() is meaningless (and initial "self._filectxfn =
filectxfn" in your change, too), because previous "self._filectxfn" (=
wrapping result) is always overwritten by result of
makecachingfilectxfn() with original "filectxfn" argument.

"self._filectxfn = makecachingfilectxfn(self._filectxfn)" seems OK, IMHO.


> My
> main question in this patch is: can we cache everything from filectxfn?
> If not, that's fine (and I would personally document the reason in the
> code).

I think that results of memctx._filectxfn() can be always cached, too.
Sean Farley - June 12, 2017, 12:29 a.m.
FUJIWARA Katsunori <foozy@lares.dti.ne.jp> writes:

> At Sun, 11 Jun 2017 15:20:25 -0700,
> Sean Farley wrote:
>> 
>> FUJIWARA Katsunori <foozy@lares.dti.ne.jp> writes:
>> 
>> > At Sat, 10 Jun 2017 16:01:23 -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 12f2faa6019f497ba08960564cfceff80250e75a
>> >> # Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
>> >> memctx: always use cache for filectxfn
>> >> 
>> >> I don't see a downside to doing this unless I'm missing something.
>> >> 
>> >> diff --git a/mercurial/context.py b/mercurial/context.py
>> >> index 7ce34af..348c13c 100644
>> >> --- a/mercurial/context.py
>> >> +++ b/mercurial/context.py
>> >> @@ -2103,18 +2103,19 @@ class memctx(committablectx):
>> >>          self._files = files
>> >>          if branch is not None:
>> >>              self._extra['branch'] = encoding.fromlocal(branch)
>> >>          self.substate = {}
>> >>  
>> >> +        self._filectxfn = filectxfn
>> >>          if isinstance(filectxfn, patch.filestore):
>> >>              self._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)
>> >> +
>> >> +        # memoizing increases performance for e.g. vcs convert scenarios.
>> >> +        self._filectxfn = makecachingfilectxfn(filectxfn)
>> >
>> > This change seems to cache also results of memfilefrompatch() and
>> > makecachingfilectxfn(). If so, should makecachingfilectxfn() be
>> > applied on self._filectxfn ?  Or:
>> >
>> > ================
>> >          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:
>> 
>> The third type of object that can be sent is a function that takes
>> (repo, memctx, path), so we can't get rid of the 'else:' like this.
>
> With your change, wrapping original "filectxfn" by memfilefrompatch()
> and memfilefromctx() is meaningless (and initial "self._filectxfn =
> filectxfn" in your change, too), because previous "self._filectxfn" (=
> wrapping result) is always overwritten by result of
> makecachingfilectxfn() with original "filectxfn" argument.
>
> "self._filectxfn = makecachingfilectxfn(self._filectxfn)" seems OK, IMHO.

Oh, I see. Ok, I'll rework the patch.

>> My
>> main question in this patch is: can we cache everything from filectxfn?
>> If not, that's fine (and I would personally document the reason in the
>> code).
>
> I think that results of memctx._filectxfn() can be always cached, too.

Cool, thanks for the feedback!
Sean Farley - June 12, 2017, 1:17 a.m.
Sean Farley <sean@farley.io> writes:

> FUJIWARA Katsunori <foozy@lares.dti.ne.jp> writes:
>
>> At Sun, 11 Jun 2017 15:20:25 -0700,
>> Sean Farley wrote:
>>> 
>>> FUJIWARA Katsunori <foozy@lares.dti.ne.jp> writes:
>>> 
>>> > At Sat, 10 Jun 2017 16:01:23 -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 12f2faa6019f497ba08960564cfceff80250e75a
>>> >> # Parent  7e9d0d8ff938dcf8ca193c17db5321a05a48e718
>>> >> memctx: always use cache for filectxfn
>>> >> 
>>> >> I don't see a downside to doing this unless I'm missing something.
>>> >> 
>>> >> diff --git a/mercurial/context.py b/mercurial/context.py
>>> >> index 7ce34af..348c13c 100644
>>> >> --- a/mercurial/context.py
>>> >> +++ b/mercurial/context.py
>>> >> @@ -2103,18 +2103,19 @@ class memctx(committablectx):
>>> >>          self._files = files
>>> >>          if branch is not None:
>>> >>              self._extra['branch'] = encoding.fromlocal(branch)
>>> >>          self.substate = {}
>>> >>  
>>> >> +        self._filectxfn = filectxfn
>>> >>          if isinstance(filectxfn, patch.filestore):
>>> >>              self._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)
>>> >> +
>>> >> +        # memoizing increases performance for e.g. vcs convert scenarios.
>>> >> +        self._filectxfn = makecachingfilectxfn(filectxfn)
>>> >
>>> > This change seems to cache also results of memfilefrompatch() and
>>> > makecachingfilectxfn(). If so, should makecachingfilectxfn() be
>>> > applied on self._filectxfn ?  Or:
>>> >
>>> > ================
>>> >          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:
>>> 
>>> The third type of object that can be sent is a function that takes
>>> (repo, memctx, path), so we can't get rid of the 'else:' like this.
>>
>> With your change, wrapping original "filectxfn" by memfilefrompatch()
>> and memfilefromctx() is meaningless (and initial "self._filectxfn =
>> filectxfn" in your change, too), because previous "self._filectxfn" (=
>> wrapping result) is always overwritten by result of
>> makecachingfilectxfn() with original "filectxfn" argument.
>>
>> "self._filectxfn = makecachingfilectxfn(self._filectxfn)" seems OK, IMHO.
>
> Oh, I see. Ok, I'll rework the patch.

Dammit, I'm an idiot. I had a typo in my patch and didn't realize it.
Foozy, you're right as always.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
index 7ce34af..348c13c 100644
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -2103,18 +2103,19 @@  class memctx(committablectx):
         self._files = files
         if branch is not None:
             self._extra['branch'] = encoding.fromlocal(branch)
         self.substate = {}
 
+        self._filectxfn = filectxfn
         if isinstance(filectxfn, patch.filestore):
             self._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)
+
+        # 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)