search

zopefoundation / persistent

automatic persistence for Python objects
https://pypi.org/project/persistent/
Other
46 stars 28 forks source link

Use ffi.gc() on the ring nodes to avoid needing a weakref to PersistentPy #134

Closed jamadden closed 4 years ago

jamadden commented 4 years ago

Except on PyPy, where we can already weakref them automatically. On CPython, track them using their id (can't use an ffi.new_handle "pointer" because it strongly references the object it's a handle for).

Fixes #133.

jamadden commented 4 years ago

I'm very unqualified to review this (what even is ffi.gc? I probably don't even want to know), but other than that this looks very plausible to me. Except for the gc.get_objects() loop.

The gc.get_objects loop is for, nominally, Jython support. Such support is still claimed and supported by comments in a few places. That once mattered a lot to me, but less so now, so if it's a problem I'm happy eliding it.

The ZODB PURE_PYTHON tests on CPython run now with this change…with the exception of a block that's specifically targeting Jython, and one that expects ref counts of None to remain stable: yikes! (I think I might have written that.)

what even is ffi.gc? I probably don't even want to know

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 __weakref__, yet still delivers a callback. (My first tries used CFFI handles, which wound up using cycles, which is not a good practice. I squashed that history away.)

This doesn't have any size benefit over __weakref__ on CPython, I think. It just has consistency benefits. Maybe that is or is not enough.

icemac commented 4 years ago

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.

jamadden commented 4 years ago

Is Jython still a thing and is anyone using it together with zopefoundation stuff?

It's still a thing. As for its use with this stuff, I don't know anymore.

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.

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.

d-maurer commented 4 years ago

I currently have the idea to use a WeakKeyDictionayy (with an index object (i.e. a persistent object) as key) to solve "https://github.com/zopefoundation/Products.ZCatalog/issues/94". Does this become impossible with your optimizations?

jamadden commented 4 years ago

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 Persistent objects that come from ZODB are stored in a PickleCache.) By default, subclasses of Persistent can be weakly referenced.