Changes between Initial Version and Version 117 of Ticket #14711


Ignore:
Timestamp:
10/06/13 21:30:55 (6 years ago)
Author:
SimonKing
Comment:

Replying to nthiery:

Could you post a quick summary (say in the ticket description/title) of what the current patch does?

Done. OK, the summary is actually not quick. Sorry.

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #14711

    • Property Status changed from new to needs_review
    • Property Authors changed from to Simon King
    • Property Cc nthiery added
    • Property Milestone changed from sage-5.11 to sage-5.12
    • Property Summary changed from Memleak when creating QuadraticField to Weak references in the coercion graph
    • Property Branch changed from to u/SimonKing/ticket/14711
    • Property Commit changed from to 5168cfd15d4cdbaa3ffdbd4be0f7d783f77257c7
  • Ticket #14711 – Description

    initial v117  
    66}}}
    77(This is with 5.10.rc0)
     8
     9__Problem analysis__
     10
     11The quadratic field is created with a coerce embedding into `CLF`. At the same
     12time, this coerce embedding is stored in `CLF._coerce_from_hash`:
     13{{{
     14sage: phi = CLF.coerce_map_from(Q)
     15sage: phi is Q.coerce_embedding()
     16True
     17sage: Q in CLF._introspect_coerce()['_coerce_from_hash']
     18True
     19}}}
     20The "coerce_from_hash" is a `MonoDict`, hence, has only a weak reference to the key
     21(Q, in this case). However, there still is a ''strong'' reference from
     22CLF to the coerce map phi. And phi has a strong reference to its
     23domain, thus, to Q. Hence, the existence of CLF prevents garbage collection of
     24Q.
     25
     26And there is a second chain of strong references from CLF to Q: From CLF to
     27phi to the parent of phi (i.e., a homset) to the domain Q of this homset.
     28
     29__Suggested solution__
     30
     31We can not turn the reference from CLF to phi into a weak reference, because
     32then even a strong reference to Q would not prevent phi from garbage
     33collection. Hence, we need to break the above mentioned reference chains in
     34two points. In the attached branch, maps generally keep a strong reference to
     35the codomain (this is important in composite maps and actions), but those used
     36''in the coercion system'' (and ''only'' there!!) will only have a weak
     37reference to the domain, and they set the cdef `._parent` attribute to `None`
     38(hence, we also override `.parent()`, so that it reconstructs the homset if
     39the weak reference to the domain is still valid).
     40
     41To preserve the `domain()/codomain()` interface, I have removed the method
     42`domain()` and have replaced it by a cdef public attribute that will either
     43hold a weak reference (which returns the domain when called, hence, the
     44interface does not change) or a `ConstantFunction` (which should actually be
     45faster to call than a method). Since accessing a cdef attribute is still
     46faster, the cdef attribute `_codomain` is kept (since this will always be a
     47strong reference), but `_domain` has been removed.
     48
     49This "weakening of references" is done for the coercions found by
     50`discover_coerce_map_from()` stored into `_coerce_from_hash`. So, this mainly
     51happens for things done with `_coerce_map_from_()` and with composite
     52maps. Similarly for `_convert_from_hash`.
     53
     54Weakening is ''not'' used on the maps that are explicitly registered by
     55`.register_embedding()` and `.register_coercion()`. This is in order to
     56preserve the connectivity of the coercion graph. The `register_*` methods
     57are only used on selected maps, that are of particular importance for the
     58backtrack search in `discover_coerce_map_from()`. These ''strong''
     59registrations do not propagate: Compositions of strongly registered
     60coercions found by `discover_coerce_map_from()` will be weakened.
     61
     62Since weakened maps should not be used outside of the coercion system, its
     63string representation shows a warning to replace them by a copy. The attached
     64branch implements copying of maps in some additional cases.
     65
     66`SchemeMorphism` can not inherit from `Morphism`, because of a bug with
     67multiple inheritance of a Python class from Cython extension classes. But once
     68this bug is fixed, we surely want to make `SchemeMorphism` inherit from
     69`Morphism`. This transition is prepared here.
     70
     71In any case, the commit messages should give a concise description of what has
     72been done.
     73
     74'''__Still TODO__'''
     75
     76Let the string representation of weakened maps point the user to the need of
     77creating a copy.
     78
     79'''__TODO in future tickets__'''
     80
     81- Provide a documentation of the use of weak references in coercion, and of
     82  different ways of registering coercions, with their different impacts on
     83  garbage collecion.
     84- Provide a version of `.register_coercion()` that weakens the coercion
     85  map. It would hence have the same effect as returning a map by
     86  `._coerce_map_from_()`, but of course `._coerce_map_from()` could not easily
     87  be changed in an interactive session.
     88- provide copying for ''all'' kinds of maps.
     89
     90'''__Effects on the overall functioning of Sage__'''
     91
     92It is conceivable that some parts of Sage still suppose implicitly that stuff
     93cached with `UniqueRepresentation` is ''permanently'' cached, even though the
     94seemingly permanent cache was not more than a consequence of a memory leak in
     95the coercion system. With the attached branch, garbage collection of parent
     96structures will much more often become possible. Hence, code that relied on a
     97fake-permanent cache would now need to create the same parent repeatedly.
     98
     99I (Simon) have tested how many additional parent creations occur with the
     100attached branch when running `sage -t --all`. The findings are summarised in
     101comment:107: The number of additional parent creations increased by not more
     102than 1% for all but two parent classes (both related with tableaux). I also
     103found that the time to run the tests did not significantly increase.
     104
     105Jean-Pierre has occasionally stated that some of his computations have been
     106infeasible with the memory leak in the above example. I hope that his
     107computations will now succeed.