Sage: Ticket #12357: Make groupoids garbage collectable
https://trac.sagemath.org/ticket/12357
<p>
Currently, the groupoid of an object P can not be garbage collected, even when deleting P.
</p>
<p>
Even worse: The persistence of <code>Groupoid(P)</code> also prevents P from being garbage collected.
</p>
<p>
The attached patch aims at solving it: An external reference to either P or <code>Groupoid(P)</code> is enough to keep both alive. But without an external reference, both P and <code>Groupoid(P)</code> become collectable.
</p>
<p>
Example from the docs:
</p>
<pre class="wiki"> sage: P = GF(151)['x','y']
sage: n = id(Groupoid(P))
sage: import gc
sage: _ = gc.collect()
sage: n == id(Groupoid(P)) # indirect doctest
True
Thus, the groupoid is cached. But when deleting ``P``, its
groupoid can be garbage collected as well::
sage: del P
sage: _ = gc.collect()
sage: P = GF(151)['x','y']
sage: n == id(Groupoid(P))
False
TESTS:
We test against some corner cases::
sage: Groupoid(None)
Traceback (most recent call last):
...
TypeError: Groupoid of None is not defined
sage: Groupoid(1)
Traceback (most recent call last):
...
TypeError: 1 must either allow attribute assignment or be instances of <type 'sage.structure.parent.Parent'>
sage: class A: pass
sage: a = A()
sage: Groupoid(a)
Groupoid with underlying set <__main__.A instance at ...>
sage: Groupoid(a) is Groupoid(a)
True
</pre><p>
Apply:
</p>
<ul><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/12357/trac12357_internal_strong_groupoid_cache.patch" title="Attachment 'trac12357_internal_strong_groupoid_cache.patch' in Ticket #12357">trac12357_internal_strong_groupoid_cache.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/12357/trac12357_internal_strong_groupoid_cache.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/12357/trac12357_reviewer.patch" title="Attachment 'trac12357_reviewer.patch' in Ticket #12357">trac12357_reviewer.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/12357/trac12357_reviewer.patch" title="Download"></a>
</li></ul>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/12357
Trac 1.1.6SimonKingWed, 25 Jan 2012 12:34:07 GMTstatus changed
https://trac.sagemath.org/ticket/12357#comment:1
https://trac.sagemath.org/ticket/12357#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
<p>
I did not run the full test suite yet. It could be that some silly old doctests use the groupoids in a way they are not intended, such as <code>Groupoid(1)</code>. If this is the case, then my patch has to change. Otherwise: Needs review!
</p>
TicketSimonKingWed, 25 Jan 2012 19:10:19 GMT
https://trac.sagemath.org/ticket/12357#comment:2
https://trac.sagemath.org/ticket/12357#comment:2
<p>
To my surprise, there were no nasty old doctests. Hence, <code>make ptest</code> went well, with the patch from here and its dependencies!
</p>
TicketSimonKingWed, 25 Jan 2012 20:30:56 GMT
https://trac.sagemath.org/ticket/12357#comment:3
https://trac.sagemath.org/ticket/12357#comment:3
<p>
Concerning timings:
</p>
<p>
Without the patch, the groupoid of P is stored in a dictionary, indexed by P. Hence, access to the cache is slow if <code>hash(P)</code> is slow.
</p>
<p>
Some data points:
</p>
<pre class="wiki">sage: P = GF(151)['x','y']
sage: %timeit G = Groupoid(P) # slow hash
625 loops, best of 3: 20.9 µs per loop
sage: %timeit G = Groupoid(ZZ) # fast hash
625 loops, best of 3: 1.75 µs per loop
sage: class Bar(Parent): pass
....:
sage: P = Bar()
sage: %timeit G = Groupoid(P) # slow hash
625 loops, best of 3: 15.3 µs per loop
</pre><p>
But with the patch, it is stored as an attribute of P. Hence, it is slow if attribute access is slow. The point is that even slow attribute access is faster than slow hash, and slow attribute access is only little slower than fast hash:
</p>
<pre class="wiki">sage: P = GF(151)['x','y']
sage: %timeit G = Groupoid(P) # slow attribute access
625 loops, best of 3: 3.11 µs per loop
sage: %timeit G = Groupoid(ZZ) # slow attribute access
625 loops, best of 3: 3.1 µs per loop
sage: class Bar(Parent): pass
....:
sage: P = Bar()
sage: %timeit G = Groupoid(P) # fast attribute access
625 loops, best of 3: 1.59 µs per loop
</pre><p>
Hence, I believe that the patch is not only fixing a memory leak, but has the potential to generate a speed-up.
</p>
TicketjpfloriWed, 08 Feb 2012 17:19:44 GMTstatus changed; keywords, work_issues set
https://trac.sagemath.org/ticket/12357#comment:4
https://trac.sagemath.org/ticket/12357#comment:4
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
<li><strong>keywords</strong>
<em>groupoid</em> <em>cache</em> <em>Cernay2012</em> added
</li>
<li><strong>work_issues</strong>
set to <em>tests for None?</em>
</li>
</ul>
<p>
This is the simplest patch of the cache problems suite, and it does not depend on #715, so I begin with this one.
</p>
<p>
I'm a little bit confused by all those tests for None in your patch.
</p>
<p>
Isn't the first one sufficient?
</p>
<p>
More specifically, the second test "if S is not None" isn't superfluous ?
</p>
<p>
And if it is not None, can G be None ? (I guess there is some cython voodoo involved here)
</p>
<p>
To answer my own question, I guess it is none if S is not a Parent (as in your example with 1).
</p>
<p>
Once you answer me back, I'll post a reviewer patch with some additional minor corrections.
</p>
TicketSimonKingWed, 08 Feb 2012 18:19:53 GMTstatus changed
https://trac.sagemath.org/ticket/12357#comment:5
https://trac.sagemath.org/ticket/12357#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
Yes, the test "if S is not None" in line 103 seems redundant, as S being None would result in an error being raised in line 92.
</p>
<p>
Concerning Cython voodoo: If S is not None and not of type Parent, then <code>G == S</code> in line 106 would result in a type error (since G is cdefined) - which is desired behaviour. But if S were None, then <code>G == S</code> in line 106 would not complain, and then line 109 would segfault. This is why there must be a test for the corner case - of course, one test would be enough.
</p>
TicketjpfloriFri, 10 Feb 2012 12:38:17 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/12357#comment:6
https://trac.sagemath.org/ticket/12357#comment:6
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>work_issues</strong>
<em>tests for None?</em> deleted
</li>
</ul>
<p>
Ok, I've posted a reviewer patch removing the second test and adding some comments on the code.
</p>
<p>
Everything is fine for me (as long as further modification in <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> do not affect the ticket here).
</p>
<p>
So I'll put is as positive review now.
</p>
TicketjpfloriFri, 10 Feb 2012 12:38:48 GMTattachment set
https://trac.sagemath.org/ticket/12357
https://trac.sagemath.org/ticket/12357
<ul>
<li><strong>attachment</strong>
set to <em>trac12357_reviewer.patch</em>
</li>
</ul>
<p>
Reviewer patch
</p>
TicketSimonKingFri, 10 Feb 2012 16:40:30 GMT
https://trac.sagemath.org/ticket/12357#comment:7
https://trac.sagemath.org/ticket/12357#comment:7
<p>
The reviewer patch looks fine to me. Thank you!
</p>
TicketjdemeyerSat, 11 Feb 2012 09:43:20 GMTdependencies changed
https://trac.sagemath.org/ticket/12357#comment:8
https://trac.sagemath.org/ticket/12357#comment:8
<ul>
<li><strong>dependencies</strong>
changed from <em>#12313</em> to <em>#715, #12313</em>
</li>
</ul>
TicketjdemeyerSun, 12 Feb 2012 12:30:03 GMTmilestone changed
https://trac.sagemath.org/ticket/12357#comment:9
https://trac.sagemath.org/ticket/12357#comment:9
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.0</em> to <em>sage-pending</em>
</li>
</ul>
<p>
Moving this to sage-pending for now since it depends on two non-positively reviewed tickets.
</p>
TicketSimonKingFri, 09 Mar 2012 16:31:03 GMTdependencies changed; work_issues set
https://trac.sagemath.org/ticket/12357#comment:10
https://trac.sagemath.org/ticket/12357#comment:10
<ul>
<li><strong>dependencies</strong>
changed from <em>#715, #12313</em> to <em>#715, #12313, #11943</em>
</li>
<li><strong>work_issues</strong>
set to <em>rebase rel #11943</em>
</li>
</ul>
<p>
There is a conflict with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11943" title="enhancement: The category graph should comply with Python's method resolution order (closed: fixed)">#11943</a>.
</p>
<ul><li>The ticket here is pending anyway
</li><li><a class="closed ticket" href="https://trac.sagemath.org/ticket/11943" title="enhancement: The category graph should comply with Python's method resolution order (closed: fixed)">#11943</a> has only one dependency, that is already merged
</li><li><a class="closed ticket" href="https://trac.sagemath.org/ticket/11943" title="enhancement: The category graph should comply with Python's method resolution order (closed: fixed)">#11943</a> is more fundamental than the patch here.
</li></ul><p>
Therefore, I suggest to rebase my patch with respect to <a class="closed ticket" href="https://trac.sagemath.org/ticket/11943" title="enhancement: The category graph should comply with Python's method resolution order (closed: fixed)">#11943</a>.
</p>
TicketSimonKingFri, 09 Mar 2012 16:45:35 GMTdescription changed
https://trac.sagemath.org/ticket/12357#comment:11
https://trac.sagemath.org/ticket/12357#comment:11
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/12357?action=diff&version=11">diff</a>)
</li>
</ul>
<p>
Done!
</p>
<p>
Apply trac12357_internal_strong_groupoid_cache.patch trac12357_reviewer.patch
</p>
TicketSimonKingSun, 15 Apr 2012 13:44:55 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/12357#comment:12
https://trac.sagemath.org/ticket/12357#comment:12
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
<em>rebase rel #11943</em> deleted
</li>
</ul>
<p>
Something went wrong. Namely, originally, Jean-Pierre reviewed this patch, because it seemed independent of the other weak reference stuff. But meanwhile it depends on the other stuff.
</p>
<p>
Should one perhaps revert the dependencies? I.e., make it so that the patch here <em>only</em> depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/11943" title="enhancement: The category graph should comply with Python's method resolution order (closed: fixed)">#11943</a>, and then rebase <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> relative to it?
</p>
TicketSimonKingSun, 15 Apr 2012 15:11:06 GMTattachment set
https://trac.sagemath.org/ticket/12357
https://trac.sagemath.org/ticket/12357
<ul>
<li><strong>attachment</strong>
set to <em>trac12357_internal_strong_groupoid_cache.patch</em>
</li>
</ul>
<p>
Make groupoids garbage collectable, but still use a cache by strong references. Relative <a class="closed ticket" href="https://trac.sagemath.org/ticket/11943" title="enhancement: The category graph should comply with Python's method resolution order (closed: fixed)">#11943</a>
</p>
TicketSimonKingSun, 15 Apr 2012 15:14:55 GMT
https://trac.sagemath.org/ticket/12357#comment:13
https://trac.sagemath.org/ticket/12357#comment:13
<p>
I had a test that works like "Create an object and store its memory address; delete it, do garbage collection, create the object again and show that the memory address is different from the old".
</p>
<p>
Of course, that is very fragile. By chance, when applying a totally unrelated ticket, the test breaks for me.
</p>
<p>
Therefore I made a change in the first patch. The test now verifies that the stored memory address does not occur among the memory addresses of uncollectable objects, after garbage collection.
</p>
<p>
I did not change the dependencies, yet. But perhaps the reviewer has an opinion on that matter?
</p>
<p>
Apply trac12357_internal_strong_groupoid_cache.patch trac12357_reviewer.patch
</p>
TicketSimonKingSun, 15 Apr 2012 15:15:12 GMTstatus changed
https://trac.sagemath.org/ticket/12357#comment:14
https://trac.sagemath.org/ticket/12357#comment:14
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketjpfloriTue, 17 Apr 2012 09:10:14 GMT
https://trac.sagemath.org/ticket/12357#comment:15
https://trac.sagemath.org/ticket/12357#comment:15
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12357#comment:12" title="Comment 12">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Something went wrong. Namely, originally, Jean-Pierre reviewed this patch, because it seemed independent of the other weak reference stuff. But meanwhile it depends on the other stuff.
</p>
</blockquote>
<p>
You're right.
</p>
<blockquote class="citation">
<p>
Should one perhaps revert the dependencies? I.e., make it so that the patch here <em>only</em> depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/11943" title="enhancement: The category graph should comply with Python's method resolution order (closed: fixed)">#11943</a>, and then rebase <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> relative to it?
</p>
</blockquote>
<p>
The correct behavior here depends on the application of the three previous tickets, am I right ?
So I think its ok to have the three of them as dependencies (as there is no conflict to apply all of them).
</p>
TicketjpfloriTue, 17 Apr 2012 11:25:42 GMTstatus, dependencies changed; reviewer set
https://trac.sagemath.org/ticket/12357#comment:16
https://trac.sagemath.org/ticket/12357#comment:16
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Jean-Pierre Flori</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#715, #12313, #11943</em> to <em>#715, #11521, #12313, #11943</em>
</li>
</ul>
<p>
Apart from my above remark, everything is fine on my install of beta13 plus <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11943" title="enhancement: The category graph should comply with Python's method resolution order (closed: fixed)">#11943</a> and this ticket.
</p>
<p>
The modified test is ok too.
</p>
<p>
From my point of view <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> (with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a>) and <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> are needed to make the ticket here work, but the converse is not true.
And from a practical point of view, <a class="closed ticket" href="https://trac.sagemath.org/ticket/11943" title="enhancement: The category graph should comply with Python's method resolution order (closed: fixed)">#11943</a> plus the ticket here can be applied before or after <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> without conflicts.
</p>
<p>
So the dependencies look fine to me as they are now, I'm just adding <a class="closed ticket" href="https://trac.sagemath.org/ticket/11521" title="defect: Use weak references to cache homsets (closed: fixed)">#11521</a> because it now goes with <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a>.
I may also have missed something.
</p>
<p>
Rebasing everything to make all of that going the other way around seems to me to be a waste of time.
</p>
TicketjpfloriTue, 17 Apr 2012 11:31:24 GMT
https://trac.sagemath.org/ticket/12357#comment:17
https://trac.sagemath.org/ticket/12357#comment:17
<p>
I've put the ticket back to positive review because I feel that <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> will get positive review as well.
</p>
TicketjdemeyerThu, 26 Apr 2012 22:36:45 GMTmilestone changed
https://trac.sagemath.org/ticket/12357#comment:18
https://trac.sagemath.org/ticket/12357#comment:18
<ul>
<li><strong>milestone</strong>
changed from <em>sage-pending</em> to <em>sage-5.1</em>
</li>
</ul>
TicketjdemeyerTue, 15 May 2012 06:47:13 GMTmilestone changed
https://trac.sagemath.org/ticket/12357#comment:19
https://trac.sagemath.org/ticket/12357#comment:19
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.1</em> to <em>sage-pending</em>
</li>
</ul>
TicketjdemeyerFri, 15 Jun 2012 09:07:04 GMTstatus changed
https://trac.sagemath.org/ticket/12357#comment:20
https://trac.sagemath.org/ticket/12357#comment:20
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Unfortunalely, this regularly (not always but quite often) gives Segmentation Faults in <code>sage/schemes/elliptic_curves/gal_reps.py</code> <em>after</em> the file has been tested:
</p>
<pre class="wiki">$ ./sage -t --verbose "devel/sage/sage/schemes/elliptic_curves/gal_reps.py"
[...]
4 items had no tests:
__main__
__main__.change_warning_output
__main__.check_with_tolerance
__main__.warning_function
25 items passed all tests:
26 tests in __main__.example_0
[...]
10 tests in __main__.example_9
263 tests in 29 items.
263 passed and 0 failed.
Test passed.
Segmentation fault
[21.5 s]
----------------------------------------------------------------------
The following tests failed:
sage -t --verbose "devel/sage/sage/schemes/elliptic_curves/gal_reps.py"
Total time for all tests: 21.6 seconds
</pre><p>
This is with sage-5.1.beta4 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/12357" title="defect: Make groupoids garbage collectable (closed: duplicate)">#12357</a> (beta4 is not yet released, but essentially finished).
</p>
TicketSimonKingFri, 18 Jan 2013 13:13:02 GMT
https://trac.sagemath.org/ticket/12357#comment:21
https://trac.sagemath.org/ticket/12357#comment:21
<p>
Jeroen, does the segmentation fault persists, now that <a class="closed ticket" href="https://trac.sagemath.org/ticket/715" title="defect: Parents probably not reclaimed due to too much caching (closed: fixed)">#715</a> seems fine due to Cython upgrade?
</p>
<p>
Anyway, I am now running make ptest with MALLOCK_CHECK_=3 on sage-5.6.rc0 debug version (<a class="closed ticket" href="https://trac.sagemath.org/ticket/13864" title="task: Configure Python with pydebug when SAGE_DEBUG is set (closed: fixed)">#13864</a>) plus <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/12357" title="defect: Make groupoids garbage collectable (closed: duplicate)">#12357</a>.
</p>
TicketSimonKingFri, 18 Jan 2013 13:39:49 GMTstatus changed
https://trac.sagemath.org/ticket/12357#comment:22
https://trac.sagemath.org/ticket/12357#comment:22
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Patchbot, apply trac12357_internal_strong_groupoid_cache.patch <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/12357/trac12357_reviewer.patch" title="Attachment 'trac12357_reviewer.patch' in Ticket #12357">attachment:trac12357_reviewer.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/12357/trac12357_reviewer.patch" title="Download"></a>
</p>
TicketSimonKingFri, 18 Jan 2013 15:49:48 GMTstatus changed
https://trac.sagemath.org/ticket/12357#comment:23
https://trac.sagemath.org/ticket/12357#comment:23
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I got one doctest error, which unfortunately shows that the patch does not solve the problem it was created for:
</p>
<pre class="wiki">File "/home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/sage/categories/groupoid.pyx", line 64:
sage: n in map(id, gc.get_objects())
Expected:
False
Got:
True
</pre><p>
Hence, some garbage that was supposed to get collected did not become collected. And there also seems to be a timeout in
</p>
<pre class="wiki">sage -t -force_lib "devel/sage/doc/en/bordeaux_2008/birds_other.rst"
</pre>
TicketSimonKingFri, 18 Jan 2013 15:54:33 GMT
https://trac.sagemath.org/ticket/12357#comment:24
https://trac.sagemath.org/ticket/12357#comment:24
<p>
In fact, the polynomial ring in the failing example does not become collected, even <em>without</em> creating its groupoid. I thought that this would have been fixed in one of the dependencies?
</p>
<pre class="wiki">sage: P = GF(151)['x','y']
sage: import gc
sage: m = id(P)
sage: del P
sage: _ = gc.collect()
sage: m in map(id, gc.get_objects())
True
</pre>
TicketSimonKingFri, 18 Jan 2013 16:03:02 GMT
https://trac.sagemath.org/ticket/12357#comment:25
https://trac.sagemath.org/ticket/12357#comment:25
<p>
Perhaps I should use a test that avoids polynomial rings, but uses a custom <code>UniqueRepresentation</code>. Namely, <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a> makes <code>UniqueRepresentation</code> collectable, and the patch from here makes the groupoid collectable as well, namely:
</p>
<pre class="wiki">sage: class MyParent(UniqueRepresentation, Parent):
....: pass
....:
sage: P = MyParent()
sage: import gc
sage: m = id(P)
sage: n = id(Groupoid(P))
sage: m in map(id, gc.get_objects())
True
sage: n in map(id, gc.get_objects())
True
sage: del P
sage: _ = gc.collect()
sage: m in map(id, gc.get_objects())
False
sage: n in map(id, gc.get_objects())
False
</pre><p>
What do you think?
</p>
<p>
By the way, the timeout is no problem: I had closed my laptop during the tests. Hence, the tests were interrupted, but the time was running.
</p>
TicketSimonKingFri, 18 Jan 2013 16:05:36 GMT
https://trac.sagemath.org/ticket/12357#comment:26
https://trac.sagemath.org/ticket/12357#comment:26
<p>
PS: In unpatched sage-5.6.rc0, one would obtain
</p>
<pre class="wiki">sage: P = Parent()
sage: import gc
sage: m = id(P)
sage: n = id(Groupoid(P))
sage: m in map(id, gc.get_objects())
True
sage: n in map(id, gc.get_objects())
True
sage: del P
sage: _ = gc.collect()
sage: m in map(id, gc.get_objects())
True
sage: n in map(id, gc.get_objects())
True
</pre><p>
And this is even <em>without</em> using a cache for P!!
</p>
<p>
Hence, the tests does demonstrate that some leak was fixed.
</p>
TicketSimonKingFri, 18 Jan 2013 17:07:25 GMT
https://trac.sagemath.org/ticket/12357#comment:27
https://trac.sagemath.org/ticket/12357#comment:27
<p>
Apart from changing the test, I'd like to change something more. Namely, it is stated that <code>UniqueRepresentation</code> is using cached_function - but that is wrong by <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a>, which is a dependency of this ticket.
</p>
<p>
The main idea of storing the groupoid of P as an attribute of P is to avoid creating an external strong reference on the groupoid. But that would not be the case, when using a weak_cached_function for caching.
</p>
<p>
Hence, it could actually be that <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a> already fixes what is attempted here! I'll test that.
</p>
TicketSimonKingFri, 18 Jan 2013 17:12:52 GMTstatus, milestone changed
https://trac.sagemath.org/ticket/12357#comment:28
https://trac.sagemath.org/ticket/12357#comment:28
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-pending</em> to <em>sage-duplicate/invalid/wontfix</em>
</li>
</ul>
<p>
Indeed. With just <a class="closed ticket" href="https://trac.sagemath.org/ticket/12215" title="defect: Memleak in UniqueRepresentation, @cached_method (closed: fixed)">#12215</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/12313" title="defect: Fix yet another memory leak caused by caching of coercion data (closed: fixed)">#12313</a> on top of sage-5.6.rc0, one obtains:
</p>
<pre class="wiki">sage: P = Parent()
sage: m = id(P)
sage: n = id(Groupoid(P))
sage: import gc
sage: m in map(id, gc.get_objects())
True
sage: n in map(id, gc.get_objects())
True
sage: del P
sage: _ = gc.collect()
sage: m in map(id, gc.get_objects())
False
sage: n in map(id, gc.get_objects())
False
</pre><p>
Hence, I suggest to resolve this as a duplicate. Jean-Pierre, please change if you disagree.
</p>
TicketjdemeyerThu, 28 Feb 2013 11:02:28 GMTstatus, reviewer, description changed; resolution set; author deleted
https://trac.sagemath.org/ticket/12357#comment:29
https://trac.sagemath.org/ticket/12357#comment:29
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>reviewer</strong>
changed from <em>Jean-Pierre Flori</em> to <em>Simon King, Jean-Pierre Flori</em>
</li>
<li><strong>resolution</strong>
set to <em>duplicate</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/12357?action=diff&version=29">diff</a>)
</li>
<li><strong>author</strong>
<em>Simon King</em> deleted
</li>
</ul>
Ticket