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