#14059: Fix refcount/deallocation of integers
------------------------------+---------------------------------------------
Reporter: SimonKing | Owner: rlm
Type: defect | Status: positive_review
Priority: blocker | Milestone: sage-5.7
Component: memleak | Resolution:
Keywords: | Work issues:
Report Upstream: N/A | Reviewers: Jean-Pierre Flori
Authors: Simon King | Merged in:
Dependencies: | Stopgaps:
------------------------------+---------------------------------------------
Changes (by jpflori):
* status: needs_review => positive_review
* reviewer: => Jean-Pierre Flori
Old description:
> In #2435, the deallocation function in sage/rings/integer.pyx became as
> follows:
> {{{
> #!python
> cdef void fast_tp_dealloc(PyObject* o):
>
> # If there is room in the pool for a used integer object,
> # then put it in rather than deallocating it.
>
> global integer_pool, integer_pool_count
>
> if integer_pool_count < integer_pool_size:
>
> # Here we free any extra memory used by the mpz_t by
> # setting it to a single limb.
> if (<__mpz_struct *>( <char *>o + mpz_t_offset))._mp_alloc > 10:
> _mpz_realloc(<mpz_t *>( <char *>o + mpz_t_offset), 10)
>
> # It's cheap to zero out an integer, so do it here.
> (<__mpz_struct *>( <char *>o + mpz_t_offset))._mp_size = 0
>
> # And add it to the pool.
> integer_pool[integer_pool_count] = o
> integer_pool_count += 1
> return
>
> # Again, we move to the mpz_t and clear it. See above, why this is
> evil.
> # The clean version of this line would be:
> # mpz_clear(<mpz_t>(<char *>o + mpz_t_offset))
>
> mpz_free((<__mpz_struct *>( <char *>o + mpz_t_offset) )._mp_d, 0)
>
> # Free the object. This assumes that Py_TPFLAGS_HAVE_GC is not
> # set. If it was set another free function would need to be
> # called.
>
> PyObject_FREE(o)
> }}}
>
> The purpose of #2435 was to fix a memory leak introduced in #1337.
> Problem: One has to manually incref `ONE`, which I find a bit suspicious.
>
> When I build sage-5.7.beta2 in debug version (as by #13864), Sage crashes
> at exit, while attempting `PyObject_FREE(o)`. I will attach backtraces in
> comments.
New description:
In #2435, the deallocation function in sage/rings/integer.pyx became as
follows:
{{{
#!python
cdef void fast_tp_dealloc(PyObject* o):
# If there is room in the pool for a used integer object,
# then put it in rather than deallocating it.
global integer_pool, integer_pool_count
if integer_pool_count < integer_pool_size:
# Here we free any extra memory used by the mpz_t by
# setting it to a single limb.
if (<__mpz_struct *>( <char *>o + mpz_t_offset))._mp_alloc > 10:
_mpz_realloc(<mpz_t *>( <char *>o + mpz_t_offset), 10)
# It's cheap to zero out an integer, so do it here.
(<__mpz_struct *>( <char *>o + mpz_t_offset))._mp_size = 0
# And add it to the pool.
integer_pool[integer_pool_count] = o
integer_pool_count += 1
return
# Again, we move to the mpz_t and clear it. See above, why this is
evil.
# The clean version of this line would be:
# mpz_clear(<mpz_t>(<char *>o + mpz_t_offset))
mpz_free((<__mpz_struct *>( <char *>o + mpz_t_offset) )._mp_d, 0)
# Free the object. This assumes that Py_TPFLAGS_HAVE_GC is not
# set. If it was set another free function would need to be
# called.
PyObject_FREE(o)
}}}
The purpose of #2435 was to fix a memory leak introduced in #1337.
Problem: One has to manually incref `ONE`, which I find a bit suspicious.
When I build sage-5.7.beta2 in debug version (as by #13864), Sage crashes
at exit, while attempting `PyObject_FREE(o)`. I will attach backtraces in
comments.
The problem is the workaround for ONE (which is not cdefed so get
collected at exit) got removed (accidently?) at #12615.
We simplify the workaround nd completetly remove the use of ONE and use
the new one from #12615.
There still are strange things going one with allocation and deallocation
before and after setting up the hooked functions with debug builds of
Python, but with the current setting everything seems, so the workarounds
are sufficient.
Apply [attachment:trac_14059.patch].
--
Comment:
If Robert has any comment, feel free to change the status of the ticket or
enlighten us.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14059#comment:50>
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 unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
