Updated memory management #543

samschott · 2024-11-19T22:26:50Z

This PR changes the memory management model of Rubicon. Previously, we would release objects on Python __del__ calls only if we created them ourselves and own them from alloc and similar calls.

In this PR, we always ensure that we own objects when we create a Python wrapper, by explicitly calling retain if we did not get them from alloc etc and always calling autorelease when the Python wrapper is garbage collected. This has a few advantages:

Users will no longer need to manually retain objects when receiving them from non-alloc methods and to release them before the Python object goes out of scope to avoid memory leaks.
Unless users manually release an object, Rubicon guarantees that a Python wrapper always points to an existing Objective-C object -- the one that it was created for.

This change should be backward compatible for most users because existing manual retain and release calls don't cause any issues if balanced and would have already caused segfaults if there are more releases than retains.

TODO:

Update docs.
Test with toga.
Figure out a decent transition plan for clients.

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

samschott · 2024-11-19T22:35:00Z

@mhsmith, care to have first look? Please note the TODOs still listed above.

freakboy3742

This turned out to be a lot less invasive than I thought it would be.

I've done a check of the toga-chart issue that triggered this change; and it seems to resolve that problem.

I also did a quick check with Toga's testbed suite on macOS; that code has a bunch of manual retains and autorelease/releases. I thought they should all be balanced though - worst case, objects would be over-retained - so I was a little surprised that it the testbed segfaults almost immediately (and the stack trace doesn't give any obvious pointers what is causing the issue).

If I remove all the retains and releases, the testbed segfaults; but on inspection, some of the uses are for objects that are created in Python, then handed to ObjC to manage (e.g., Toolbar items created here), or the copyWithZone handler here). But I guess those uses of memory handling make sense - and they're a lot closer aligned to the "spirit" of ObjC memory handling, Plus, in at least the ToolbarItem case, it could be avoided by keeping the toolbar instance in the cache of items.

freakboy3742 · 2024-11-20T06:13:20Z

Related - if we land this, I suspect a version bump to 0.5 might be called for. This is just backwards incompatible enough that I think it's worth flagging the significance of the change.

samschott · 2024-11-20T09:43:37Z

Thanks for the thorough checks! I've updated the PR description to give a better summary of the change and also discuss why this should be non-breaking for most users.

I'll have a closer look at the segfaults that you encountered, later. They might be caused by the usage of release instead of autorelease in __del__ which does not give Objective-C a chance to take over ownership.

Maybe there are also ways to prevent users from shooting themselves in the foot, e.g., raise an exception on manual release calls if there is only a single reference left.

mhsmith · 2024-11-20T13:40:29Z

Thanks, this looks great. I'm busy today, but I'll take a look at this as soon as I can.

samschott · 2024-11-23T11:52:54Z

I've had a closer look now at the segfaults and could identify two cases where they happen:

The object is released by Rubicon but still needed in ObjC, for example because it is being assigned to a property. Replacing release by autorelease in __del__ fixes this by giving ObjC a chance to take over ownership.
Toga has a few "stray" release or autorelease calls sprinkled throughout the codebase to manually clean up memory, for example because of point (1). This relies on Rubicon internally setting _needs_release = False after those calls and disabling its own cleanup logic. This no longer works and results in one too many release calls now.

beeware/toga#2978 contains all the changes that I found to (1) prevent segfaults and (2) remove now unneeded manual memory management.

mhsmith

I've added #539 and #48 to the Fixes list in the top comment.

docs/how-to/memory-management.rst

src/rubicon/objc/api.py

changes/256.bugfix.rst

src/rubicon/objc/api.py

tests/test_core.py

Co-authored-by: Malcolm Smith <[email protected]>

changes/256.removal.rst

src/rubicon/objc/api.py

changes/256.bugfix.rst

tests/test_core.py

samschott · 2024-11-27T22:01:24Z

Something that just occurred to me in reviewing beeware/toga#2978 in the context of this change - does this also resolve the NSImage "init fail" issue?

If we do things right, i.e., the way that ARC would handle it, it would indeed remove the need for special handling on the NSImage "init failed" issue.

but that mirrors the underlying ObjC behavior, so it strikes me as the sort of thing that can be documented.

Agreed, I think we would represent different objects by different Python wrappers instead of swapping out the pointer under the hood.

samschott · 2024-11-27T22:06:48Z

I've looked through ARC docs and found the relevant places to explain the init behavior: https://clang.llvm.org/docs/AutomaticReferenceCounting.html#semantics-of-method-families.

Basically, all methods in the alloc, copy, mutableCopy, new and init families return a retained object.

init is special because it consumes its self parameter. I.e., its takes ownership of it. This is what allows it to either (1) pass on ownership to the caller of init when returning or (2) release the object and return a different retained object.

Getting those semantics wrong seems to me the root cause of beeware/toga#2978.

freakboy3742 · 2024-11-27T23:36:18Z

I've looked through ARC docs and found the relevant places to explain the init behavior: https://clang.llvm.org/docs/AutomaticReferenceCounting.html#semantics-of-method-families.

Basically, all methods in the alloc, copy, mutableCopy, new and init families return a retained object.

init is special because it consumes its self parameter. I.e., its takes ownership of it. This is what allows it to either (1) pass on ownership to the caller of init when returning or (2) release the object and return a different retained object.

Nice find - that's a helpful reference. Probably worth adding a link to that in a docstring somewhere as well.

If I'm reading that right (and especially the "Rationale" block"):

We need to strip leading underscores before the check against _OWNERSHIP_METHOD_PREFIXES
There's an edge case where we should be looking for the objc_method_family attribute to determine if a method is in one of these special families
We are safe to assume "starts with init, takes an id, returns an id" is a method that needs to do an extra release

(1) seems like an edge case, but it's also not hard to accomodate. (2) also seems like an edge case, but one that's a little harder to implement, so if that was bumped to a "known limitation" with documentation and open feature ticket, I'd be OK with that.

freakboy3742 · 2024-11-27T23:55:34Z

Is there a particular reason you suggest patching the old instance, rather than creating a new instance?

Speaking with @mhsmith in person, he pointed out that if we create a new Python object for the init, the alloc Python object will continue to be a valid Python reference, but one that will be explosive if used. That's completely consistent with the ObjC behavior, but would be at surprising behavior to a Python user. Replacing the pointer reference on the existing object avoids this.

This is all true; my question is how much we care, balanced against the complexity and efficiency of the implementation.

Rubicon is a Python wrapper around ObjC; I don't think it's unreasonable to expect our users to need a basic understanding of ObjC, especially if we document these edge cases. "Don't use the alloc ptr once it's been init'd" is one of those edge cases. And the use case for keeping a pointer to the alloc independent of the init is niche at best.

The only thing that gives me pause is the performance aspect. If every .alloc().init() causes 2 Python objects to be created, that is going to have a direct performance overhead. If it will only be 2 Python objects for objects that change pointer, that's a lot less of a concern.

If the fix to rewrite the pointer for those cases is <10 lines of code with no real other risks, then we might as well take the win. But if it's more complex than that, or there's any question over whether there are edge cases in getting the caching right, then the simple solution of "just create 2 objects and document the limitation" is something I can live with.

samschott · 2024-11-28T00:09:04Z

Speaking with @mhsmith in person, he pointed out that if we create a new Python object for the init, the alloc Python object will continue to be a valid Python reference, but one that will be explosive if used.

That's a good point. Sending messages to an uninitialized object will segfault for most selectors (with a few useful exceptions such as init). But I don't think this pitfall can be avoided completely. If the user chooses to do a standalone alloc() instead of alloc().init(), they can still run use an uninitialized object.

Another option could be explicitly track / flag uninitialized objects.

The only thing that gives me pause is the performance aspect. If every .alloc().init() causes 2 Python objects to be created, that is going to have a direct performance overhead. If it will only be 2 Python objects for objects that change pointer, that's a lot less of a concern.

I am trying out a solution at the moment that would indeed only create 2 Python objects if the pointer changes, keeping with what ObjCInstances are supposed to represent. I am however still running into test failures in Python 3.12 + macOS 15 which are a bit concerning.

If I'm reading that right (and especially the "Rationale" block") [...]

I think you are reading this correctly. Though my first stab is certainly less sophisticated than that.

mhsmith · 2024-11-28T20:05:02Z

AIUI, your proposal is extra handling in ObjCBountMethod.__call__ that will be invoked if self.method.name.startswith("init")

I'm not familiar enough with Rubicon to suggest the best way to implement this, but yes, this is generally what I had in mind.

this logic will:

check that self.receiver (the object on which the method is invoked) is the same value as the return value of the method; 2. If it isn't the same pointer, __call__ will modify the ObjCInstance instance itself, and the ObjCInstance cache to reflect the new value, releasing the "old" version from the alloc

Nothing should be released, because the old pointer was already "consumed" by the init method.

Are there "init" methods that don't start with init?

It looks like this is answered by the specification Sam found, along the converse question of "are there methods that do start with init but shouldn't have special behavior".

does this also resolve the NSImage "init fail" issue? The return value from init will be different to the alloc'd object... so the alloc'd object should be released; the only catch is that we don't need to create the ObjCInstance because it's a None object.

Again, we shouldn't call release, because init has already done that. But we should set the ObjCInstance's internal pointer to NULL, and make sure we handle that NULL pointer in the future as follows:

Attempting to call a method on the object, or use its pointer in any other way, should throw an exception.
When the object's __del__ method is called, it should not be autoreleased.

mhsmith · 2024-11-28T20:16:31Z

We need to strip leading underscores before the check against _OWNERSHIP_METHOD_PREFIXES

It's a little more complicated than that:

A selector is in a certain selector family if, ignoring any leading underscores, the first component of the selector either consists entirely of the name of the method family or it begins with that name followed by a character other than a lowercase letter. For example, _perform:with: and performWith: would fall into the perform family (if we recognized one), but performing:with would not.

Or for a more realistic example, any method beginning with the string initialize is not in the init family.

The spec also gives a few other conditions for deciding whether a method is in a family, but I don't know whether it's possible to check them at runtime, or how likely they are to matter in practice.

samschott · 2024-11-28T22:45:11Z

The spec also gives a few other conditions for deciding whether a method is in a family, but I don't know whether it's possible to check them at runtime, or how likely they are to matter in practice.

I don't have enough experience with Objective-C to know this either, so I've implemented the name-base checks for now. Name-based rules seem sufficient for selectors families, which is required but not sufficient for the method family.

I've found what I believe is a relatively lightweight solution now that does not change ObjCInstance pointers under the hood and keeps the current semantics of "if we have Python wrapper, we Obj-C object must still exist". Admittedly, this object is much less useful when not yet initialized, but that's a problem that I don't want to solve for in this PR.

Why I like the current current solution:

It upholds clear promises in the API: A ObjCInstance instance always refers to the same wrapped Objective-C object and this object is never deallocated while we still have a Python wrapper. This will hopefully make it easier to reason about future issues.
It treats all methods that return a retained object ({"init", "alloc", "new", "copy", "mutableCopy"}) equally when putting in them cache.
It has special handling for init consuming its self argument.

samschott · 2024-11-28T22:59:00Z

src/rubicon/objc/api.py

-            if self.superclass.methods_ptr is None:
-                with self.superclass.cache_lock:
-                    self.superclass._load_methods()
+        # Traverse superclasses and load methods.


I am not entirely sure why a full traversal of superclasses is required now but wasn't previously for tests on Python 3.12 + macOS 15 to pass. But I do believe that a class hierarchy traversal makes sense regardless.

Agreed this is weird; I can only assume we're hitting some weird cache initialisation order thing (e.g., the test was previously initializing the super class before the class that was causing a problem). However, the solution here makes sense.

Running the toga testbed suite reveals a similar issue for TogaSlider.setValue(value, animated=True) where the superclass method cannot be found. This is despite the fix here and could be related to the TogaSlider being a subclass defined in Python.

Note that the direct method call TogaSlider.setValue_animated_(value, True) succeeds: https://github.com/beeware/toga/actions/runs/12089448592/job/33714837506?pr=2978

I've managed to fix that by allowing direct method lookup if there is no cached partial method, similar to what we already do for the old style syntax.

This is very likely a race condition somewhere, but the code around ObjCClass._load_methods, ObjCClass._cache_method and ObjCPartialMethod quite complex and hard to follow.

But all those changes to method lookup are making this PR a bit unwieldy. I've reverted some of the unneeded commits and am happy to split this off into an entirely different PR if that makes it easier.

I agree that the method lookup isn't strictly related to the memory retention issue, but I'm OK with the level of complexity it adds to this PR in the interest of addressing some issues that we know exist when this PR is used in Toga.

Well, something is still very fishy about my solution in this PR. _load_methods should have already been recursive, with the main difference that is was hard-stopping the recursion if one of the superclasses had already called _load_methods. This makes assumptions of the superclass chain not changing during init.

The new solution forces recursion to continue, but in a horribly hacky way. I do still want to find a more elegant solution here.

freakboy3742

This looks really good to me. A couple of comments inline, mostly about the comments :-)

src/rubicon/objc/api.py

freakboy3742 · 2024-11-28T23:45:52Z

src/rubicon/objc/api.py

-            if self.superclass.methods_ptr is None:
-                with self.superclass.cache_lock:
-                    self.superclass._load_methods()
+        # Traverse superclasses and load methods.


Agreed this is weird; I can only assume we're hitting some weird cache initialisation order thing (e.g., the test was previously initializing the super class before the class that was causing a problem). However, the solution here makes sense.

tests/test_core.py

changes/256.feature.rst

freakboy3742 · 2024-11-28T23:54:24Z

I've found what I believe is a relatively lightweight solution now that does not change ObjCInstance pointers under the hood and keeps the current semantics of "if we have Python wrapper, we Obj-C object must still exist". Admittedly, this object is much less useful when not yet initialized, but that's a problem that I don't want to solve for in this PR.

Why I like the current current solution:

Agreed - I like the solution you've presented here. It may still have some interesting behavior if you try to use an alloc'd object... but then, so will ObjC, so it shouldn't be surprising.

Co-authored-by: Russell Keith-Magee <[email protected]>

This reverts commit c6096c2.

This reverts commit 2e4eccb. # Conflicts: # src/rubicon/objc/api.py

This reverts commit b78c41d.

freakboy3742

I'm happy with this PR as it stands now; in addition to solving the originally reported problem (beeware/toga-chart#191), it solves the bigger problem of memory management (#256) problem; it addresses the remaining iOS leakage issues (see beeware/toga#2853); and apparently also fixes a method caching issue that has been unreported to date.

I've left one comment about the argument used when constructing arguments, but that's a minor cleanup suggestion, and one that I'll gladly back down from if you disagrees.

Before we merge, I'd also like @mhsmith's final review in case he can think of any edge cases or other issues I might have missed.

src/rubicon/objc/api.py

freakboy3742 · 2024-12-02T04:52:14Z

src/rubicon/objc/api.py

-            if self.superclass.methods_ptr is None:
-                with self.superclass.cache_lock:
-                    self.superclass._load_methods()
+        # Traverse superclasses and load methods.


I agree that the method lookup isn't strictly related to the memory retention issue, but I'm OK with the level of complexity it adds to this PR in the interest of addressing some issues that we know exist when this PR is used in Toga.

samschott added 3 commits November 19, 2024 23:44

Retain on ObjCInstance creation, autorelease on __del__

9abcd0d

update tests

6605842

add change note

931c352

samschott force-pushed the updated-memory-management branch from 89a4d62 to 931c352 Compare November 19, 2024 22:44

freakboy3742 reviewed Nov 20, 2024

View reviewed changes

freakboy3742 mentioned this pull request Nov 20, 2024

Force cache eviction when instance classes don't match. #540

Closed

4 tasks

samschott added 3 commits November 20, 2024 21:11

use autorelease instead of release in __del__

a618f2a

code formatting

b1bf61c

update docs

21f2e0b

samschott marked this pull request as ready for review November 21, 2024 01:12

samschott added 2 commits November 23, 2024 12:42

add comment about autorelease vs release

20ab8f9

remove now unneeded cache staleness check

160c819

samschott mentioned this pull request Nov 23, 2024

Rubicon update beeware/toga#2978

Draft

remove stale instance cache tests

ce9d78c

mhsmith reviewed Nov 24, 2024

View reviewed changes

samschott and others added 7 commits November 24, 2024 21:42

update test_objcinstance_dealloc

6d89330

correct inline comment

3bb7ccc

Co-authored-by: Malcolm Smith <[email protected]>

make returned_from_method private

c0b091c

update ObjCInstance doc string

ab1f762

updated docs

544d694

update spellchecker

b4a1624

update change notes with migration instructions

f0edb5b

samschott force-pushed the updated-memory-management branch from 6648cd0 to f0edb5b Compare November 24, 2024 21:52

mhsmith reviewed Nov 25, 2024

View reviewed changes

changes/256.removal.rst Outdated Show resolved Hide resolved

src/rubicon/objc/api.py Outdated Show resolved Hide resolved

changes/256.bugfix.rst Outdated Show resolved Hide resolved

mhsmith reviewed Nov 25, 2024

View reviewed changes

tests/test_core.py Outdated Show resolved Hide resolved

tests/test_core.py Outdated Show resolved Hide resolved

tests/test_core.py Outdated Show resolved Hide resolved

special handling for init

84e3a9f

add tests for init object change

ab46b9d

samschott added 2 commits November 28, 2024 22:12

implement proper method family detection

2305122

ensure partial methods are loaded from all superclasses

2e4eccb

samschott commented Nov 28, 2024

View reviewed changes

freakboy3742 requested changes Nov 28, 2024

View reviewed changes

samschott and others added 12 commits November 29, 2024 19:13

remove unneeded whitespace

2989540

Co-authored-by: Russell Keith-Magee <[email protected]>

improved release-on-cache-hit documentation

0bc749c

updated change notes

04981e3

add test for get_method_family

54ed55c

remove loop that breaks early on method loading

c6096c2

make method loading slightly clearer

a28c901

extract and document method name to tuple logic

1b306e2

fall back to full method usage if partial method lookup fails

849749e

update partial method cache after successful lookup

15593c1

Revert "remove loop that breaks early on method loading"

39c548e

This reverts commit c6096c2.

Revert "ensure partial methods are loaded from all superclasses"

b78c41d

This reverts commit 2e4eccb. # Conflicts: # src/rubicon/objc/api.py

Reapply "ensure partial methods are loaded from all superclasses"

cb996ad

This reverts commit b78c41d.

freakboy3742 approved these changes Dec 2, 2024

View reviewed changes

centralize logic for method family

6cf88a5

freakboy3742 mentioned this pull request Dec 3, 2024

Resizing window of example app breaks layout and generates TypeError beeware/toga-chart#191

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated memory management #543

Updated memory management #543

samschott commented Nov 19, 2024 •

edited by freakboy3742

Loading

samschott commented Nov 19, 2024

freakboy3742 left a comment

freakboy3742 commented Nov 20, 2024

samschott commented Nov 20, 2024 •

edited

Loading

mhsmith commented Nov 20, 2024

samschott commented Nov 23, 2024 •

edited

Loading

mhsmith left a comment

samschott commented Nov 27, 2024

samschott commented Nov 27, 2024

freakboy3742 commented Nov 27, 2024

freakboy3742 commented Nov 27, 2024

samschott commented Nov 28, 2024 •

edited

Loading

mhsmith commented Nov 28, 2024 •

edited

Loading

mhsmith commented Nov 28, 2024

samschott commented Nov 28, 2024 •

edited

Loading

samschott Nov 28, 2024 •

edited

Loading

freakboy3742 Nov 28, 2024

samschott Nov 29, 2024 •

edited

Loading

samschott Nov 29, 2024 •

edited

Loading

samschott Nov 29, 2024

samschott Nov 30, 2024

freakboy3742 Dec 2, 2024

samschott Dec 2, 2024 •

edited

Loading

freakboy3742 left a comment

freakboy3742 Nov 28, 2024

freakboy3742 commented Nov 28, 2024

freakboy3742 left a comment

freakboy3742 Dec 2, 2024

Updated memory management #543

Are you sure you want to change the base?

Updated memory management #543

Conversation

samschott commented Nov 19, 2024 • edited by freakboy3742 Loading

PR Checklist:

samschott commented Nov 19, 2024

freakboy3742 left a comment

Choose a reason for hiding this comment

freakboy3742 commented Nov 20, 2024

samschott commented Nov 20, 2024 • edited Loading

mhsmith commented Nov 20, 2024

samschott commented Nov 23, 2024 • edited Loading

mhsmith left a comment

Choose a reason for hiding this comment

samschott commented Nov 27, 2024

samschott commented Nov 27, 2024

freakboy3742 commented Nov 27, 2024

freakboy3742 commented Nov 27, 2024

samschott commented Nov 28, 2024 • edited Loading

mhsmith commented Nov 28, 2024 • edited Loading

mhsmith commented Nov 28, 2024

samschott commented Nov 28, 2024 • edited Loading

samschott Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

freakboy3742 Nov 28, 2024

Choose a reason for hiding this comment

samschott Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

samschott Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

samschott Nov 29, 2024

Choose a reason for hiding this comment

samschott Nov 30, 2024

Choose a reason for hiding this comment

freakboy3742 Dec 2, 2024

Choose a reason for hiding this comment

samschott Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

freakboy3742 left a comment

Choose a reason for hiding this comment

freakboy3742 Nov 28, 2024

Choose a reason for hiding this comment

freakboy3742 commented Nov 28, 2024

freakboy3742 left a comment

Choose a reason for hiding this comment

freakboy3742 Dec 2, 2024

Choose a reason for hiding this comment

samschott commented Nov 19, 2024 •

edited by freakboy3742

Loading

samschott commented Nov 20, 2024 •

edited

Loading

samschott commented Nov 23, 2024 •

edited

Loading

samschott commented Nov 28, 2024 •

edited

Loading

mhsmith commented Nov 28, 2024 •

edited

Loading

samschott commented Nov 28, 2024 •

edited

Loading

samschott Nov 28, 2024 •

edited

Loading

samschott Nov 29, 2024 •

edited

Loading

samschott Nov 29, 2024 •

edited

Loading

samschott Dec 2, 2024 •

edited

Loading