#13447: Make libsingular multivariate polynomial rings collectable
----------------------------------------------------------------+-----------
Reporter: nbruin |
Owner:
Type: defect |
Status: needs_review
Priority: major |
Milestone: sage-5.4
Component: memleak |
Resolution:
Keywords: | Work
issues: Input from a libsingular expert
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 nbruin):
I'm not so sure that
{{{#!python
currRingHdl = enterid("my_awesome_sage_ring", 0, RING_CMD, &IDROOT, 1)
currRingHdl.data.uring.ref += 1
}}}
is entirely safe. In 'libsing-test2.cc' (in the Singular package, the
following similar code is used:
{{{#!C++
// prepare the arguments
// create a ring Q[x,y]
// the variable names
char **n=(char**)omAlloc(2*sizeof(char*));
n[0]=(char*)"x";
n[1]=(char*)"y";
// create the ring
ring R=rDefault(0,2,n);
// n is not needed any more:
omFreeSize(n,2*sizeof(char*));
// make it the default ring, also for the interpeter
idhdl newRingHdl=enterid("R" /* ring name*/,
0, /*nesting level, 0=global*/
RING_CMD,
&IDROOT,
FALSE);
IDRING(newRingHdl)=R;
rSetHdl(newRingHdl);
}}}
I'd assume `IDRING(newRingHdl)` is `newRingHdl.data.uring`, so the
assignment above does not assume anything about that field being
initialized. So who knows where `newRingHdl.data.uring.ref` points!
I get the impression that indeed, `currRing` is set to something valid
nearly always, so your assumption that it is during
`SingularFunction.__init__` (that's earlier than calling!) might be OK.
The singular code does contain an awful lot of `currRing!=NULL` checks
though, so I'm not so sure that's really guaranteed.
OK, a little searching of the Singular source shows that `enterid` (which
takes 6 arguments but we call it with 5 and so does the above example)
apparently calls `idrec::set`, in our case with `init=1` and the example
above with `init=0`. The `init=1` causes a call `idrecDataInit` and for a
ring command that does `omAlloc0Bin(sip_sring_bin)`. The resulting pointer
gets indeed assigned to what I think is `...data.uring` (although not in a
very typerespecting way, but I guess that's par for C code).
So my guess is that by calling with `init=1` we indeed get a block that we
can delete. Since that's all we do with it I think the cleaner way for us
would be
{{{
currRingHdl = enterid("my_awesome_sage_ring", 0, RING_CMD,
&IDROOT, 0)
currRingHdl.data.uring = NULL
}}}
(or whatever is the appropriate value for uring), as per the libsing
example above)
{{{
if currRingHdl.data.uring!= currRing:
if currRingHdl.data.uring != NULL:
singular_ring_delete(currRingHdl.data.uring)
currRingHdl.data.uring = currRing
singular_ring_reference(currRing)
}}}
(or indeed just leave init=1 in place so that `currRingHdl.data.uring` is
never NULL. In any case, this happens at most once in any sage session, so
the leaked memory (if any) would be minuscule.
Concerning refcounts: I found the following in
`dyn_modules/python/ring_wrap.h`
{{{#!C++
#include <boost/intrusive_ptr.hpp>
using namespace boost;
//typedef intrusive_ptr<ip_sring> Ring;
// inline void intrusive_ptr_add_ref(ring r){
// r->ref++;
// }
// inline void intrusive_ptr_release(ring r){
// r->ref--;
// if (r->ref<=0) rDelete(r);
// }
}}}
which is in step with our use of the ref field. But the code is commented
out! There is such a thing as an `intrusive_ptr` in Boost and I think it
needs exactly those two functions declared to function. If this is what is
intended then we're OK. If this is commented out because now a different
scheme is used, we may be in trouble.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13447#comment:51>
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.
