[PATCH] localrepo: clear the filecache on _rollback() and destroyed()

[Idan Kamara]

On Tue, Jul 31, 2012 at 4:14 AM, Greg Ward <greg at gerg.ca> wrote:
>
> [me]
> > It's unclear what you're fixing here.
>
> [Idan Kamara]
> > I'm fixing this: http://selenic.com/repo/hg/rev/357e6bcfb619 which
> > basically disabled
> > the filecache.
>
> So, this is a performance fix? And that's why there is no change to
> the tests? Or did 357e6bcfb619 affect correctness?

This is a performance fix. 357e6bcfb619 fixed a bug by disabling the
whole mechanism.

>
> [back to the patch]
> > # HG changeset patch
> > # User Idan Kamara <idankk86 at gmail.com>
> > # Date 1343504430 -10800
> > # Branch stable
> > # Node ID b4aefebff45caa66ca253c43a673b5847a2b84a6
> > # Parent  a09cc6aeed4a6d54a3846b6653165fd3b328d1ef
> > localrepo: clear the filecache on _rollback() and destroyed()
> >
> > This restores the old behaviour of clearing the filecache when the repo
> > is
> > destroyed but combines it with also clearing it on _rollback. Before, we
> > tried
> > to only call it through _rollback but that ruined callers of destroyed.
> >
> > Doing it on both code paths covers destroyed being called from somewhere
> > else, e.g. strip.
>
> You're essentially backing out 357e6bcfb619 here, so IMHO you should
> mention it in the commit message. From Matt's commit message there, it
> sounds like *he* was fixing something, so you're in danger of getting
> into patch ping-pong. Nobody wins that game.
>
> OK, I have reviewed the last couple of patches in this area
> (ce0ad184f489, 9a99224a6409, 357e6bcfb619), and I *think* your patch
> is correct, although it's too bad that we have to call
> self._filecache.clear() from two disparate places. (And I can see that
> you and Matt are already playing patch ping-pong. Good luck with
> that.)

Frankly, this whole thing is pretty complicated and even I'm not 100%
that I really understand the underlying problem. I've looked at it several
times but never went all the way inside.

I think this patch is safe because it restores the original behavior (that
wasn't failing mq tests) and combines it with the fix that was made for
_rollback.

I'll probably want to come back to this sometime in the future.

>
> But I can just see some enterprising young developer coming along and
> "cleaning things up", causing a performance regression -- again. (I'm
> still assuming all the bugs here are performance, not correctness.)
>
> So... is there any way to test it?

Adding something to perf.py should be possible.

(thanks for going over this btw)

>
>        Greg
> --
> Greg Ward                                http://www.gerg.ca/
> Know thyself.  If you need help, call the CIA.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20120731/203fc650/attachment.html>