#12313: Fix yet another memory leak caused by caching of coercion data
--------------------------------------------------+-------------------------
Reporter: SimonKing | Owner:
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: #11521, #11599, #12969, #12215 | Stopgaps:
--------------------------------------------------+-------------------------
Comment (by SimonKing):
Replying to [comment:173 nbruin]:
> > I wrote some general code for what I now call `IdKeyDict`: A weak key
dictionary (that also accepts items that aren't weak-reffable) with keys
of prescribed length, keys being compared by identity. There, the tmp is
not used.
>
> `<size_t><object>...` has two effects: It does de refcount dance (mostly
superfluous) and it inserts PyInt_AsSsize_t call (necessary). By
introducing the right header template you can get the second and avoid the
first. See the code at #13387.
Thank you for the pointer!
> Perhaps JP has a point -- the main concern on this ticket is to get a
''correct'' `MonoDict`. Discussions about optimizing it should go on
#13387. The memory leaks desperately need solving. Why don't you move
`IdKeyDict` there? If you can get it to perform sufficiently well, we
could drop it in. Having the code a little more structured would be a
benefit from a maintenance point of view.
I see these alternative approaches:
1. We essentially keep the code from #715 and the main patch from here,
for now. However, we need to do some fixes, because of the segfaults and
stuff. But apart from that fix, #13387 should focus on performance and
perhaps generality `IdKeyDict`. #12313 will be a dependency for #13387.
2. The performance of `IdKeyDict` is decent, actually seems to be better
than `MonoDict` and `TripleDict`. So, why not use the more general
structure for the coercion dicts? Hence, we could make #13387 be
independent of #715 and #12313 and mark both #715 and #12313 as
duplicates.
To decide which way to go, we should probably ask: How much additional
work is needed to fix the current segfault problems with #715 and #12313?
If it isn't much, then the first way may be better. Otherwise, we may
choose the second way.
As much as I see, what needs to be fixed (or just needs to be reviewed,
because it already is in the patches): After testing equality of memory
addresses, test whether the weak reference is still valid. If it isn't
then delete the weak reference, in order to avoid its callback being
active.
So, 1. or 2., or a third way?
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/12313#comment:180>
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.
