Skip to content

Move ref-counting operations from DynMemory to RawGd #1667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Bromeon merged 1 commit into from
Jul 29, 2026
Merged

Move ref-counting operations from DynMemory to RawGd #1667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion godot-core/src/builder/method.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
)*
Expand Down
7 changes: 3 additions & 4 deletions godot-core/src/builtin/callable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down Expand Up @@ -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.
<classes::Object as Bounds>::DynMemory::maybe_inc_ref(&mut object.raw);
object.raw.refc_inc();
Some(object)
}

Expand Down
7 changes: 3 additions & 4 deletions godot-core/src/builtin/signal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}

<Object as Bounds>::DynMemory::maybe_inc_ref(&mut object.raw);
object.raw.refc_inc();
Some(object)
}

Expand Down
149 changes: 6 additions & 143 deletions godot-core/src/obj/bounds.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -162,40 +162,15 @@ pub trait Memory: Sealed {
/// Specifies the memory strategy of the dynamic type.
///
/// For `Gd<Object>`, 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<T: GodotClass>(obj: &mut RawGd<T>);

/// If ref-counted, then increment count
#[doc(hidden)]
fn maybe_inc_ref<T: GodotClass>(obj: &mut RawGd<T>);

/// 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<T: GodotClass>(obj: &mut RawGd<T>) -> 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<T: GodotClass>(obj: &RawGd<T>) -> Option<bool>;

/// Return the reference count, or `None` if the object is dead or manually managed.
#[doc(hidden)]
fn get_ref_count<T: GodotClass>(obj: &RawGd<T>) -> Option<usize>;

/// Returns `true` if argument and return pointers are passed as `Ref<T>` pointers given this
/// [`PtrcallType`].
///
Expand All @@ -214,65 +189,10 @@ impl Memory for MemRefCounted {
const IS_REF_COUNTED: bool = true;
}
impl DynMemory for MemRefCounted {
fn maybe_init_ref<T: GodotClass>(obj: &mut RawGd<T>) {
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<T: GodotClass>(obj: &mut RawGd<T>) {
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<T: GodotClass>(obj: &mut RawGd<T>) -> 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<T: GodotClass>(_obj: &RawGd<T>) -> Option<bool> {
Some(true)
}

fn get_ref_count<T: GodotClass>(obj: &RawGd<T>) -> Option<usize> {
// 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)
}
Expand All @@ -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<T: GodotClass>(obj: &RawGd<T>) -> 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<T: GodotClass>(obj: &mut RawGd<T>) {
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<T: GodotClass>(obj: &mut RawGd<T>) {
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<T: GodotClass>(obj: &mut RawGd<T>) -> 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<T: GodotClass>(obj: &RawGd<T>) -> Option<bool> {
// 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<T: GodotClass>(obj: &RawGd<T>) -> Option<usize> {
if Self::inherits_refcounted(obj) {
MemRefCounted::get_ref_count(obj)
} else {
None
}
}
}

/// No memory management, user responsible for not leaking.
Expand All @@ -347,17 +218,9 @@ impl Memory for MemManual {
const IS_REF_COUNTED: bool = false;
}
impl DynMemory for MemManual {
fn maybe_init_ref<T: GodotClass>(_obj: &mut RawGd<T>) {}
fn maybe_inc_ref<T: GodotClass>(_obj: &mut RawGd<T>) {}
unsafe fn maybe_dec_ref<T: GodotClass>(_obj: &mut RawGd<T>) -> bool {
false
}
fn is_ref_counted<T: GodotClass>(_obj: &RawGd<T>) -> Option<bool> {
Some(false)
}
fn get_ref_count<T: GodotClass>(_obj: &RawGd<T>) -> Option<usize> {
None
}
}

// ----------------------------------------------------------------------------------------------------------------------------------------------
Expand Down
9 changes: 3 additions & 6 deletions godot-core/src/obj/gd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,12 +1007,9 @@ where
/// }
/// ```
pub fn try_to_unique(self) -> Result<Self, (Self, usize)> {
use crate::obj::bounds::DynMemory as _;

match <T as Bounds>::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)),
}
}
}
Expand Down
Loading
Loading