GeezerVison

Skipping news since this is just fixing a bug in an unreleased new feature.

🤖 New build scheduled with the buildbot fleet by @carljm for commit 6d0b789 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

🤖 New build scheduled with the buildbot fleet by @carljm for commit 9b938c7 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

Thanks @Yhg1s for the review! I've updated per all comments.

@Yhg1s I pushed the explicit exhaustiveness check, and one possible way of addressing your comments about the x-macros. I really don't feel strongly about how to handle the event type logging, so if you have a clearly preferred approach, just let me know and I'll do that.

And as discussed offline, let's run the buildbots again to be safe; after that feel free to merge.

🤖 New build scheduled with the buildbot fleet by @carljm for commit 05e2ce0 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

I can't help feeling that we would be better off without these watchers.
They appear to be bug magnets. Handling the "zombie" objects is tricky and error prone.
How is the watcher interacting with the freed object if there are no references to it?

This PR adds quite a lot of code to dict_dealloc which is called for every dictionary created, which has performance implications.

I think we should just remove these watchers. What are they for?
I've been going through possible optimizations for CPython and these watchers aren't needed for any of them.
I don't see why Cinder would need them either.

(As an aside: we won't need the creation events either, but I can see why Cinder wants those).

This PR adds quite a lot of code to dict_dealloc which is called for every dictionary created, which has performance implications.

I was also initially concerned about adding an incref and decref in this path, but on a pyperformance run (with --enable-optimizations build, on a bare-metal AWS host) this PR was 1.00x faster than the base commit in main, so it doesn't seem to have a detectable impact?

I don't see why Cinder would need them either.

In general, in Cinder they are used for de-registering objects from JIT registries consisting of borrowed references, to avoid having dangling pointers in these registries. E.g. for each watched dict, we remember what key(s) we care about in that dict, and for each key we may also watch a different builtins (and maybe also globals) dict that that key might actually have been found in (or might still need to be found in if the key goes away in the main dict). If the main dict is deallocated we unwatch the additional globals/builtins dicts, and erase the whole entry from the registry. Similarly, we have a registry of created functions/code-objects that are pending compilation (in some modes we don't compile eagerly but do a multi-threaded compilation of everything pending compilation, right before fork), so if a function or code object is deallocated we remove it from that registry.

In every case the deallocated handler doesn't do anything with the zombie PyObject itself, it just needs the pointer value to erase any registry entry for that pointer value.

The watcher must take a reference to any object it is watching, otherwise the refcount is incorrect and we crash and burn.

Cinder JIT currently doesn't take references to objects it is watching, so as not to keep them alive if they would otherwise die. There is no "crash and burn" precisely because of the dealloc events; if the object is deallocated we know about it and can erase any pointer to it we were keeping. Effectively it's like we are keeping a weakref, but without the allocation and indirection overhead of an actual weakref object. Perhaps we should be keeping an actual weakref instead; it would take some experimentation to check the performance impact of that.