-
-
Notifications
You must be signed in to change notification settings - Fork 528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Memleak in UniqueRepresentation, @cached_method #12215
Comments
This comment has been minimized.
This comment has been minimized.
comment:3
See my comment at #5970: It seems that having a weak version of cached_function (which is used to decorate I think this should be done on top of #11115, which rewrites cached methods and already has a positive review. |
Dependencies: #11115 |
comment:5
Here is a patch. It isn't tested yet. |
comment:6
... and I immediately updated the patch: Join categories were not using unique representation but cached_function (by #11900). So, that had to change. |
comment:7
Sorry, it was impossible to use weak_cached_function on the join function in sage.categories.category, since it may return a list (not weakly referenceable). Hence, I had to work around. With the attached patch (applied on top of #11900 and its dependencies), sage at least starts... |
comment:8
It turns out that all the patches can still not fix the problem. We also have to deal with I suggest to add an option to The applications of Coercion sucks. |
comment:9
It turns out that It doesn't solve the problem, though. |
comment:10
I have slightly updated my patch, so that there is no conflict with #11935. |
comment:11
There is yet another location where it makes sense to use Namely, dynamic classes are frequently used in the category framework, they have a strong cache, and the parent/element classes keep a pointer to the category they belong to. So, that's preventing categories from being garbage collected. I think that my patches from here, #715, and #11935 (which reduces the number of dynamic classes created) might actually be enough to fix the problem. When I run
then one initially still sees an increased memory usage. But after a while it seems to stabilise. |
This comment has been minimized.
This comment has been minimized.
comment:12
I have updated the patch. It documents the changes, and at least the tests in sage/misc/cachefunc.pyx, in sage/categories/..., in sage/rings/... and in sage/structure/unique_representation.py pass. Hence, needs review! |
Author: Simon King |
This comment has been minimized.
This comment has been minimized.
Work Issues: segfaults for elliptic curves |
comment:15
While the tests in sage/categories, sage/rings and sage/structure/unique_representation.py pass, I get some segfaults for the elliptic curve tests. Thus, needs work. |
comment:16
I did
Strange. |
comment:17
I think I found the problem. Some doctest of the form
segfaults. But when the result is assigned to a variable, like this
then everything works. Is it perhaps the case that garbage collection of the residue field (that was enabled by my patch) happens between the creation and the computation of the string representation of the object? But that is strange. There are variables |
comment:18
It isn't ready for review, yet, because of the segfaults. |
comment:19
Some old code is not using the cache: There was some coerce map created in sage/rings/residue_field.pyx, whose parent was not created by Changing it fixed at least one segfault. I wish all segfaults would go away so easily... |
comment:20
Fortunately, I now have a short example that triggers a memory access error when leaving Sage:
However, I wonder how I can trigger the error without leaving Sage, and how I can trace what is going on. |
comment:149
Fortunately I can confirm it (at least with MALLOC_CHECK_=3). I'm running it now under gdb. |
comment:150
What I get is:
I see a couple of familiar names in the backtrace... |
Crash log |
comment:151
Attachment: sage_crash_WgD9iG.log Thanks to Volker's enhanced backtraces, running the test in verbose mode and without gdb yields this backtrace, and the crash occurs here (line 6467 of heegner.py)
Unfortunately, running this in an interactive session works just fine. |
comment:152
Got it, I think. The crash happens in the last line of the following snippet
and according to the crash log, we have
Hence, again, we have the problem that some attributes have already become invalid. I think this was fixed by the second patch from #12313. Suggestion: In order to keep things modular, the part of the second #12313 patch that applies to Rationale: #12313 has a problem with a time regression, while #12215 should (hopefully) be fine after installing the fix. |
Safer callback in |
comment:153
Attachment: trac12215_safe_callback.patch.gz The patch's up, and it fixes the crash in heegner.py (tested in sage-5.6.rc0 debug version with MALLOC_CHECK_=3) Apply trac12215_weak_cached_function_combined.patch trac12215_safe_callback.patch |
This comment has been minimized.
This comment has been minimized.
comment:154
Yes, this solves the problem here as well, so positive review. It looks like the analysis on #12313:226 and the patch that followed from it was based on this ticket. I probably pulled the non-raw patches for #12313 when I tested ... Should we factor out a utility from the Patchbot to pull and apply patches given a ticket number? Happy to see this work did find some use after all. Again, I believe that in the future, when That enhanced traceback (including cython code!) is extremely cool. A big thanks to Volker for making that happen. With that traceback, you only only have to stare at the traceback to diagnose this problem. |
Merged: sage-5.7.beta1 |
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:Related tickets:
Further notes:
None
cannot.Apply
CC: @simon-king-jena @jdemeyer @mwhansen @vbraun @jpflori
Component: memleak
Keywords: UniqueRepresentation cached_method caching
Author: Simon King
Reviewer: Nils Bruin
Merged: sage-5.7.beta1
Issue created by migration from https://trac.sagemath.org/ticket/12215
The text was updated successfully, but these errors were encountered: