From 91f5e82240c8f6c9721ba22139eb1d8916d56fcc Mon Sep 17 00:00:00 2001 From: Jovan Gerodetti Date: Sat, 1 Apr 2023 15:42:47 +0200 Subject: [PATCH 1/2] Implement InstanceStorage optionally as sync #18 --- .github/workflows/full-ci.yml | 5 + godot-core/Cargo.toml | 1 + godot-core/src/obj/guards.rs | 23 +++ godot-core/src/storage.rs | 255 ++++++++++++++++++++++++---------- godot/Cargo.toml | 1 + itest/rust/Cargo.toml | 1 + 6 files changed, 216 insertions(+), 70 deletions(-) diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index 372fc80fe..76e32dd21 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -169,6 +169,11 @@ jobs: godot-binary: godot.linuxbsd.editor.dev.double.x86_64 rust-extra-args: --features double-precision + - name: linux-threads + os: ubuntu-20.04 + godot-binary: godot.linuxbsd.editor.dev.double.x86_64 + rust-extra-args: --features threads + - name: linux-nightly os: ubuntu-20.04 artifact-name: linux diff --git a/godot-core/Cargo.toml b/godot-core/Cargo.toml index 93a7c4978..692fb9869 100644 --- a/godot-core/Cargo.toml +++ b/godot-core/Cargo.toml @@ -14,6 +14,7 @@ codegen-fmt = ["godot-ffi/codegen-fmt", "godot-codegen/codegen-fmt"] codegen-full = ["godot-codegen/codegen-full"] double-precision = ["godot-codegen/double-precision"] custom-godot = ["godot-ffi/custom-godot", "godot-codegen/custom-godot"] +threads = [] [dependencies] godot-ffi = { path = "../godot-ffi" } diff --git a/godot-core/src/obj/guards.rs b/godot-core/src/obj/guards.rs index 307920e57..d831278a7 100644 --- a/godot-core/src/obj/guards.rs +++ b/godot-core/src/obj/guards.rs @@ -4,22 +4,35 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ +#[cfg(not(feature = "threads"))] use std::cell; use std::fmt::Debug; use std::ops::{Deref, DerefMut}; +#[cfg(feature = "threads")] +use std::sync; /// Immutably/shared bound reference guard for a [`Gd`][crate::obj::Gd] smart pointer. /// /// See [`Gd::bind`][crate::obj::Gd::bind] for usage. #[derive(Debug)] pub struct GdRef<'a, T> { + #[cfg(not(feature = "threads"))] cell_ref: cell::Ref<'a, T>, + + #[cfg(feature = "threads")] + cell_ref: sync::RwLockReadGuard<'a, T>, } impl<'a, T> GdRef<'a, T> { + #[cfg(not(feature = "threads"))] pub(crate) fn from_cell(cell_ref: cell::Ref<'a, T>) -> Self { Self { cell_ref } } + + #[cfg(feature = "threads")] + pub(crate) fn from_cell(cell_ref: sync::RwLockReadGuard<'a, T>) -> Self { + Self { cell_ref } + } } impl Deref for GdRef<'_, T> { @@ -39,13 +52,23 @@ impl Deref for GdRef<'_, T> { /// See [`Gd::bind_mut`][crate::obj::Gd::bind_mut] for usage. #[derive(Debug)] pub struct GdMut<'a, T> { + #[cfg(not(feature = "threads"))] cell_ref: cell::RefMut<'a, T>, + + #[cfg(feature = "threads")] + cell_ref: sync::RwLockWriteGuard<'a, T>, } impl<'a, T> GdMut<'a, T> { + #[cfg(not(feature = "threads"))] pub(crate) fn from_cell(cell_ref: cell::RefMut<'a, T>) -> Self { Self { cell_ref } } + + #[cfg(feature = "threads")] + pub(crate) fn from_cell(cell_ref: sync::RwLockWriteGuard<'a, T>) -> Self { + Self { cell_ref } + } } impl Deref for GdMut<'_, T> { diff --git a/godot-core/src/storage.rs b/godot-core/src/storage.rs index b501d1de3..2eac95187 100644 --- a/godot-core/src/storage.rs +++ b/godot-core/src/storage.rs @@ -9,16 +9,6 @@ use crate::out; use godot_ffi as sys; use std::any::type_name; -use std::cell; - -/// Manages storage and lifecycle of user's extension class instances. -pub struct InstanceStorage { - user_instance: cell::RefCell, - - // Declared after `user_instance`, is dropped last - pub lifecycle: Lifecycle, - godot_ref_count: i32, -} #[derive(Copy, Clone, Debug)] pub enum Lifecycle { @@ -27,76 +17,201 @@ pub enum Lifecycle { Dead, // reading this would typically already be too late, only best-effort in case of UB } -/// For all Godot extension classes -impl InstanceStorage { - pub fn construct(user_instance: T) -> Self { - out!(" Storage::construct <{}>", type_name::()); +#[cfg(not(feature = "threads"))] +pub use single_thread::*; - Self { - user_instance: cell::RefCell::new(user_instance), - lifecycle: Lifecycle::Alive, - godot_ref_count: 1, - } - } +#[cfg(feature = "threads")] +pub use multi_thread::*; - pub(crate) fn on_inc_ref(&mut self) { - self.godot_ref_count += 1; - out!( - " Storage::on_inc_ref (rc={}) <{}>", // -- {:?}", - self.godot_ref_count, - type_name::(), - //self.user_instance - ); +#[cfg(not(feature = "threads"))] +mod single_thread { + use std::any::type_name; + use std::cell; + + use crate::obj::GodotClass; + use crate::out; + + use super::Lifecycle; + + /// Manages storage and lifecycle of user's extension class instances. + pub struct InstanceStorage { + user_instance: cell::RefCell, + + // Declared after `user_instance`, is dropped last + pub lifecycle: Lifecycle, + godot_ref_count: u32, } - pub(crate) fn on_dec_ref(&mut self) { - self.godot_ref_count -= 1; - out!( - " | Storage::on_dec_ref (rc={}) <{}>", // -- {:?}", - self.godot_ref_count, - type_name::(), - //self.user_instance - ); + /// For all Godot extension classes + impl InstanceStorage { + pub fn construct(user_instance: T) -> Self { + out!(" Storage::construct <{}>", type_name::()); + + Self { + user_instance: cell::RefCell::new(user_instance), + lifecycle: Lifecycle::Alive, + godot_ref_count: 1, + } + } + + pub(crate) fn on_inc_ref(&mut self) { + self.godot_ref_count += 1; + out!( + " Storage::on_inc_ref (rc={}) <{}>", // -- {:?}", + self.godot_ref_count(), + type_name::(), + //self.user_instance + ); + } + + pub(crate) fn on_dec_ref(&mut self) { + self.godot_ref_count -= 1; + out!( + " | Storage::on_dec_ref (rc={}) <{}>", // -- {:?}", + self.godot_ref_count(), + type_name::(), + //self.user_instance + ); + } + + /* pub fn destroy(&mut self) { + assert!( + self.user_instance.is_some(), + "Cannot destroy user instance which is not yet initialized" + ); + assert!( + !self.destroyed, + "Cannot destroy user instance multiple times" + ); + self.user_instance = None; // drops T + // TODO drop entire Storage + }*/ + + pub fn get(&self) -> cell::Ref { + self.user_instance.try_borrow().unwrap_or_else(|_e| { + panic!( + "Gd::bind() failed, already bound; T = {}.\n \ + Make sure there is no &mut T live at the time.\n \ + This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.", + type_name::() + ) + }) + } + + pub fn get_mut(&mut self) -> cell::RefMut { + self.user_instance.try_borrow_mut().unwrap_or_else(|_e| { + panic!( + "Gd::bind_mut() failed, already bound; T = {}.\n \ + Make sure there is no &T or &mut T live at the time.\n \ + This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.", + type_name::() + ) + }) + } + + pub(super) fn godot_ref_count(&self) -> u32 { + self.godot_ref_count + } } +} - /* pub fn destroy(&mut self) { - assert!( - self.user_instance.is_some(), - "Cannot destroy user instance which is not yet initialized" - ); - assert!( - !self.destroyed, - "Cannot destroy user instance multiple times" - ); - self.user_instance = None; // drops T - // TODO drop entire Storage - }*/ +#[cfg(feature = "threads")] +mod multi_thread { + use std::any::type_name; + use std::sync; + use std::sync::atomic::{AtomicU32, Ordering}; - #[must_use] - pub fn into_raw(self) -> *mut Self { - Box::into_raw(Box::new(self)) + use crate::obj::GodotClass; + use crate::out; + + use super::Lifecycle; + + /// Manages storage and lifecycle of user's extension class instances. + pub struct InstanceStorage { + user_instance: sync::RwLock, + + // Declared after `user_instance`, is dropped last + pub lifecycle: Lifecycle, + godot_ref_count: AtomicU32, } - pub fn get(&self) -> cell::Ref { - self.user_instance.try_borrow().unwrap_or_else(|_e| { - panic!( - "Gd::bind() failed, already bound; T = {}.\n \ - Make sure there is no &mut T live at the time.\n \ - This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.", - type_name::() - ) - }) + /// For all Godot extension classes + impl InstanceStorage { + pub fn construct(user_instance: T) -> Self { + out!(" Storage::construct <{}>", type_name::()); + + Self { + user_instance: sync::RwLock::new(user_instance), + lifecycle: Lifecycle::Alive, + godot_ref_count: AtomicU32::new(1), + } + } + + pub(crate) fn on_inc_ref(&mut self) { + self.godot_ref_count.fetch_add(1, Ordering::Relaxed); + out!( + " Storage::on_inc_ref (rc={}) <{}>", // -- {:?}", + self.godot_ref_count(), + type_name::(), + //self.user_instance + ); + } + + pub(crate) fn on_dec_ref(&mut self) { + self.godot_ref_count.fetch_sub(1, Ordering::Relaxed); + out!( + " | Storage::on_dec_ref (rc={}) <{}>", // -- {:?}", + self.godot_ref_count(), + type_name::(), + //self.user_instance + ); + } + + /* pub fn destroy(&mut self) { + assert!( + self.user_instance.is_some(), + "Cannot destroy user instance which is not yet initialized" + ); + assert!( + !self.destroyed, + "Cannot destroy user instance multiple times" + ); + self.user_instance = None; // drops T + // TODO drop entire Storage + }*/ + + pub fn get(&self) -> sync::RwLockReadGuard { + self.user_instance.read().unwrap_or_else(|_e| { + panic!( + "Gd::bind() failed, already bound; T = {}.\n \ + Make sure there is no &mut T live at the time.\n \ + This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.", + type_name::() + ) + }) + } + + pub fn get_mut(&mut self) -> sync::RwLockWriteGuard { + self.user_instance.write().unwrap_or_else(|_e| { + panic!( + "Gd::bind_mut() failed, already bound; T = {}.\n \ + Make sure there is no &T or &mut T live at the time.\n \ + This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.", + type_name::() + ) + }) + } + + pub(super) fn godot_ref_count(&self) -> u32 { + self.godot_ref_count.load(Ordering::Relaxed) + } } +} - pub fn get_mut(&mut self) -> cell::RefMut { - self.user_instance.try_borrow_mut().unwrap_or_else(|_e| { - panic!( - "Gd::bind_mut() failed, already bound; T = {}.\n \ - Make sure there is no &T or &mut T live at the time.\n \ - This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.", - type_name::() - ) - }) +impl InstanceStorage { + #[must_use] + pub fn into_raw(self) -> *mut Self { + Box::into_raw(Box::new(self)) } pub fn mark_destroyed_by_godot(&mut self) { @@ -127,7 +242,7 @@ impl Drop for InstanceStorage { fn drop(&mut self) { out!( " Storage::drop (rc={}) <{}>", // -- {:?}", - self.godot_ref_count, + self.godot_ref_count(), type_name::(), //self.user_instance ); diff --git a/godot/Cargo.toml b/godot/Cargo.toml index c6828b723..1813708bc 100644 --- a/godot/Cargo.toml +++ b/godot/Cargo.toml @@ -12,6 +12,7 @@ default = ["codegen-full"] formatted = ["godot-core/codegen-fmt"] double-precision = ["godot-core/double-precision"] custom-godot = ["godot-core/custom-godot"] +threads = ["godot-core/threads"] # Private features, they are under no stability guarantee codegen-full = ["godot-core/codegen-full"] diff --git a/itest/rust/Cargo.toml b/itest/rust/Cargo.toml index 53f5f203f..dd84f5c1e 100644 --- a/itest/rust/Cargo.toml +++ b/itest/rust/Cargo.toml @@ -12,6 +12,7 @@ crate-type = ["cdylib"] default = [] trace = ["godot/trace"] double-precision = ["godot/double-precision"] +threads = ["godot/threads"] [dependencies] godot = { path = "../../godot", default-features = false, features = ["formatted"] } From 338779d27e2be8cfe8a138a68b373b2b6ac37245 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 1 May 2023 12:24:59 +0200 Subject: [PATCH 2/2] CI: update artifact name for linux-threads --- .github/workflows/full-ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index 76e32dd21..038be1907 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -171,7 +171,8 @@ jobs: - name: linux-threads os: ubuntu-20.04 - godot-binary: godot.linuxbsd.editor.dev.double.x86_64 + artifact-name: linux + godot-binary: godot.linuxbsd.editor.dev.x86_64 rust-extra-args: --features threads - name: linux-nightly