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

FUJIWARA Katsunori foozy at lares.dti.ne.jp
Mon Jun 12 00:06:37 UTC 2017


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.


> 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.


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



More information about the Mercurial-devel mailing list