Ensure we only weakref() obj once#5
Conversation
| def __get__(self, obj, cls=None): | ||
| if self._all_weakrefable_instances and cls is not None and obj in self._method_cache: | ||
| return self._method_cache[obj] | ||
| caching = self._all_weakrefable_instances and obj is not None |
There was a problem hiding this comment.
This does no longer check for cls is not None. Is that ok? I could not construct any test cases where cls = None, so in my PR i kept it in.
There was a problem hiding this comment.
I think it's okay. The tests I added in this PR fail if we don't do the if obj is not None check. But, like you, I couldn't come up with a test case where cls=None. I'm also not actually sure why it would be a problem if cls=None for our caching here.
There was a problem hiding this comment.
https://docs.python.org/3/howto/descriptor.html#overview-of-descriptor-invocation appears to confirm that desc.__get__(obj, None) is semantically equivalent to desc.__get__(obj, type(obj))
2 participants
The performance characteristics of
WeakKeyDictionary()seem pretty different todict, so going back to atry/except KeyErrorstructure actually seems much more performant now we're using aWeakKeyDictionaryfor the non-slotted case, and doesn't seem to cost us anything for the slotted case