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