-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Replace use of renderer._uid by weakref. #9070
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This avoids the need for a note regarding issues with renderer classes that do not inherit from RendererBase. Remember that the goal of _uid was to prevent two renderers from sharing the same id() even though they were actually different (if one has been GC'd and another is created at the same address). Maintaining a weakref to the renderer works as well, as weakref equality is thusly defined: If the referents are still alive, two references have the same equality relationship as their referents (regardless of the callback). If either referent has been deleted, the references are equal only if the reference objects are the same object.
Does this trigger the edge case you fix in #9074? |
#9074 is actually totally unrelated (that came up when I was trying to build a weak cache of FT2Font -> FT_Face for mpl_cairo some time ago). |
Question... this all hinges on the definition of the equality of weakrefs and to not hold hard references to large python objects. The property tuple that contains the weakref here is meant to get hashed, which means that the weakrefs aren't directly compared, right? I don't know what the rules are with weakref hashes. |
|
Ah, so the hashes of two weakrefs that point to the same object will be the same? |
Yes. |
Doing a bit more digging, I think we can simplify this even more and just use I think there is a (probably impossible) race condition here is that if you try to take the hash for the first time after the object has gone away. In [108]: class foo(): pass
In [109]: (weakref.ref(foo()), ) in {}
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-109-32cf9cf15e2e> in <module>()
----> 1 (weakref.ref(foo()), ) in {}
TypeError: weak object has gone away The docs from
|
Isn't that what the original code did (before the uid change)? The uid approach tried to fix an edge case where id() of two renderers created at different times could collide, right? |
no, the original code was using |
If ids can collide, hashs can collide too.
The only use of get_prop_tup in the codebase immediately uses the tuple for a dict lookup (in _get_layout), while the renderer is definitely still alive, so there won't be a race condition. |
Hmm, I wonder if that is an edge case that should go up to cpython? I think this should go in as it eliminates a epi-cycle that we added this cycle. |
id(C()) == id(C()) makes sense because id just returns the pointer address and the object is being reused (there's probably a free list somewhere). |
In [122]: class foo():...
In [123]: foo() == foo()
Out[123]: False
In [125]: id(foo()) == id(foo())
Out[125]: True
In [126]: hash(foo()) == hash(foo())
Out[126]: True
In [127]: foo() == foo()
Out[127]: False
In [128]: foo() is foo()
Out[128]: False violates the docs on |
Ah, I see. In https://docs.python.org/3/reference/datamodel.html#object.__hash__
should be
right? |
I think the sentence is ok as it is, it may need some clarity that it is 'if' not 'iff' though. |
This avoids the need for a note regarding issues with renderer classes
that do not inherit from RendererBase.
Remember that the goal of _uid was to prevent two renderers from sharing
the same id() even though they were actually different (if one has been
GC'd and another is created at the same address). Maintaining a weakref
to the renderer works as well, as weakref equality is thusly defined:
Alternate for #8708, which it reverts. #9069 should get modified accordingly if this gets merged.
attn @QuLogic @tacaswell
PR Summary
PR Checklist