Use ffi.gc() on the ring nodes to avoid needing a weakref to PersistentPy #134
Conversation
persistent/picklecache.py
Outdated
| def _from_addr(self, addr): | ||
| for i in gc.get_objects(): | ||
| if id(i) == addr: | ||
| return i |
There was a problem hiding this comment.
I don't like this. id() values can be reused for unrelated objects in CPython.
There was a problem hiding this comment.
Very true!
I don't expect this to be used on CPython. Even if it were, the ref counting semantics that are tied to ffi.gc should eliminate the race condition.
There was a problem hiding this comment.
That fallback loop was borrowed from objgraph, IIRC. Perhaps the state of the art for non-CPython implementations has moved on since I checked?
There was a problem hiding this comment.
objgraph is a debugging tool. I allow hacks in debugging code that I wouldn't allow in production code ;)
There was a problem hiding this comment.
I think my primary reason for rejecting this is that AFAIU _from_addr() is supposed to return None if the object associated with the old id() value has been freed, but the get_objects() method cannot reliably implement that.
| # they don't cooperate (without this check a bunch of test_picklecache | ||
| # breaks) | ||
| or not isinstance(value, self._SWEEPABLE_TYPES)): | ||
| ring.delete_node(node) |
There was a problem hiding this comment.
Just double-checking: does self.ring support mutation during iteration?
There was a problem hiding this comment.
Happily now it does!
I took this opportunity to improve the semantics of the one-ond-only Ring implementation. It resulted in, i think, some nice simplifications.
| # We allow mutation during iteration, which means | ||
| # we must get the next ``here`` value before | ||
| # yielding, just in case the current value is | ||
| # removed. |
There was a problem hiding this comment.
And this nicely answers my question ;)
The The ZODB
Indeed. My first few attempts resulted in segfaults everywhere, and then in undefined behaviour. But I think (hope?) this is close to the simplest possible exploitation of it: a reliable ref-counted object that doesn't depend on the circular CPython GC or This doesn't have any size benefit over |
|
Is Jython still a thing and is anyone using it together with zopefoundation stuff? Currently we are not even testing (at least not automatically) on Jython so I feel not very well in claiming that we still support it. |
It's still a thing. As for its use with this stuff, I don't know anymore.
We don't really claim to support it (it's not in the classifiers, for example). There are just some remnants of support for it left in comments and alternate code paths. However, as of persistent 4.5.0, which made CFFI a firm requirement, all possibility of using Jython was eliminated (until such time as the CFFI port is finished). (I should have remembered that.) So there's really no need for this code path and I can drop it. |
…nt objects. Except on PyPy, where we can already weakref them automatically. Fixes #133.
|
I currently have the idea to use a `WeakKeyDictionayy` (with an
index object (i.e. a persistent object) as key) to
solve "zopefoundation/Products.ZCatalog#94".
Does this become impossible with your optimizations?
|
|
I believe the two are unrelated. The changes here have nothing to do with making objects weakly referenceable or not. All that changed here is that the pure Python PickleCache can now store persistent objects that cannot be weakly referenced; the C implementation has always been able to do this. (All |
Except on PyPy, where we can already weakref them automatically. On CPython, track them using their
id(can't use anffi.new_handle"pointer" because it strongly references the object it's a handle for).Fixes #133.