You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
New memory management system for SwiftGodot for Godot Objects. (#663)
* New memory management system for SwiftGodot for Godot Objects.
In this change we deal with proper ownership of objects in Godot, the
RefCounted objects that Godot releases via ref/unref operations, and
those that are queued for destruction.
To free objects:
* If you have a Node subclass, use `queueFree`
* If you have a RefCounted object, just clear your Swift Reference
* Otherwise, call `free`
In this change, we take advantage of Godot's system that notifies us
when a Godot object has been released so we clear our "handle", and
the `isValid` method that we have had for a while is now correct on
all cases.
It is important because otherwise we could have a situation where
Godot frees an object, but we still keep a pointer to it. And due to
how Godot's object allocation works, as Godot created new objects,
rather than a crash to an invalid memory address, we would just have a
pointer to the wrong object and things would seem to work.
To implement this, we also had to change an old SwiftGodot design
principle - in the past, we did not care if multiple Swift peers were
created for the underlying object, because we use to consider them
purely peers that could be recreated on demand. But now, when we take
advantage of Godot notifying us of the object being deallocated, we
would only deallocate one instance, not all the instances. So to fix
this, we now only surface one SwiftGodot object wrapper for a given
Godot Object.
Some special handling is done for different objects in the Godot
hierarchy. The way we now handle the lifecycle of Godot objects in
Swift is the following: If an object is not a RefCounted, we keep a
strong reference to it, so that only a call to the appropriate free
method can delete the object.
If an object is a RefCounted, we keep track of the
reference()/unreference() calls to it, and we keep a weak reference to
it if there are no references to it in Godot, otherwise we keep a
strong reference to it. This guarantees that the object can still be
deleted when no reference to it exists anymore, while making sure that
subtyped objects keep their state on the Swift side throughout the
Godot object's lifetime. As the Swift wrapper also increases the
reference count of the RefCounted, this means that once a RefCounted
is passed to the Swift side, that Godot object will always be
destroyed by the Swift wrapper eventually.
This also introduces a cute method called
`attemptToUseObjectFreedByGodot`, so that if you see it on a stack
trace, it is a clear indicator that this took place. This can happen
on code that kept references to long-term held objects, in those
cases, you must call `isValid` on the object.
The system now can toggle if we need to keep a strong reference to the
underlying object via the WrappedReference class, which can strongify
or weakify the changes.
While I did the initial work on making the objects be unique and
clearing of the handle, the real hard work and the correct tracking of
object ownership was done by Gabor Koncz and Gergely Kis at Migeran
who worked on all the various and difficult corner cases.
In particular the new object lifecycle is tracked like this:
- Every time godot returns a RefCounted object in a Ref<> wrapper for
a ptrcall, its reference count is incremeneted. As object identity
results in the return of the same Swift proxy for the same RefCounted
object, this means that every subsequent return should result in an
unreference() call, so that the existence of the Swift proxy object
always results in a single increment of the reference count.
- On the other hand, if a RefCounted object was returned through a
non-RefCounted static return type (e.g. as an Object), then Godot did
not increment its reference count. This means that on the first return
of such an object, the reference count should be incremented by a
reference() call, so that similarly the existence of the Swift proxy
object always results in a single increment of the reference count.
- The ownsRef parameter is true iff Godot can pass ownership of a
Ref<> wrapper to SwiftGodot, e.g. with a Ref<> return value of a
ptrcall.
This work was the result of using a large SwiftGodot-based application
on production through real life testing, where we slowly uncovered the
original design flaws and worked to fix each of those one by one.
That is why this commit is a bundle of fixes, as the original history
contains various attempts at solving the problem until the real
solution emerged.
* Incorporate feedback from Elijah Semyonov from #663
* I don't think that we should be using this teardown that goes behind the
back of the system.
* It is possible to call the extensionInterface, regardless of whether
the caller has provided an extensionInitCallback. This is the
scenario for the synthetic SwiftGodotTestability/GodotRuntime.
* For now, non-Refcounted, non-Node are released with free(), discussing whether this should be automatic in deinit
* Fix RefCounted initialization in case of failed cast. Bug #1024.
* Fix unregister order of inheritance chains
* Provide an API to externally terminate all the RefCounted objects
slated for destruction, so we can use it in the test suite. While
doing this, also optimized this process by queuing a bunch of
operations in one go, rather than create one Callable per release.
* Use cached values for public SwiftGodot
* Optimization, should go to main, do not create a new StringName for each use of the class
* We already own this StringName, so do not leak
* This was deadlocking, caught it once
* Second attempt to fix the deadlock
* Shorter version, also I think this fixes a leak
* Release pending objects before unregistering
* Adjust the validateProperty callback code after merging from main, to take into account the new memory usage pattern
* Add updated documentation
* Small doc update
* Follow my own guidance, which fixes one of the tests
* frameworkTypeBindingFree: By the time this method is called, accessing
the wrapped object's value returns nil, because this is why the deinit
for RefCounted was triggered by - so we can just use the instance
parameter directly.
This fixes a leak caused by these objects not being removed from our
internal table.
* Wrapped: reuse the callable, there is no need to create this for every deinit, hides the fact that creating a Callable is leaking
* Add more tests that I used while debugging
* Nasty crash: if we do not call `free` on the objects, Godot's debug
code to dump leaked objects attempts to dereference fields in the
object that have already been destroyed.
A proper fix is likely to add a callback to Godot to wipe those
objects from the tables when the extension is terminated. But at
least to get the test suite going, release our custom objects, rather
than leaking them.
---------
Co-authored-by: Roland Vigh <[email protected]>
Co-authored-by: Gabor Koncz <[email protected]>
return"guard let _result else { \(returnOptional ?"return nil":"fatalError (\"Unexpected nil return from a method that should never return nil\")") } ; return lookupObject (nativeHandle: _result)!"
673
+
return"guard let _result else { \(returnOptional ?"return nil":"fatalError (\"Unexpected nil return from a method that should never return nil\")") } ; return lookupObject (nativeHandle: _result, ownsRef: true)\(returnOptional ?"":"!")"
0 commit comments