diff --git a/godot-core/src/builder/method.rs b/godot-core/src/builder/method.rs index b4fe33b4b..adb618014 100644 --- a/godot-core/src/builder/method.rs +++ b/godot-core/src/builder/method.rs @@ -117,7 +117,7 @@ macro_rules! impl_code_method { $( let $arg = <$Param as sys::GodotFfi>::from_sys(*args.offset(idx)); - // FIXME update refcount, e.g. Gd::ready() or T::DynMemory::maybe_inc_ref(&result); + // FIXME update refcount, e.g. Gd::ready() or result.raw.refc_inc(); // possibly in from_sys() directly; what about from_sys_init() and from_{obj|str}_sys()? idx += 1; )* diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index 23935bd9b..d747f8dad 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -13,8 +13,7 @@ use sys::{ExtVariantType, GodotFfi, ffi_methods}; use crate::builtin::{AnyArray, CowStr, StringName, Variant, inner}; use crate::meta::{GodotType, ToGodot}; -use crate::obj::bounds::DynMemory; -use crate::obj::{Bounds, Gd, GodotClass, InstanceId, Singleton}; +use crate::obj::{Gd, GodotClass, InstanceId, Singleton}; use crate::{classes, meta}; #[cfg(before_api = "4.3")] @@ -418,13 +417,13 @@ impl Callable { let mut object = self.as_inner().get_object()?; // get_object() may return pointer to already-freed object -> return None for dead objects instead of accessing one in - // maybe_inc_ref(). Best-effort: the object can still be freed between this check and the inc-ref, like in Signal::object(). + // refc_inc(). Best-effort: the object can still be freed between this check and the inc-ref, like in Signal::object(). if !object.is_instance_valid() { return None; } // `InnerCallable::get_object` doesn't increment the refcount, so do it here in case the object is ref-counted. - ::DynMemory::maybe_inc_ref(&mut object.raw); + object.raw.refc_inc(); Some(object) } diff --git a/godot-core/src/builtin/signal.rs b/godot-core/src/builtin/signal.rs index 5e66bc618..4440c2ec4 100644 --- a/godot-core/src/builtin/signal.rs +++ b/godot-core/src/builtin/signal.rs @@ -16,8 +16,7 @@ use crate::classes::object::ConnectFlags; use crate::global::Error; use crate::meta; use crate::meta::{FromGodot, GodotType, ToGodot}; -use crate::obj::bounds::DynMemory; -use crate::obj::{Bounds, EngineBitfield, Gd, GodotClass, InstanceId}; +use crate::obj::{EngineBitfield, Gd, GodotClass, InstanceId}; use crate::signal::store_custom_callable_connection; /// Untyped Godot signal. @@ -144,12 +143,12 @@ impl Signal { // `get_object()` may hand out a pointer to an already-freed object (e.g. when the object was destroyed on another thread). // Validate liveness before touching the instance, honoring this method's contract to return `None` for dead objects. Without this, - // `maybe_inc_ref()` below would access the freed instance and panic -- fatal if it happens during `Drop`, see `FallibleSignalFuture`. + // `refc_inc()` below would access the freed instance and panic -- fatal if it happens during `Drop`, see `FallibleSignalFuture`. if !object.is_instance_valid() { return None; } - ::DynMemory::maybe_inc_ref(&mut object.raw); + object.raw.refc_inc(); Some(object) } diff --git a/godot-core/src/obj/bounds.rs b/godot-core/src/obj/bounds.rs index 5dca613ef..fac7d9bd2 100644 --- a/godot-core/src/obj/bounds.rs +++ b/godot-core/src/obj/bounds.rs @@ -52,7 +52,7 @@ use private::Sealed; use crate::obj::cap::GodotDefault; use crate::obj::{Bounds, Gd, GodotClass, RawGd}; use crate::storage::{InstanceCache, Storage}; -use crate::{out, sys}; +use crate::sys; // ---------------------------------------------------------------------------------------------------------------------------------------------- // Sealed trait @@ -162,40 +162,15 @@ pub trait Memory: Sealed { /// Specifies the memory strategy of the dynamic type. /// /// For `Gd`, it is determined at runtime whether the instance is manually managed or ref-counted. +/// +/// This trait only answers *whether* an object is ref-counted; the ref-counting operations themselves are the same in all cases and +/// live on [`RawGd`]. For `MemRefCounted`/`MemManual`, the answer is known at compile-time and can be optimized. #[doc(hidden)] pub trait DynMemory: Sealed { - /// Initialize reference counter - #[doc(hidden)] - fn maybe_init_ref(obj: &mut RawGd); - - /// If ref-counted, then increment count - #[doc(hidden)] - fn maybe_inc_ref(obj: &mut RawGd); - - /// If ref-counted, then decrement count. Returns `true` if the count hit 0 and the object can be - /// safely freed. - /// - /// This behavior can be overriden by a script, making it possible for the function to return `false` - /// even when the reference count hits 0. This is meant to be used to have a separate reference count - /// from Godot's internal reference count, or otherwise stop the object from being freed when the - /// reference count hits 0. - /// - /// # Safety - /// - /// If this method is used on a [`Gd`] that inherits from [`RefCounted`](crate::classes::RefCounted) - /// then the reference count must either be incremented before it hits 0, or some [`Gd`] referencing - /// this object must be forgotten. - #[doc(hidden)] - unsafe fn maybe_dec_ref(obj: &mut RawGd) -> bool; - - /// Check if ref-counted, return `None` if information is not available (dynamic and obj dead) + /// Check if ref-counted, return `None` if information is not available (dynamic and obj dead). #[doc(hidden)] fn is_ref_counted(obj: &RawGd) -> Option; - /// Return the reference count, or `None` if the object is dead or manually managed. - #[doc(hidden)] - fn get_ref_count(obj: &RawGd) -> Option; - /// Returns `true` if argument and return pointers are passed as `Ref` pointers given this /// [`PtrcallType`]. /// @@ -214,65 +189,10 @@ impl Memory for MemRefCounted { const IS_REF_COUNTED: bool = true; } impl DynMemory for MemRefCounted { - fn maybe_init_ref(obj: &mut RawGd) { - out!(" MemRefc::init: {obj:?}"); - if obj.is_null() { - return; - } - - // SAFETY: DynMemory=MemRefCounted statically guarantees T inherits RefCounted. - unsafe { - obj.with_ref_counted_unchecked(|refc| { - let success = refc.init_ref(); - assert!(success, "init_ref() failed"); - }) - }; - } - - fn maybe_inc_ref(obj: &mut RawGd) { - out!(" MemRefc::inc: {obj:?}"); - if obj.is_null() { - return; - } - - // SAFETY: DynMemory=MemRefCounted statically guarantees T inherits RefCounted. - unsafe { - obj.with_ref_counted_unchecked(|refc| { - let success = refc.reference(); - assert!(success, "reference() failed"); - }) - }; - } - - unsafe fn maybe_dec_ref(obj: &mut RawGd) -> bool { - out!(" MemRefc::dec: {obj:?}"); - if obj.is_null() { - return false; - } - - // SAFETY: DynMemory=MemRefCounted statically guarantees T inherits RefCounted. - unsafe { - obj.with_ref_counted_unchecked(|refc| { - let is_last = refc.unreference(); - out!(" +-- was last={is_last}"); - is_last - }) - } - } - fn is_ref_counted(_obj: &RawGd) -> Option { Some(true) } - fn get_ref_count(obj: &RawGd) -> Option { - // SAFETY: DynMemory=MemRefCounted statically guarantees T inherits RefCounted. - let ref_count = - unsafe { obj.with_ref_counted_unchecked(|refc| refc.get_reference_count()) }; - - // TODO find a safer cast alternative, e.g. num-traits crate with ToPrimitive (Debug) + AsPrimitive (Release). - Some(ref_count as usize) - } - fn pass_as_ref(call_type: sys::PtrcallType) -> bool { matches!(call_type, sys::PtrcallType::Virtual) } @@ -282,61 +202,12 @@ impl DynMemory for MemRefCounted { /// This is used only for `Object` classes. #[doc(hidden)] pub struct MemDynamic {} -impl MemDynamic { - /// Check whether dynamic type is ref-counted. - fn inherits_refcounted(obj: &RawGd) -> bool { - obj.instance_id_unchecked() - .is_some_and(|id| id.is_ref_counted()) - } -} impl Sealed for MemDynamic {} impl DynMemory for MemDynamic { - fn maybe_init_ref(obj: &mut RawGd) { - out!(" MemDyn::init: {obj:?}"); - if Self::inherits_refcounted(obj) { - // Will call `RefCounted::init_ref()` which checks for liveness. - out!(" MemDyn -> MemRefc"); - MemRefCounted::maybe_init_ref(obj) - } else { - out!(" MemDyn -> MemManu"); - } - } - - fn maybe_inc_ref(obj: &mut RawGd) { - out!(" MemDyn::inc: {obj:?}"); - if Self::inherits_refcounted(obj) { - // Will call `RefCounted::reference()` which checks for liveness. - MemRefCounted::maybe_inc_ref(obj) - } - } - - unsafe fn maybe_dec_ref(obj: &mut RawGd) -> bool { - unsafe { - out!(" MemDyn::dec: {obj:?}"); - if obj - .instance_id_unchecked() - .is_some_and(|id| id.is_ref_counted()) - { - // Will call `RefCounted::unreference()` which checks for liveness. - MemRefCounted::maybe_dec_ref(obj) - } else { - false - } - } - } - fn is_ref_counted(obj: &RawGd) -> Option { - // Return `None` if obj is dead + // Return `None` if obj is dead. The instance ID carries a ref-countedness bit, so this needs no FFI call. obj.instance_id_unchecked().map(|id| id.is_ref_counted()) } - - fn get_ref_count(obj: &RawGd) -> Option { - if Self::inherits_refcounted(obj) { - MemRefCounted::get_ref_count(obj) - } else { - None - } - } } /// No memory management, user responsible for not leaking. @@ -347,17 +218,9 @@ impl Memory for MemManual { const IS_REF_COUNTED: bool = false; } impl DynMemory for MemManual { - fn maybe_init_ref(_obj: &mut RawGd) {} - fn maybe_inc_ref(_obj: &mut RawGd) {} - unsafe fn maybe_dec_ref(_obj: &mut RawGd) -> bool { - false - } fn is_ref_counted(_obj: &RawGd) -> Option { Some(false) } - fn get_ref_count(_obj: &RawGd) -> Option { - None - } } // ---------------------------------------------------------------------------------------------------------------------------------------------- diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 04cfb5757..7cf43bc54 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -1007,12 +1007,9 @@ where /// } /// ``` pub fn try_to_unique(self) -> Result { - use crate::obj::bounds::DynMemory as _; - - match ::DynMemory::get_ref_count(&self.raw) { - Some(1) => Ok(self), - Some(ref_count) => Err((self, ref_count)), - None => unreachable!(), + match self.raw.ref_count() { + 1 => Ok(self), + ref_count => Err((self, ref_count)), } } } diff --git a/godot-core/src/obj/raw_gd.rs b/godot-core/src/obj/raw_gd.rs index 0bf619509..caac0f463 100644 --- a/godot-core/src/obj/raw_gd.rs +++ b/godot-core/src/obj/raw_gd.rs @@ -86,10 +86,10 @@ impl RawGd { /// Returns `self` but with initialized ref-count. fn with_inc_refcount(mut self) -> Self { - // Note: use init_ref and not inc_ref, since this might be the first reference increment. + // Note: use refc_init() and not refc_inc(), since this might be the first reference increment. // Godot expects RefCounted::init_ref to be called instead of RefCounted::reference in that case. // init_ref also doesn't hurt (except 1 possibly unnecessary check). - T::DynMemory::maybe_init_ref(&mut self); + self.refc_init(); self } @@ -240,7 +240,7 @@ impl RawGd { /// Unlike [`try_with_ref_counted`](Self::try_with_ref_counted), this does **not** check the type at runtime. /// /// # Safety - /// Caller must guarantee that `T` (statically) inherits from `RefCounted`. + /// Caller must guarantee that `T` inherits from `RefCounted`. pub(crate) unsafe fn with_ref_counted_unchecked( &self, apply: impl FnOnce(&mut classes::RefCounted) -> R, @@ -261,6 +261,65 @@ impl RawGd { apply(borrow.as_target_mut()) } + /// Whether this object is non-null and ref-counted, i.e. the `refc_*` operations below have an effect. + /// + /// For `T=Object` this is a runtime query; for all other classes it's known statically and the branch compiles away. + fn is_ref_counted(&self) -> bool { + !self.is_null() && T::DynMemory::is_ref_counted(self) == Some(true) + } + + /// Sets the reference count to 1, when this `RawGd` becomes the object's first ref; see Godot's `RefCounted::init_ref()`. + /// + /// Use [`refc_inc()`][Self::refc_inc] instead if the object is already referenced. + /// + /// No-op if `T` is not ref-counted. Panics if the object is already being destroyed (Godot's `bool` return is asserted). + fn refc_init(&mut self) { + if !self.is_ref_counted() { + return; + } + + out!(" RawGd::refc_init: {self:?}"); + + // SAFETY: object is ref-counted, as checked above. + let success = unsafe { self.with_ref_counted_unchecked(|refc| refc.init_ref()) }; + assert!(success, "init_ref() failed"); + } + + /// Increments the reference count, for objects which are already referenced. + /// + /// No-op if `T` is not ref-counted. + pub(crate) fn refc_inc(&mut self) { + if !self.is_ref_counted() { + return; + } + + out!(" RawGd::refc_inc: {self:?}"); + + // SAFETY: object is ref-counted, as checked above. + let success = unsafe { self.with_ref_counted_unchecked(|refc| refc.reference()) }; + assert!(success, "reference() failed"); + } + + /// Decrements the reference count. Returns `true` if the count hit 0 and the object can be safely freed. + /// + /// No-op returning `false` if `T` is not ref-counted. A script can override `unreference()`, so `false` is also possible at count 0. + /// + /// # Safety + /// If the object is ref-counted, then the reference count must either be incremented before it hits 0, or some [`Gd`] referencing + /// this object must be forgotten. + unsafe fn refc_dec(&mut self) -> bool { + if !self.is_ref_counted() { + return false; + } + + out!(" RawGd::refc_dec: {self:?}"); + + // SAFETY: object is ref-counted, as checked above. + let is_last = unsafe { self.with_ref_counted_unchecked(|refc| refc.unreference()) }; + out!(" +-- was last={is_last}"); + is_last + } + /// Enables outer `Gd` APIs or bypasses additional null checks, in cases where `RawGd` is guaranteed non-null. /// /// # Safety @@ -443,6 +502,28 @@ impl RawGd { } } +impl RawGd +where + T: GodotClass + Bounds, +{ + /// Returns the reference count. + /// + /// Needs no runtime check, unlike the other ref-count operations: `T` is statically ref-counted, and `Gd` upholds non-nullness. + pub(crate) fn ref_count(&self) -> usize { + sys::strict_assert!( + !self.is_null(), + "RawGd::ref_count() called on null pointer; this is UB" + ); + + // SAFETY: Memory=MemRefCounted statically guarantees T inherits RefCounted. + let ref_count = + unsafe { self.with_ref_counted_unchecked(|refc| refc.get_reference_count()) }; + + // TODO find a safer cast alternative, e.g. num-traits crate with ToPrimitive (Debug) + AsPrimitive (Release). + ref_count as usize + } +} + impl RawGd where T: GodotClass + Bounds, @@ -635,9 +716,9 @@ where fn adjust_refcount_on_ptrcall_return(&mut self) { // Static type not ref-counted -> is Object, Node etc -> possibly needs incrementing refcount. - // maybe_inc_ref() is a no-op for Node etc. + // refc_inc() is a no-op for Node etc. if !::IS_REF_COUNTED { - T::DynMemory::maybe_inc_ref(self); + self.refc_inc(); } } } @@ -742,7 +823,7 @@ impl Drop for RawGd { // SAFETY: This `Gd` won't be dropped again after this. // If destruction is triggered by Godot, Storage already knows about it, no need to notify it - let is_last = unsafe { T::DynMemory::maybe_dec_ref(self) }; // may drop + let is_last = unsafe { self.refc_dec() }; // may drop if is_last { unsafe { interface_fn!(object_destroy)(self.obj_sys()); diff --git a/godot-core/src/signal/typed_signal.rs b/godot-core/src/signal/typed_signal.rs index 3056663ed..f05324d45 100644 --- a/godot-core/src/signal/typed_signal.rs +++ b/godot-core/src/signal/typed_signal.rs @@ -283,7 +283,7 @@ impl TypedSignal<'_, C, Ps> { // Godot before reaching here). Only fails for a stale Callable clone invoked manually after the object died -> no-op, like Godot. // // Edge case: emission during object destruction (e.g. PREDELETE) -- ObjectDB lookup succeeds, but re-creating a Gd for a RefCounted - // at refcount 0 panics in maybe_init_ref(). Pre-existing limitation shared with Gd::from_instance_id(). + // at refcount 0 panics in RawGd::refc_init(). Pre-existing limitation shared with Gd::from_instance_id(). let Ok(mut gd) = Gd::::try_from_instance_id(instance_id) else { return; };