Change interaction between MemoryManager and KtObject

docs/src/doc/contribution/knowledge-base/memory-management.md

@@ -82,7 +82,7 @@ the counter of the C++ `RefCounted` and free it.
 
 ### Cyclical references
 
-`Wrapper` and `Script Instance` all holds references to each other, linking their lifetimes to each other.
+`Wrapper` and `Script Instance` can hold references to each other, linking their lifetimes.
 Even if the manager can freely switch between wrapper and script instance when necessary, this is not the case for regular user code.
 Someone can hold a reference to a wrapper when another part of the code is going to set a script on the native object, creating a script instance in the process.
-In such case, we need to make sure that this wrapper reference can also keep the script alive.
+In such case, we need to make sure that this older wrapper reference can also keep the new script alive.

kt/common/src/main/kotlin/godot/common/interop/NativePointer.kt

@@ -7,11 +7,6 @@ interface NativePointer {
     val ptr: VoidPtr
 }
 
-interface Binding {
-    val objectID: ObjectID
-    val instance: NativeWrapper?
-}
-
 interface NativeWrapper: NativePointer {
-    val memoryBinding: Binding
+    val objectID: ObjectID
 }

kt/godot-core-library/src/main/kotlin/godot/core/KtObject.kt

@@ -1,14 +1,12 @@
 package godot.core
 
-import godot.common.interop.Binding
 import godot.common.interop.NativeWrapper
 import godot.common.interop.ObjectID
 import godot.common.interop.VoidPtr
 import godot.common.interop.nullObjectID
 import godot.common.interop.nullptr
 import godot.internal.memory.MemoryManager
 import godot.internal.memory.TransferContext
-import godot.internal.memory.binding.GodotBinding
 import godot.internal.reflection.TypeManager
 import kotlin.contracts.ExperimentalContracts
 
@@ -31,15 +29,15 @@ abstract class KtObject : NativeWrapper {
     }
 
     final override val ptr: VoidPtr
-    final override val memoryBinding: Binding
+    final override val objectID: ObjectID
 
     init {
         val config = initConfig.get()
 
         if (config.ptr != nullptr) {
             // Native object already exists, so we know the id and ptr without going back to the other side.
             ptr = config.ptr
-            memoryBinding = GodotBinding.create(this, config.objectID)
+            objectID = config.objectID
             config.reset()
             // We don't need to register the instance to the MemoryManager, it is the responsibility of the caller.
         } else {
@@ -48,9 +46,9 @@ abstract class KtObject : NativeWrapper {
             new(TypeManager.userTypeToId[this::class] ?: -1)
             TransferContext.unsafeRead { buffer ->
                 ptr = buffer.getLong()
-                memoryBinding = GodotBinding.create(this, ObjectID(buffer.getLong()))
+                objectID = ObjectID(buffer.getLong())
             }
-            MemoryManager.registerNewNativeObject(memoryBinding as GodotBinding)
+            MemoryManager.registerNewNativeObject(this)
         }
 
     }
@@ -77,10 +75,10 @@ abstract class KtObject : NativeWrapper {
     }
 
     private fun removeScript(constructorIndex: Int) {
-        createScriptInstance(ptr, memoryBinding.objectID,  TypeManager.engineTypesConstructors[constructorIndex] as () -> KtObject)
+        createScriptInstance(ptr, objectID,  TypeManager.engineTypesConstructors[constructorIndex])
     }
 
-    override fun equals(other: Any?) = this === other || (other is KtObject && ptr == other.ptr)
+    override fun equals(other: Any?) = this === other || (other is KtObject && objectID == other.objectID)
 
     override fun hashCode() = ptr.toInt()
 
@@ -93,9 +91,9 @@ abstract class KtObject : NativeWrapper {
         }
 
         /** When using this constructor, the newly created instances doesn't register itself to the MemoryManager, the caller must do it.*/
-        inline fun <T : KtObject> createScriptInstance(rawPtr: VoidPtr, id: ObjectID, constructor: () -> T) = withConfig(rawPtr, id) {
+        inline fun <T : NativeWrapper> createScriptInstance(rawPtr: VoidPtr, id: ObjectID, constructor: () -> T) = withConfig(rawPtr, id) {
             val obj = constructor()
-            MemoryManager.registerExistingNativeObject(obj.memoryBinding as GodotBinding)
+            MemoryManager.registerExistingNativeObject(obj)
             obj
         }
 

kt/godot-internal-library/src/main/kotlin/godot/internal/memory/MemoryManager.kt

@@ -7,9 +7,10 @@ import godot.common.interop.ObjectID
 import godot.common.interop.NativePointer
 import godot.common.interop.VariantConverter
 import godot.common.interop.VoidPtr
-import godot.internal.memory.binding.GodotBinding
+import godot.internal.memory.binding.Binding
 import godot.internal.memory.binding.NativeCoreBinding
 import godot.internal.memory.binding.RefCountedBinding
+import java.lang.ref.WeakReference
 import java.util.concurrent.ConcurrentHashMap
 import java.util.concurrent.locks.ReentrantReadWriteLock
 import kotlin.concurrent.read
@@ -63,7 +64,10 @@ object MemoryManager {
     private val lock = ReentrantReadWriteLock()
 
     /** Pointers to Godot objects.*/
-    private val ObjectDB = HashMap<ObjectID, GodotBinding>(BINDING_INITIAL_CAPACITY)
+    private val ObjectDB = HashMap<ObjectID, Binding>(BINDING_INITIAL_CAPACITY)
+
+    /** Pointers to Godot objects.*/
+    private val refCountedLinks = HashMap<RefCountedBinding, NativeWrapper>(BINDING_INITIAL_CAPACITY)
 
     /** List of references to decrement.*/
     private val deadReferences = mutableListOf<RefCountedBinding>()
@@ -85,34 +89,32 @@ object MemoryManager {
     /**
      * The Godot native object has just been created. We can directly add it to ObjectDB because we know the slot is free.
      */
-    fun registerNewNativeObject(binding: GodotBinding) = lock.write {
-        ObjectDB[binding.objectID] = binding
-        binding
+    fun registerNewNativeObject(nativeWrapper: NativeWrapper) = lock.write {
+        ObjectDB[nativeWrapper.objectID] = Binding.create(nativeWrapper)
     }
 
     /**
      * Check if a native object already has a wrapper, if not, we create it.
      */
-    fun getInstanceOrCreate(id: ObjectID, constructor: () -> NativeWrapper): NativeWrapper {
-
-        return lock.read {
-            // We first try to get a match.
-            ObjectDB[id]?.instance
-        } ?: lock.write {
-            // Fallback to creating the wrapper.
-            // We check a second time in a write lock in case it got created after the read lock by another thread
-            ObjectDB[id]?.instance ?: constructor().also { ObjectDB[id] = it.memoryBinding as GodotBinding }
-        }
+    fun getInstanceOrCreate(id: ObjectID, constructor: () -> NativeWrapper) = lock.read {
+        // We first try to get a match.
+        ObjectDB[id]?.instance
+    } ?: lock.write {
+        // Fallback to creating the wrapper.
+        // We check a second time in a write lock in case it got created after the read lock by another thread
+        ObjectDB[id]?.instance ?: constructor().also { ObjectDB[id] = Binding.create(it) }
     }
 
     /**
-     * Create a script on top of an existing native object. We need additional checks to find if a twin exists.
+     * Create a script on top of an existing native object. It's usually called when adding/removing scripts. If RefCounted, we need to link the old and new instance together
      */
-    fun registerExistingNativeObject(binding: GodotBinding) = lock.write {
-        val oldBinding = ObjectDB.put(binding.objectID, binding)
+    fun registerExistingNativeObject(nativeWrapper: NativeWrapper) = lock.write {
+        val objectId = nativeWrapper.objectID
+        val oldBinding = ObjectDB.put(objectId, Binding.create(nativeWrapper))
 
-        if (oldBinding != null) {
-            oldBinding.instance = binding.instance
+        // If an old binding exist, it means that we added/removed a script and create a link between the 2 bindings.
+        if (oldBinding != null && objectId.isReference) {
+            refCountedLinks[oldBinding as RefCountedBinding] = nativeWrapper
         }
     }
 
@@ -123,7 +125,7 @@ object MemoryManager {
         ObjectDB.remove(ObjectID(id))
     }
 
-    fun isInstanceValid(ktObject: NativeWrapper) = checkInstance(ktObject.ptr, ktObject.memoryBinding.objectID.id)
+    fun isInstanceValid(ktObject: NativeWrapper) = checkInstance(ktObject.ptr, ktObject.objectID.id)
 
     fun registerNativeCoreType(nativeCoreType: NativePointer, variantType: VariantConverter) {
         val rawPtr = nativeCoreType.ptr
@@ -153,7 +155,7 @@ object MemoryManager {
     private fun getDeadReferences(): LongArray {
         // Pool all dead references first, so we can know the amount of work to do.
         while (true) {
-            val ref = ((GodotBinding.queue.poll() ?: break) as RefCountedBinding)
+            val ref = ((Binding.queue.poll() ?: break) as RefCountedBinding)
             deadReferences.add(ref)
         }
 
@@ -184,6 +186,9 @@ object MemoryManager {
                 val decrement = it === otherRef
                 if (decrement) {
                     ObjectDB.remove(objectID)
+                } else {
+                    // The binding is not in the ObjectDB, it can because it's a Twin of a more recent wrapper. In this case, we remove the link.
+                    refCountedLinks.remove(it)
                 }
                 decrement
             }

kt/godot-internal-library/src/main/kotlin/godot/internal/memory/binding/GodotBinding.kt

@@ -1,43 +1,39 @@
 package godot.internal.memory.binding
 
-import godot.common.interop.Binding
 import godot.common.interop.NativeWrapper
 import godot.common.interop.ObjectID
 import java.lang.ref.ReferenceQueue
 import java.lang.ref.WeakReference
 
-interface GodotBinding: Binding {
-    override var instance: NativeWrapper?
+interface Binding {
+    val instance: NativeWrapper?
 
     companion object {
-        val queue = ReferenceQueue<ObjectBinding>()
+        val queue = ReferenceQueue<NativeWrapper>()
 
-        fun create(instance: NativeWrapper, objectID: ObjectID): GodotBinding {
-            return if (objectID.isReference) {
-                RefCountedBinding(instance, objectID, queue)
+        fun create(instance: NativeWrapper): Binding {
+            return if (instance.objectID.isReference) {
+                RefCountedBinding(instance, queue)
             } else {
-                ObjectBinding(instance, objectID)
+                ObjectBinding(instance)
             }
         }
     }
 }
 
 class ObjectBinding(
-    override var instance: NativeWrapper?,
-    override val objectID: ObjectID
-) : GodotBinding
+    override val instance: NativeWrapper,
+) : Binding
 
 class RefCountedBinding(
     instance: NativeWrapper,
-    override val objectID: ObjectID,
-    queue: ReferenceQueue<ObjectBinding>,
-) : WeakReference<ObjectBinding>(ObjectBinding(instance, objectID), queue), GodotBinding {
-
-    override var instance: NativeWrapper?
-        get() = this.get()?.instance
-        set(value) {
-            this.get()?.instance = value
-        }
+    queue: ReferenceQueue<NativeWrapper>,
+) : WeakReference<NativeWrapper>(instance, queue), Binding {
+
+    val objectID: ObjectID = instance.objectID
+
+    override val instance: NativeWrapper?
+        get() = this.get()
 }
 
 

src/lifecycle/jvm_manager.cpp

@@ -4,7 +4,6 @@
 #include "jvm_wrapper/bridge/callable_bridge.h"
 #include "jvm_wrapper/bridge/dictionary_bridge.h"
 #include "jvm_wrapper/bridge/godot_print_bridge.h"
-#include "jvm_wrapper/bridge/jvm_stack_trace.h"
 #include "jvm_wrapper/bridge/lambda_callable_bridge.h"
 #include "jvm_wrapper/bridge/node_path_bridge.h"
 #include "jvm_wrapper/bridge/packed_array_bridge.h"