Patchwork [3,of,3] localrepo: cache types for filtered repos (issue5043)

login
register
mail settings
Submitter Gregory Szorc
Date May 27, 2017, 12:42 a.m.
Message ID <1755f6e0f866afa6bbc2.1495845756@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/20956/
State Accepted
Headers show

Comments

Gregory Szorc - May 27, 2017, 12:42 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1495845557 25200
#      Fri May 26 17:39:17 2017 -0700
# Node ID 1755f6e0f866afa6bbc20b688fab2995f4bca566
# Parent  9a7f60cb77b6969608d2a9b72a2ccb31a53db82b
localrepo: cache types for filtered repos (issue5043)

For reasons I can't explain, localrepository.filtered() leaks the
dynamically created type. As far as I can tell, this is due to a
cycle between the dynamically generated type and its .__bases__
tuple. This /should/ get garbage collected once all references
to both objects are in the garbage list. However, when running
`hg convert` with garbage collection debugging enabled,
objgraph.show_backrefs() shows all references outside of gc.garbage
are in fact gone. However, these objects aren't being collected!
I spent a few hours trying to track this down and didn't have any
luck. But I'm 100% confident we're leaking the dymaically generated
type and 2 tuples of types on every revision that `hg convert`
processes.

Unable to stop the leak, the next best thing is to contain it.

This patch adds a cache of of the dynamically generated repoview/filter
types on the localrepo object. Since we only generate each type
once, we cap the amount of memory that can leak to something
reasonable.

As the inline comment explains, there is danger to caching the type
if the __class__ of the repo is updated after first call. Code to
validate this doesn't happen has been added. It is possible this may
break some extensions. If it does, I think they deserve to break,
as mutating the __class__ of a repo is dangerous and should be done
as early as possible and only in reposetup().

After this change, `hg convert` no longer leaks memory on every
revision. The process will likely grow memory usage over time due
to e.g. larger manifests. But there are no leaks.
Jun Wu - May 27, 2017, 3:55 a.m.
The reason of leaking is CLASS.__mro__ has a reference cycle. That could be
detected by my leak detection tool [2], or see comment at [1].

I'm very sad knowing the fact that __mro__ is an instant cycle in Python 2
world given the fact we use that pattern frequently to wrap ui or repo
objects. The best way I can think of to break the cycle without having to
use Python 3 is to write some low-level CPython code and I'm not sure
whether that will work yet.

[1]: http://bugs.python.org/msg60985
[2]: https://patchwork.mercurial-scm.org/patch/20886/

Excerpts from Gregory Szorc's message of 2017-05-26 17:42:36 -0700:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1495845557 25200
> #      Fri May 26 17:39:17 2017 -0700
> # Node ID 1755f6e0f866afa6bbc20b688fab2995f4bca566
> # Parent  9a7f60cb77b6969608d2a9b72a2ccb31a53db82b
> localrepo: cache types for filtered repos (issue5043)
> 
> For reasons I can't explain, localrepository.filtered() leaks the
> dynamically created type. As far as I can tell, this is due to a
> cycle between the dynamically generated type and its .__bases__
> tuple. This /should/ get garbage collected once all references
> to both objects are in the garbage list. However, when running
> `hg convert` with garbage collection debugging enabled,
> objgraph.show_backrefs() shows all references outside of gc.garbage
> are in fact gone. However, these objects aren't being collected!
> I spent a few hours trying to track this down and didn't have any
> luck. But I'm 100% confident we're leaking the dymaically generated
> type and 2 tuples of types on every revision that `hg convert`
> processes.
> 
> Unable to stop the leak, the next best thing is to contain it.
> 
> This patch adds a cache of of the dynamically generated repoview/filter
> types on the localrepo object. Since we only generate each type
> once, we cap the amount of memory that can leak to something
> reasonable.
> 
> As the inline comment explains, there is danger to caching the type
> if the __class__ of the repo is updated after first call. Code to
> validate this doesn't happen has been added. It is possible this may
> break some extensions. If it does, I think they deserve to break,
> as mutating the __class__ of a repo is dangerous and should be done
> as early as possible and only in reposetup().
> 
> After this change, `hg convert` no longer leaks memory on every
> revision. The process will likely grow memory usage over time due
> to e.g. larger manifests. But there are no leaks.
> 
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -396,6 +396,10 @@ class localrepository(object):
>          # - bookmark changes
>          self.filteredrevcache = {}
>  
> +        # Cache of types representing filtered repos.
> +        self._filteredexpectedbasetype = None
> +        self._filteredrepotypes = {}
> +
>          # generic mapping between names and nodes
>          self.names = namespaces.namespaces()
>  
> @@ -493,11 +497,31 @@ class localrepository(object):
>  
>      def filtered(self, name):
>          """Return a filtered version of a repository"""
> +        # The dynamically created types can easily leak. To curtail this,
> +        # we cache the types so we're not always creating them. The main
> +        # risk to caching is that the __class__ of the original repo
> +        # changes after caching. This should never happen, as __class__
> +        # adjustment should only be performed in reposetup() functions
> +        # in extensions *before* any kind of operation that accesses
> +        # repoviews. So we check for this explicitly and error when
> +        # extensions are misbehaving.
> +        unfiltered = self.unfiltered()
> +
> +        if self._filteredexpectedbasetype is None:
> +            self._filteredexpectedbasetype = unfiltered.__class__
> +        else:
> +            if self._filteredexpectedbasetype != unfiltered.__class__:
> +                raise error.ProgrammingError('__class__ of repo modified after '
> +                                             'call to filtered()')
> +
>          # build a new class with the mixin and the current class
>          # (possibly subclass of the repo)
> -        bases = (repoview.repoview, self.unfiltered().__class__)
> -        cls = type('%sfilteredrepo' % name, bases, {})
> -        return cls(self, name)
> +        if name not in self._filteredrepotypes:
> +            bases = (repoview.repoview, unfiltered.__class__)
> +            cls = type('%sfilteredrepo' % name, bases, {})
> +            self._filteredrepotypes[name] = cls
> +
> +        return self._filteredrepotypes[name](self, name)
>  
>      @repofilecache('bookmarks', 'bookmarks.current')
>      def _bookmarks(self):
> diff --git a/mercurial/statichttprepo.py b/mercurial/statichttprepo.py
> --- a/mercurial/statichttprepo.py
> +++ b/mercurial/statichttprepo.py
> @@ -164,6 +164,9 @@ class statichttprepository(localrepo.loc
>          self.encodepats = None
>          self.decodepats = None
>          self._transref = None
> +        # Cache of types representing filtered repos.
> +        self._filteredexpectedbasetype = None
> +        self._filteredrepotypes = {}
>  
>      def _restrictcapabilities(self, caps):
>          caps = super(statichttprepository, self)._restrictcapabilities(caps)
Jun Wu - May 27, 2017, 8:59 a.m.
Patch 1 looks good to me.

Note: Working around Python class cycle bug [1] is hard and I have given up.
I think it's safe to assume every class ever defined won't be freed by
refcount in Python 2. So reducing the number of classes seems good.

[1]: http://bugs.python.org/issue17950

Excerpts from Gregory Szorc's message of 2017-05-26 17:42:36 -0700:
> [...] 
> As the inline comment explains, there is danger to caching the type
> if the __class__ of the repo is updated after first call. Code to
> validate this doesn't happen has been added. It is possible this may
> break some extensions. If it does, I think they deserve to break,
> as mutating the __class__ of a repo is dangerous and should be done
> as early as possible and only in reposetup().

Mutating __class__  is a common pattern in Mercurial world. wrapfunction
docstring explicitly recommends this pattern. I don't think we should break
extensions here.

> [...]
> +        # Cache of types representing filtered repos.
> +        self._filteredexpectedbasetype = None
> +        self._filteredrepotypes = {}

(also to patch 2): Different filters could reuse a same filteredrepo class
to reduce class count (since they cannot be freed). Maybe we can implement
__repr__ to make debugging easier.

We can use self.unfiltered().__class__ as the cache keys. Since class won't
be freed, it does not matter if we add new references. And that solves the
correctness issue.

> [...]
>          self.names = namespaces.namespaces()
>  
> @@ -493,11 +497,31 @@ class localrepository(object):
>  
>      def filtered(self, name):

Maybe something like this:

          key = self._unfiltered().__class__
          if key not in self._filteredrepotypes:
              class filteredrepo(repoview.repoview, key):
                  pass
              self._filteredrepotypes[key] = filteredrepo
          return self._filteredrepotypes[key](self, name)

> [...]
Pierre-Yves David - May 27, 2017, 11:28 a.m.
On 05/27/2017 10:59 AM, Jun Wu wrote:
> Patch 1 looks good to me.
>
> Note: Working around Python class cycle bug [1] is hard and I have given up.
> I think it's safe to assume every class ever defined won't be freed by
> refcount in Python 2. So reducing the number of classes seems good.
>
> [1]: http://bugs.python.org/issue17950
>
> Excerpts from Gregory Szorc's message of 2017-05-26 17:42:36 -0700:
>> [...]
>> As the inline comment explains, there is danger to caching the type
>> if the __class__ of the repo is updated after first call. Code to
>> validate this doesn't happen has been added. It is possible this may
>> break some extensions. If it does, I think they deserve to break,
>> as mutating the __class__ of a repo is dangerous and should be done
>> as early as possible and only in reposetup().
>
> Mutating __class__  is a common pattern in Mercurial world. wrapfunction
> docstring explicitly recommends this pattern. I don't think we should break
> extensions here.

+1, patch 3 as it is is a bit scary. I think jun proposed solution 
(using source class as cache key) is a good one.

>> [...]
>> +        # Cache of types representing filtered repos.
>> +        self._filteredexpectedbasetype = None
>> +        self._filteredrepotypes = {}
>
> (also to patch 2): Different filters could reuse a same filteredrepo class
> to reduce class count (since they cannot be freed). Maybe we can implement
> __repr__ to make debugging easier.
>
> We can use self.unfiltered().__class__ as the cache keys. Since class won't
> be freed, it does not matter if we add new references. And that solves the
> correctness issue.
>
>> [...]
>>          self.names = namespaces.namespaces()
>>
>> @@ -493,11 +497,31 @@ class localrepository(object):
>>
>>      def filtered(self, name):
>
> Maybe something like this:
>
>           key = self._unfiltered().__class__
>           if key not in self._filteredrepotypes:
>               class filteredrepo(repoview.repoview, key):
>                   pass
>               self._filteredrepotypes[key] = filteredrepo
>           return self._filteredrepotypes[key](self, name)
>
>> [...]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - May 31, 2017, 1:11 p.m.
On Sat, 27 May 2017 13:28:43 +0200, Pierre-Yves David wrote:
> 
> 
> On 05/27/2017 10:59 AM, Jun Wu wrote:
> > Patch 1 looks good to me.
> >
> > Note: Working around Python class cycle bug [1] is hard and I have given up.
> > I think it's safe to assume every class ever defined won't be freed by
> > refcount in Python 2. So reducing the number of classes seems good.
> >
> > [1]: http://bugs.python.org/issue17950
> >
> > Excerpts from Gregory Szorc's message of 2017-05-26 17:42:36 -0700:
> >> [...]
> >> As the inline comment explains, there is danger to caching the type
> >> if the __class__ of the repo is updated after first call. Code to
> >> validate this doesn't happen has been added. It is possible this may
> >> break some extensions. If it does, I think they deserve to break,
> >> as mutating the __class__ of a repo is dangerous and should be done
> >> as early as possible and only in reposetup().
> >
> > Mutating __class__  is a common pattern in Mercurial world. wrapfunction
> > docstring explicitly recommends this pattern. I don't think we should break
> > extensions here.
> 
> +1, patch 3 as it is is a bit scary. I think jun proposed solution 
> (using source class as cache key) is a good one.

+1. I do that in TortoiseHg for caching changectx class wrappers. If we're
afraid of possible leak because of the class cache, we could use
WeakKeyDictionary.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -396,6 +396,10 @@  class localrepository(object):
         # - bookmark changes
         self.filteredrevcache = {}
 
+        # Cache of types representing filtered repos.
+        self._filteredexpectedbasetype = None
+        self._filteredrepotypes = {}
+
         # generic mapping between names and nodes
         self.names = namespaces.namespaces()
 
@@ -493,11 +497,31 @@  class localrepository(object):
 
     def filtered(self, name):
         """Return a filtered version of a repository"""
+        # The dynamically created types can easily leak. To curtail this,
+        # we cache the types so we're not always creating them. The main
+        # risk to caching is that the __class__ of the original repo
+        # changes after caching. This should never happen, as __class__
+        # adjustment should only be performed in reposetup() functions
+        # in extensions *before* any kind of operation that accesses
+        # repoviews. So we check for this explicitly and error when
+        # extensions are misbehaving.
+        unfiltered = self.unfiltered()
+
+        if self._filteredexpectedbasetype is None:
+            self._filteredexpectedbasetype = unfiltered.__class__
+        else:
+            if self._filteredexpectedbasetype != unfiltered.__class__:
+                raise error.ProgrammingError('__class__ of repo modified after '
+                                             'call to filtered()')
+
         # build a new class with the mixin and the current class
         # (possibly subclass of the repo)
-        bases = (repoview.repoview, self.unfiltered().__class__)
-        cls = type('%sfilteredrepo' % name, bases, {})
-        return cls(self, name)
+        if name not in self._filteredrepotypes:
+            bases = (repoview.repoview, unfiltered.__class__)
+            cls = type('%sfilteredrepo' % name, bases, {})
+            self._filteredrepotypes[name] = cls
+
+        return self._filteredrepotypes[name](self, name)
 
     @repofilecache('bookmarks', 'bookmarks.current')
     def _bookmarks(self):
diff --git a/mercurial/statichttprepo.py b/mercurial/statichttprepo.py
--- a/mercurial/statichttprepo.py
+++ b/mercurial/statichttprepo.py
@@ -164,6 +164,9 @@  class statichttprepository(localrepo.loc
         self.encodepats = None
         self.decodepats = None
         self._transref = None
+        # Cache of types representing filtered repos.
+        self._filteredexpectedbasetype = None
+        self._filteredrepotypes = {}
 
     def _restrictcapabilities(self, caps):
         caps = super(statichttprepository, self)._restrictcapabilities(caps)