Replace/remove the DeallocationObserver mechanism

[dgelessus]

Currently, every Objective-C object that is wrapped in ObjCInstance by Rubicon has a DeallocationObserver object associated with it. DeallocationObserver is a custom Objective-C class defined by Rubicon, which is used to notify Rubicon when the Objective-C object it's attached to is deallocated. When that happens, the object's ObjCInstance is removed from the global ObjCInstance._cached_objects cache.

This mechanism has two main disadvantages:

• It requires creating a new Objective-C object from Python whenever an Objective-C object is wrapped in ObjCInstance for the first time. This has a noticeable performance impact when creating many Objective-C objects from Python - see Speed up instantiating new class instances #183.
• It uses the Objective-C runtime's "associated objects" mechanism. There's nothing directly wrong with this - it's a public, documented, mature, non-deprecated API - but it is a relatively obscure feature of the runtime, and general advice is that it should be considered a last-resort solution.
  • Also, the GNUStep Objective-C runtime doesn't support associated objects. GNUStep support isn't a very high priority and would also require some other changes, but the fact that Rubicon's core makes heavy use of associated objects is actually one of the major blockers for GNUStep compatibility - see Add support for GNUStep #176.

The question is how we could replace this mechanism. I don't know of any better way to implement the exact behavior that DeallocationObserver currently provides, i. e. notifying Rubicon when an Objective-C object is deallocated. But there are other ways to implement what we use this mechanism for, namely removing no longer needed entries from the ObjCInstance cache. For example, we could replace the cache dictionary with a weakref.WeakValueDictionary, so that entries would automatically disappear as soon as the ObjCInstance in question is deallocated on the Python side.

A consequence of using a weak dictionary as the cache would be that it's no longer guaranteed that wrapping the same Objective-C object will always return the same ObjCInstance. It's only guaranteed that if an ObjCInstance exists for an Objective-C object, wrapping the same object again will return the same ObjCInstance. But once the last reference to that ObjCInstance (outside the cache) is deleted, the ObjCInstance is probably deallocated, so wrapping the same Objective-C object again in the future will probably return a new ObjCInstance.

This could be a problem if the ObjCInstance had Python attributes attached to it by the user, because they would be lost when the ObjCInstance is destroyed and recreated from only the Objective-C object information. I don't know how common this case is, i. e. that an ObjCInstance has Python objects attached to it, has no references from the Python side, but the code expects the Python attributes to still be there when the Objective-C object is wrapped in an ObjCInstance again in the future.

A possible solution for that might be to make the ObjCInstance cache weak, but keep an additional collection of strong references to ObjCInstances that have custom Python attributes attached. That way only ObjCInstances with custom Python attributes need to have a DeallocationObserver, to remove them from the strong reference collection once the corresponding Objective-C object is deallocated. This would allow creating the DeallocationObserver only once it's actually needed, i. e. when the first custom Python attribute is added, instead of immediately when the object is first wrapped. Most ObjCInstances never have custom Python attributes added, so in many cases this would avoid the creation of a DeallocationObserver entirely.

[freakboy3742]

FWIW, I don't think 100% adherence to the current behavior is necessary. Having perfect lifecycle correspondence with a DeallocationObserver is certainly a nice trick, but in practice, I'm not we rely on it - and as you've pointed out, it has a performance overhead to maintain.

However, beyond that... I'm not sure what to suggest. I think the analysis you've provided on using WeakReferences as an alternative is pretty much correct. However, I'm not sure how much having the same literal ObjCInstance will matter. I can think of a couple of places in Toga where we're using the ObjCInstance as a cache key... but if we're doing that, presumably we'd also be preserving the reference in the WeakValueDictionary, so we wouldn't be affected by the "new instance" problem you describe.

I can see how your suggestion about an additional collection of strong references - although, at that point, it sounds like we're getting dangerously close to re-inventing manual retain/release for objects. That's not necessarily the end of the world... it's just an odd parallel.

[dgelessus]

You're right that any code that cares about the exact identity of an ObjCInstance will probably keep a reference to the ObjCInstance anyway, so there's no risk of it being de-/reallocated in that case. What I'm worried about is code like this:

class TogaTextFieldDelegate(NSObject):
    @objc_method
    def controlTextDidChange_(self, notification) -> None:
        if self.interface.on_change:
            self.interface.on_change(self.interface)


class TextInput(Widget):
    def create(self):
        self.native = NSTextField.new()
        self.native.interface = self.interface

        delegate = TogaTextFieldDelegate.new()
        delegate.interface = self.interface
        self.native.delegate = delegate

This works fine with our current implementation, but will probably no longer work if we make the cache a weak dictionary and drop DeallocationObserver entirely. The problem here is that the delegate.interface attribute is stored only on the Python side, meaning that when delegate is no longer referenced on the Python side (i. e. when TextInput.create returns), the ObjCInstance is destroyed and the interface attribute is lost. When in the future the delegate method is called, Rubicon creates a new ObjCInstance for the Objective-C delegate object, but that ObjCInstance won't have the interface attribute that the method expects, so the delegate method will fail.

The simplest solution from Rubicon's perspective would be to require users to manually keep an extra reference to the ObjCInstance in cases like this, to prevent the Python object and attributes from "getting lost". This isn't very user-friendly though, and bugs caused by not keeping the required extra reference might be very difficult to reproduce and debug, because Python object deallocation doesn't always happen at predictable moments.

The proper solution for this problem would probably be a mechanism for wrapping Python object pointers in Objective-C objects, so that Python objects could be stored in actual Objective-C properties. That way it would be safe to destroy and later recreate ObjCInstances without losing the Python objects stored on them.

[samschott]

I was just investigating what appeared to be an issue in toga-cocoa with widgets not being destroyed when they should be (beeware/toga#1215) and stumbled on this conversation which may be related.

In toga-cocoa, it is common place to assign Python attributes _impl and interface to every ObjCInstance which represents a "widget" which therefore always use the DeallocationObserver mechanism. The interface, which is the top-level toga widget, will therefore always have a reference stored in the ObjCInstance which in turn has a reference stored in ObjCInstance._keep_alive_objects.

I don't really understand however under which circumstances an ObjCInstance instance which is kept alive this way will be destroyed. AFAIK, the corresponding Objective-C object will never be deallocated because we do not have automatic reference counting. This was exactly what we tried to solve with #201. But the __del__ method of the Python instance will never be called since the instance is kept alive...

Am I missing something here?

[samschott]

Maybe to give a more concrete example, what is the expected behaviour from the following code?

from rubicon.objc import NSObject

obj = NSObject.alloc().init()
obj.py_attr = "some string"
del obj

At the moment, it looks like obj is not destroyed even though it should be.

[dgelessus]

Good observation, I think you're right. Objects stored in ObjCInstance._keep_alive_objects are kept there permanently until they are destroyed on the Objective-C side, at which point the DeallocationObserver also removes the corresponding ObjCInstance on the Python side. As you've pointed out, this doesn't work well with our new automatic reference management - Rubicon now automatically retains Objective-C objects as long as they are referenced from Python, and an object with Python attributes will be kept alive in Python as long as it exists on the Objective-C side, meaning that such objects are never released automatically.

As a workaround, you should be able to manually release the object in question when you know for certain that it won't be used anymore on the Python side (and that no custom methods are called on it that need to use its Python attributes). Then the Objective-C object will be properly destroyed once it's no longer referenced from Objective-C, and then the DeallocationObserver will also remove the keep-alive reference on the Python side.

Of course this isn't a good long-term solution. I think this problem would go away if we implement a proper mechanism for storing Python object references on the Objective-C side (as I described in another comment above), because then ObjCInstances with extra Python attributes no longer need to be kept alive, so __del__ will run at the expected time. But I haven't gotten around to actually implementing this solution yet, so I'm not completely certain that it would actually fix the issue - it might lead to some other reference cycle problems that I haven't thought of.

[samschott]

I agree, storing the Python object reference on the Objective-C side would be best. It will however require careful memory management to keep the Pyhon object itself alive when it's only reference is on the Objective-C side.

I've had a look at what PyObjC does and it appears like they use a very elaborate method to wrap Python objects in Object-C objects:

• Wrapping the Python object: https://github.com/ronaldoussoren/pyobjc/blob/master/pyobjc-core/Modules/objc/OC_PythonObject.m
• Maintaining a registry of wrapped objects: https://github.com/ronaldoussoren/pyobjc/blob/efecb479c3cf32ec1d36abfb9b7d7c8cfd4ede6d/pyobjc-core/Modules/objc/proxy-registry.m)

It is the OC_PythonObject which is stored on the Objective-C side that handles reference management.

As for creating the property on the Objective-C object, it is probably best to use objc_setAssociatedObject since instance variables cannot be added to existing instances.

To sum up, it will be some amount of work. But I think it's worth doing since this will result in more reasonable memory usage in toga-cocoa.