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

[Sean Farley]

FUJIWARA Katsunori <foozy at lares.dti.ne.jp> writes:

> At Sun, 11 Jun 2017 15:20:25 -0700,
> Sean Farley wrote:
>> 
>> FUJIWARA Katsunori <foozy at lares.dti.ne.jp> writes:
>> 
>> > At Sat, 10 Jun 2017 16:01:23 -0700,
>> > Sean Farley wrote:
>> >> 
>> >> # HG changeset patch
>> >> # User Sean Farley <sean at 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!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170611/0346193d/attachment.asc>