#12215: Memleak in UniqueRepresentation, @cached_method
-----------------------+----------------------------------------------------
Reporter: vbraun | Owner: rlm
Type: defect | Status: needs_review
Priority: major | Milestone: sage-5.0
Component: memleak | Keywords: UniqueRepresentation cached_method
caching
Work_issues: | Upstream: N/A
Reviewer: | Author: Simon King
Merged: | Dependencies: #11115 #11900
-----------------------+----------------------------------------------------
Old description:
> The documentation says that UniqueRepresentation uses weak refs, but this
> was switched over to the @cached_method decorator. The latter does
> currently use strong references, so unused unique parents stay in memory
> forever:
> {{{
> import sage.structure.unique_representation
> len(sage.structure.unique_representation.UniqueRepresentation.__classcall__.cache)
>
> for i in range(2,1000):
> ring = ZZ.quotient(ZZ(i))
> vectorspace = ring^2
>
> import gc
> gc.collect()
> len(sage.structure.unique_representation.UniqueRepresentation.__classcall__.cache)
> }}}
> Related tickets:
> * #11521 (needs review, introducing weak references for caching
> homsets), and
> * #715 (using weak references for caching coerce maps).
> * #5970 (the polynomial rings cache use strong references, which may
> now be a duplicate, as I introduce the weak cache in #715)
>
> Further notes:
> * not everything in Python can be weakref'ed, for example ``None``
> cannot.
> * some results that are expensive to compute should not just be cached
> by a weak reference. Perhaps there is place for a permanent cache, or
> maybe some minimal age before garbage collecting it.
New description:
The documentation says that UniqueRepresentation uses weak refs, but this
was switched over to the @cached_method decorator. The latter does
currently use strong references, so unused unique parents stay in memory
forever:
{{{
import sage.structure.unique_representation
len(sage.structure.unique_representation.UniqueRepresentation.__classcall__.cache)
for i in range(2,1000):
ring = ZZ.quotient(ZZ(i))
vectorspace = ring^2
import gc
gc.collect()
len(sage.structure.unique_representation.UniqueRepresentation.__classcall__.cache)
}}}
Related tickets:
* #11521 (needs review, introducing weak references for caching
homsets), and
* #715 (needs review, using weak references for caching coerce maps).
* #5970 (the polynomial rings cache use strong references, which may now
be a duplicate, as I introduce the weak cache in #715)
Further notes:
* not everything in Python can be weakref'ed, for example ``None``
cannot.
* some results that are expensive to compute should not just be cached
by a weak reference. Perhaps there is place for a permanent cache, or
maybe some minimal age before garbage collecting it.
__Apply__
* [attachment:trac12215_weak_cached_function.patch]
* [attachment:trac12215_segfault_fixes.patch]
--
Comment(by SimonKing):
I have updated the second patch, which was about fixing segfaults anyway.
As I already stated: I find it weird that the problem is solved by
''incrementing'' the reference count of the domain and codomain of an
action when the action is deallocated. But it works, i.e., the doctests
that used to segfault with #12313 and the old version of the patches run
fine with the new patch version.
I need an expert opinion, though, and the full test suite is also to be
run.
Concerning memleaks, here is the example from the ticket description.
With #12313 and the patches from here:
{{{
sage: import sage.structure.unique_representation
sage:
len(sage.structure.unique_representation.UniqueRepresentation.__classcall__.cache)
135
sage:
sage: for i in range(2,1000):
....: ring = ZZ.quotient(ZZ(i))
....: vectorspace = ring^2
....:
sage: import gc
sage: gc.collect()
16641
sage:
len(sage.structure.unique_representation.UniqueRepresentation.__classcall__.cache)
227
}}}
With #12313 only:
{{{
sage: import sage.structure.unique_representation
sage:
len(sage.structure.unique_representation.UniqueRepresentation.__classcall__.cache)
151
sage:
sage: for i in range(2,1000):
....: ring = ZZ.quotient(ZZ(i))
....: vectorspace = ring^2
....:
sage: import gc
sage: gc.collect()
3805
sage:
len(sage.structure.unique_representation.UniqueRepresentation.__classcall__.cache)
5142
}}}
So, it is a clear progress, and IIRC the patches comprise tests against at
least one memory leak that is fixed. Needs review!
Apply trac12215_weak_cached_function.patch trac12215_segfault_fixes.patch
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/12215#comment:60>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
