[PATCH 3 of 3 v2] repoview: cache repoview class instances (issue5043)

[Yuya Nishihara]

On Mon, 18 Jan 2016 09:42:10 +0000, Simon King wrote:
> > >>>> On 01/17/2016 02:18 PM, Gregory Szorc wrote:
> > >>>> # HG changeset patch
> > >>>> # User Gregory Szorc <gregory.szorc at gmail.com>
> > >>>> # Date 1453069024 28800
> > >>>> #      Sun Jan 17 14:17:04 2016 -0800
> > >>>> # Node ID 3a418d20ca9d0345416d65b82fd3f49101bcc5b2
> > >>>> # Parent  91969eee20a1dcd52470258164bc44528861a4c6
> > >>>> repoview: cache repoview class instances (issue5043)
> > >>>>
> > >>>> The Python garbage collector was reporting the proxycls instances
> > >>>> created on every invocation of localrepository.filtered() were
> > >>>> uncollectable. This led to leaking memory.
> > >>>>
> > >>>> We fix the leak by introducing a cache of the proxy classes. After
> > >>>> this patch, the garbage collector no longer reports uncollectable
> > >>>> objects after each iteration of `hg convert`. This likely prevents
> > >>>> memory leaks in a number of other operations as well (pretty much
> > >>>> anything relying heavily on repo.filtered() calls).
> > >>>>
> > >>>> We stop short of caching the instantiated proxycls instances, which
> > >>>> we /should/ be able to do safely. This feels a bit too risky for
> > >>>> a change just before code freeze, however.
> > >>>
> > >>> I'm either missing something or the behavior of this patch is
> > incorrect.
> > >>>
> > >>> If the class of the repo objet changes, we won't invalidate the cache,
> > and the next call to "filtered" will return an object with an outdated
> > proxy class.
> > >>
> > >> That's a valid observation. But the class should be static after
> > reposetup functions are called. And we shouldn't be obtaining a repoview
> > before reposetup is called, right?
> > >
> > > I would not bet on class being static (and I'am pretty sure it is not).

Can't it be a cache keyed by repo.__class__ ?

> > > What's make the class object uncollectable, could we not fix this
> > instead?
> >
> > I, uh, have no idea. Either classes aren't garbage collected or the ref to
> > repo.__class__ is somehow making it not collectible. This is a minor leak
> > compared to the first 2 patches. So if you could queue those, it would be
> > appreciated.
> >
> >
> I think classes *are* garbage collected, but not as soon as the obvious
> reference is dropped (presumably there's a reference cycle):

Perhaps you are right.

import gc
from mercurial import hg, repoview, ui as uimod

ui = uimod.ui()
repo = hg.repository(ui)
for x in xrange(100):
    #gc.collect()
    repo.unfiltered().filtered('visible')
    print len(repoview.repoview.__subclasses__())