diff --git a/docs/src/doc/contribution/knowledge-base/memory-management.md b/docs/src/doc/contribution/knowledge-base/memory-management.md index e67954136b..30cea154d9 100644 --- a/docs/src/doc/contribution/knowledge-base/memory-management.md +++ b/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. diff --git a/kt/common/src/main/kotlin/godot/common/interop/NativePointer.kt b/kt/common/src/main/kotlin/godot/common/interop/NativePointer.kt index 3d925b267e..53847e6b31 100644 --- a/kt/common/src/main/kotlin/godot/common/interop/NativePointer.kt +++ b/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 } diff --git a/kt/godot-core-library/src/main/kotlin/godot/core/KtObject.kt b/kt/godot-core-library/src/main/kotlin/godot/core/KtObject.kt index d8b139f27b..55f1a5a2fc 100644 --- a/kt/godot-core-library/src/main/kotlin/godot/core/KtObject.kt +++ b/kt/godot-core-library/src/main/kotlin/godot/core/KtObject.kt @@ -1,6 +1,5 @@ package godot.core -import godot.common.interop.Binding import godot.common.interop.NativeWrapper import godot.common.interop.ObjectID import godot.common.interop.VoidPtr @@ -8,7 +7,6 @@ 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,7 +29,7 @@ abstract class KtObject : NativeWrapper { } final override val ptr: VoidPtr - final override val memoryBinding: Binding + final override val objectID: ObjectID init { val config = initConfig.get() @@ -39,7 +37,7 @@ abstract class KtObject : NativeWrapper { 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 createScriptInstance(rawPtr: VoidPtr, id: ObjectID, constructor: () -> T) = withConfig(rawPtr, id) { + inline fun createScriptInstance(rawPtr: VoidPtr, id: ObjectID, constructor: () -> T) = withConfig(rawPtr, id) { val obj = constructor() - MemoryManager.registerExistingNativeObject(obj.memoryBinding as GodotBinding) + MemoryManager.registerExistingNativeObject(obj) obj } diff --git a/kt/godot-internal-library/src/main/kotlin/godot/internal/memory/MemoryManager.kt b/kt/godot-internal-library/src/main/kotlin/godot/internal/memory/MemoryManager.kt index 2b8bc10e5b..360a9540f0 100644 --- a/kt/godot-internal-library/src/main/kotlin/godot/internal/memory/MemoryManager.kt +++ b/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(BINDING_INITIAL_CAPACITY) + private val ObjectDB = HashMap(BINDING_INITIAL_CAPACITY) + + /** Pointers to Godot objects.*/ + private val refCountedLinks = HashMap(BINDING_INITIAL_CAPACITY) /** List of references to decrement.*/ private val deadReferences = mutableListOf() @@ -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 } diff --git a/kt/godot-internal-library/src/main/kotlin/godot/internal/memory/binding/GodotBinding.kt b/kt/godot-internal-library/src/main/kotlin/godot/internal/memory/binding/GodotBinding.kt index b58ed287b1..7eb624ccb4 100644 --- a/kt/godot-internal-library/src/main/kotlin/godot/internal/memory/binding/GodotBinding.kt +++ b/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() + val queue = ReferenceQueue() - 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, -) : WeakReference(ObjectBinding(instance, objectID), queue), GodotBinding { - - override var instance: NativeWrapper? - get() = this.get()?.instance - set(value) { - this.get()?.instance = value - } + queue: ReferenceQueue, +) : WeakReference(instance, queue), Binding { + + val objectID: ObjectID = instance.objectID + + override val instance: NativeWrapper? + get() = this.get() } diff --git a/src/lifecycle/jvm_manager.cpp b/src/lifecycle/jvm_manager.cpp index 5f7d7a0d28..43aa245013 100644 --- a/src/lifecycle/jvm_manager.cpp +++ b/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"