#14711: Memleak when creating QuadraticField
-------------------------------------------------+-------------------------
Reporter: jpflori | Owner:
Type: defect | davidloeffler
Priority: critical | Status: new
Component: number fields | Milestone: sage-5.13
Keywords: memleak, number field, | Resolution:
QuadraticField | Merged in:
Authors: | Reviewers:
Report Upstream: N/A | Work issues:
Branch: u/SimonKing/ticket/14711 | Commit:
Dependencies: | Stopgaps:
-------------------------------------------------+-------------------------
Comment (by nbruin):
Replying to [comment:62 SimonKing]:
> I think this is not the case with the branch that I have just uploaded.
I did
Got it! Thanks. To have documented ''why'' we're OK here:
`_coerce_from_hash` is a `TripleDict`, so it holds a ''strong'' reference
to the map stored. This is what keeps the map alive. However, when one of
the domain or codomain (or category; whatever the third key component is)
vanishes, then the `TripleDict` callback will remove the entry and hence
the strong reference to the map. This makes it possible for the map to be
GCed. So the required callback to clean up is already triggered via the
key triple.
> What weakrefs are you talking about? Those to domain and codomain?
> And what would the callback be supposed to do?
Indeed, domain and codomain. So those callbacks are already taken care of
by `_coerce_from_hash` being a `TripleDict`.
> A different story is `_coerce_from_list`, which is a list. Here, we
might need to take care of defunct maps. Perhaps the maps there shouldn't
be weakened in the first place (`_coerce_from_list` is filled by
`register_coercion()`, and perhaps it would make sense to keep these maps
strong), but I am not sure if this wouldn't re-introduce the
`QuadraticField` leak I have just fixed.
Do we really need `_coerce_from_list`? Have we identified what it does
that cannot be accomplished with the data stored in `_coerce_from_hash`?
> As I have stated above, I don't think there is a problem with
`_coerce_from_hash` accumulating garbage. But `_coerce_from_list` may
contain garbage, of limited size, because it is only added to by explicit
calls to `register_coercion()`.
>
> I think a better strategy would be to make `discover_coercion` check
whether it meets a defunct map when it performs the backtrack algorithm.
I doubt you could prove about such an incidental "bumping in" strategy
that the number of defunct maps is bounded linearly in the number of
active stuff (perhaps measured by active entries in `_coerce_from`?). That
means you wouldn't be able to prove that you don't have a memory leak, and
I suspect that with a little study, one could concoct an example that
exhibits the leak as well. If `discover_coercion` would regularly visit
all entries in `_coerce_from_list` then the process would be way too slow.
Given how delicate this code is, please do put ample documentation about
assumptions and strategies in the code. Otherwise, the next person to work
on this code will likely screw things up. Hopefully it'll be cought by
doctests, but we've seen that this could easily not happen.
--
Ticket URL: <http://trac.sagemath.org/ticket/14711#comment:69>
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.
For more options, visit https://groups.google.com/groups/opt_out.
