#12313: Fix yet another memory leak caused by caching of coercion data
-------------------------------------------------+--------------------------
       Reporter:  SimonKing                      |         Owner:  rlm          
                            
           Type:  defect                         |        Status:  needs_review 
                            
       Priority:  major                          |     Milestone:  sage-5.3     
                            
      Component:  memleak                        |    Resolution:               
                            
       Keywords:  coercion weak dictionary       |   Work issues:               
                            
Report Upstream:  N/A                            |     Reviewers:  Simon King, 
Jean-Pierre Flori, John Perry
        Authors:  Simon King, Jean-Pierre Flori  |     Merged in:               
                            
   Dependencies:  #715, #11521, #11599, #12969   |      Stopgaps:               
                            
-------------------------------------------------+--------------------------

Comment (by nbruin):

 '''sage/structure/coerce_dict.pyx''':

 line 96:

 Perhaps you can explain the mechanism here. If I understand correctly,
 `_refcache`
 is a global dictionary, keyed with the "key" part of `KeyedRef`s and as
 values
 lists of `KeyedRef`s with that key. The deletion consists of looking up
 the key
 in the registered `MonoDict` and removing the entry if it's there (it
 could
 already have disappeared if the entry was explicitly deleted from the
 `MonoDict`, because `MonoDict.__delitem__` doesn't bother updating
 `_refcache`)

 It then proceeds to remove the now defunct `KeyedRef` instance from
 `_refcache`.
 But doesn't that mean that even if we have `len(L)==0` as a result from
 that,
 the entry `k : L` remains in `_refcache`? That means we get a whole pile-
 up of
 integers and empty lists in memory as a result (something I have indeed
 observed). These objects aren't so big, so this leaks much slower of
 course,
 but it's still a leak. I suspect you copied this from `TripleDict` which
 will
 have the same problem.

 line 103:
 {{{
         for i from 0 <= i < PyList_GET_SIZE(bucket) by 4:
             if <size_t><object>PyList_GET_ITEM(bucket,i)==k:
                 del bucket[i:i+2]
 }}}
 It reads to me that that should be `... by 2` corresponding tro the loops
 in
 `MonoDict.__setitem__` and `MonoDict.__getitem__`

 line 497: (in `Monodict._getitem__`)

 How bad is the race condition here? I guess we're trusting that
 `bucket` doesn't change and that `bucket` has been purged of any dead
 references
 (i.e., that all `MonoDictEraser` callbacks have happened). Does keeping
 the GIL
 guarantee this?

 Does the line
 {{{
                 return <object>PyList_GET_ITEM(bucket, i+1)
 }}}
 increase a refcount on the object we retrieve? Does the `<object>`
 typecast do
 that? Or does the `return` do that? (now that the object has been
 retrieved it
 should be protected from deletion)

 line 536:

 Am I reading correctly that if k cannot be keyreffed then k is going to
 get a
 strong ref in _refcache and will be eternalized? Is that what we want? If
 so,
 write this in a comment! Especially because currently even explicit
 deletion
 would leave the reference in `_refcache` intact, so you'd be leaking in
 cases
 where a normal dictionary would not.

-- 
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/12313#comment:125>
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.

Reply via email to