#11521: Use weak references to cache homsets
--------------------------------------------------+-------------------------
Reporter: jpflori | Owner: robertwb
Type: defect | Status: needs_work
Priority: major | Milestone: sage-5.3
Component: coercion | Resolution:
Keywords: sd35 | Work issues:
Report Upstream: N/A | Reviewers: Jean-Pierre
Flori
Authors: Simon King | Merged in:
Dependencies: #12969; to be merged with #715 | Stopgaps:
--------------------------------------------------+-------------------------
Changes (by SimonKing):
* status: needs_review => needs_work
Comment:
Replying to [comment:145 nbruin]:
> '''sage/categories/homset.py''':
> {{{
> _cache = TripleDict(53)
> }}}
> I guess this 53 is a tuned parameter. Can you comment on the choice?
My patch in #715 is, of course, based on old code. In the old code, there
were comments on the choice of the parameter, that I summarise here. The
parameter gives the initial number of buckets in a `TripleDict`. Thus, in
order to ensure a good distribution of the items into the buckets, the
parameter should be prime and should certainly not be even; there actually
is a doc test that shows what happens if the parameter is even. It should
not be too big, because it determines the number of empty buckets that are
created during initialisation of the `TripleDict`. And it should not be
too small, because otherwise the `TripleDict` would soon be resized (which
is a costly operation).
I thought that 53 is a good choice, given these comments. However, it is
not tuned by experiments.
> line 214 ...
> Could it happen that bool(H) is false if H is not None? Perhaps safer to
test
> `H is not None`? I'm not saying that you should. Just comment please.
That line is old code that has not been introduced by my patch, right? I
think one ''should'' test `H is not None` - mainly for speed, but also for
preventing that bool(H) is false even though H is not None.
> line 247 (same thing)
>
> line 263
> {{{
> _cache[key] = weakref.ref(H)
> }}}
> Can you clarify the following: The way I read this is that H will not be
> protected against GC (good!), but that `_cache` will be be storing a
strong(?)
> reference to `key`, meaning that the entry in `_cache` under `key` will
linger
> with a dead weakref afterwards.
Correct, at least theoretically.
The reason for the code was that at some point one needed to break a chain
of strong references. In the memory leak fixed here, there was a reference
from H back to the keys under which H is stored in the `TripleDict`.
Hence, practically, when H gets collected, the item in the `TripleDict`
becomes collectable as well. Hence, the dead weakref will soon vanish.
At least, that's what ''should'' happen. But:
> Shouldn't there be a callback registered here
> with the weakref that contacts `_cache` to remove the `key` entry when
`H` gets
> collected? It may be that your #715 changes already make `_cache` into
essentially a
> `WeakKeyDict`,
Yes. A `WeakKeyDict`, but not a `WeakValueDict`. That's why I am using a
weak reference to H.
> but it seems to me that `_cache` should still have a callback
> registered on the `weakref` to `H` as well, to remove the entry if `H`
gets GCd.
Recall that each `TripleDict` has an eraser. I think, at least with the
new patch at #715 that I have posted today, such a callback would be
easily possible.
> I haven't done extensive testing myself, but you have and the bot happy,
so I don't think me running tests would change much.
Well, in an ideal world, a different brain will be able to invent
different stress tests, thus, find different bugs...
Anyway. I think I should turn `if H:` into `if H is not None:` and should
introduce a callback to the weak reference.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11521#comment:146>
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.
