#13447: Make libsingular multivariate polynomial rings collectable
----------------------------------------------------------------+-----------
Reporter: nbruin |
Owner:
Type: defect |
Status: needs_work
Priority: major |
Milestone: sage-5.4
Component: memleak |
Resolution:
Keywords: | Work
issues: Understand why sometimes `new_RingWrap` needs an incref and sometimes
not
Report Upstream: None of the above - read trac for reasoning. |
Reviewers: Simon King
Authors: Nils Bruin, Simon King | Merged
in:
Dependencies: #11521 |
Stopgaps:
----------------------------------------------------------------+-----------
Comment (by SimonKing):
I have attached a new version of the patch. Here are a few comments:
For easier debugging, I created methods returning the refcount for
`RingWrap` and for polynomial rings, and for g-algebras.
'''__Concerning singular_function__'''
`currRingHdl` is now taken care of when the function is called, and not
when it is created. In that way, one prevents nastinesses like
`currRingHdl` being NULL because of a previous deallocation. When creating
it `currRing` is guaranteed to exist. I call
`rFindHdl(currRing,NULL,NULL)`, which may find a previously defined
thingy. If it can not be found, a mock ring is created by
{{{
currRingHdl = enterid("my_awesome_sage_ring", 0, RING_CMD, &IDROOT, 0)
}}}
Since the last argument is 0, we can then do
{{{
currRingHdl.data.uring = currRing
}}}
without the need to call `rDelete(currRingHdl.data.uring)`. Problem: See
below.
Since `currRing` is now not necessarily available when initialising
`SingularKernelFunction`, it is impossible to determine its arity during
initialisation, but fortunately it is still possible to determine whether
the function exists in the Singular kernel.
Therefore, during `__init__`, I merely test for existence, while the
creation of `self.call_handler` is postponed to a point were `currRing` is
guaranteed to be around.
Since Hans said that we need to make sure that `currRing->ref` is positive
when calling an interpreter function, I temporarily increment the counter
during the call.
'''__Concerning ref counting__'''
I think we should simply rely on Singular's `rKill` for deletion of rings,
so that internal data are taken care of.
`currRingHdl.data.uring` and `currRing` do not constitute a reference in
the sense of "interpreter variable". Therefor I am not incrementing or
decrementing the refcount on them (unless I missed something).
Because of ref counting based at -1, a naked `MPolynomialRing_libsingular`
and `NCPolynomialRing_plural` should have `self._ring.ref == 0`, so that
`self._ring` can be properly deleted. All other ring-related data, such as
a Gröbner strategy or elements of the ring, constitute a new reference.
The same should hold for a `RingWrap`: If one has a naked `RingWrap` then
the underlying ring should be deletable when deleting `RingWrap`. A
complication arises if both a `RingWrap` and a polynomial ring refer to
''the same'' underlying libsingular ring.
This occurs in `NCPolynomialRing_plural`, which is created from a ring
wrap. I do
{{{
cdef RingWrap rw = ncalgebra(self._c, self._d, ring = P)
# rw._output()
self._ring = singular_ring_reference(rw._ring)
}}}
hence incref, because at the end of the initialisation of self, `rw` will
be deleted, so that the refcount for `self._ring` will then be fine. The
same holds for `new_CRing` and `new_NRing` in
sage.rings.polynomial.plural.
'''__Some experiments with the new patch__'''
Worst of all: I did not run the tests yet.
Here is a test whether there is a memleak when creating a super-
commutative algebra repeatedly. Note that one needs
{{{
$ hg qa
trac_715_combined.patch
trac_715_local_refcache.patch
trac_715_safer.patch
trac_715_specification.patch
trac_11521_homset_weakcache_combined.patch
trac_11521_callback.patch
13145.patch
trac12215_weak_cached_function-sk.patch
trac12215_segfault_fixes.patch
trac_12313-mono_dict-combined-random-sk.patch
trac_12313_quit_sage.patch
trac13370_deprecate_is_field.patch
trac_13447-sanitise_ring_refcount.patch
}}}
because otherwise super commutative algebras could not be garbage
collected.
{{{
sage: from sage.rings.polynomial.plural import SCA
sage: import gc
sage: while 1:
....: E = SCA(QQ, ['x', 'y', 'z'], [0, 1], order = 'degrevlex')
....: print get_memory_usage(),id(E)
....: del E
....: _ = gc.collect()
....:
223.98828125 4520110400
223.98828125 4520102992
223.98828125 4520145584
223.98828125 4520100800
223.98828125 4520190656
223.98828125 4520116128
223.98828125 4516130448
223.98828125 4520183424
223.98828125 4338291168
223.98828125 4520148816
223.98828125 4520173408
223.98828125 4510376384
223.98828125 4520106672
...
}}}
Conclusion: There is no leak, even though it really is a new ring each
time.
However, not all is good. Namely, we interrupt the computation above, and
continue as follows:
{{{
sage: from sage.libs.singular.function import singular_function
sage: P.<x,y,z> = PolynomialRing(QQ)
sage: ringlist = singular_function("ringlist")
sage: l = ringlist(P)
// ** redefining my_awesome_sage_ring **
}}}
So, here you see the problem with creating the mock ring. I can only hope
that Hans will tell us how one can ''create'' a `currRingHdl` for a given
`currRing`, rather than only finding a previously created `currRingHdl`.
Because of that last problem, I think it doesn't make much sense to test.
However, comments are welcome.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13447#comment:74>
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.
