#13394: Write a WeakValueDictionary with safer key removal
-------------------------------------+-------------------------------------
Reporter: nbruin | Owner: rlm
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.13
Component: memleak | Resolution:
Keywords: | Merged in:
Authors: Simon King | Reviewers:
Report Upstream: None of the above | Work issues:
- read trac for reasoning. | Commit:
Branch: | e4adaebf2bd8a219f05b28746210cc0b0d0bba88
u/SimonKing/ticket/13394 | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Comment (by nbruin):
Replying to [comment:26 SimonKing]:
> Hi Nils,
>
> do I understand correctly: What you tried means patching Python,
No, it doesn't mean patching python. The file I attached is straight
cython, you can compile it on pretty much any sage, and it will give you
the routine `del_dictitem_exact`. You could use this to subclass `dict`
now to create a !WeakValueDictionary and use that routine in the callback
to clean up without triggering python equality testing. You'd end up with
an implementation that stays very close to Python's original `dict`. The
only other thing to do is to ensure that values get wrapped and unwrapped
upon entering/leaving with the appropriate !KeyedRef (and backport the
iteration protection if we care)
Some arguments against doing so are:
- We rely on some of the fields of !PyDictObject that are declared in
`Python.h` but that are not part of the documented C-API of cpython
- We rely on the method python currently uses to "delete" elements from a
dict
- We rely on the probing formula that python uses.
Arguments for:
- None of these things have changed in ages and they haven't changed in
Python3 either, so from a practical point of view it's unlikely this will
trigger incompatibilities
- If this changes, the implementation details of !PyDictObject would be
different and we'll notice this by changes in `Python.h`. So, even if this
changes we won't get silent errors, we'll just get a compilation error,
telling us exactly which code to adapt.
- I expect that this is more efficient and easier to write.
> and you want to propose this upstream, but do not necessarily want to
include it in Sage yet?
we could propose it, but it's not going to be in Python2 (I suspect it'll
be considered a new feature, not a bug-fix. Even
[http://bugs.python.org/issue7105] which is recognised as a bug hasn't
been backported to 2.7.
> Strange. How does it crash? By commit [changeset:c3dba98] the
`WeakValueDictionary` is used in Sage. Are you saying it crashes at
startup? What does it say?
There was a problem with MRO resolution upon startup, in resolving
lazyimport of `Facade.Sets` or something like that. It seemed similar to
the `Matriod` docbuild problem. he problem seems to be gone now, after
wrestling git into unpdating the proper branch. Are we getting a FAQ on
"how to do ..." for the git stuff?
--
Ticket URL: <http://trac.sagemath.org/ticket/13394#comment:34>
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.
