From bf7f29b0bc55dc66fe1828ad43422e281b8cb1b4 Mon Sep 17 00:00:00 2001 From: Jovan Gerodetti Date: Sat, 1 Apr 2023 15:42:47 +0200 Subject: [PATCH] Implement InstanceStorage optionally as sync #18 --- .github/workflows/full-ci.yml | 16 +-- 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, 219 insertions(+), 78 deletions(-) diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index 65bb9025c..f128ab8ad 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -14,10 +14,6 @@ on: - trying env: - GDEXT_FEATURES: '' -# GDEXT_FEATURES: '--features crate/feature' -# GDEXT_CRATE_ARGS: '-p godot-codegen -p godot-ffi -p godot-core -p godot-macros -p godot' - # LSan options: https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer # * report_objects: list individual leaked objects when running LeakSanitizer LSAN_OPTIONS: report_objects=1 @@ -80,7 +76,7 @@ jobs: - name: "Check clippy" run: | - cargo clippy --all-targets $GDEXT_FEATURES ${{ matrix.rust-extra-args }} -- \ + cargo clippy --all-targets ${{ matrix.rust-extra-args }} -- \ -D clippy::suspicious -D clippy::style -D clippy::complexity -D clippy::perf \ -D clippy::dbg_macro -D clippy::todo -D clippy::unimplemented -D warnings @@ -92,6 +88,8 @@ jobs: strategy: fail-fast: false # cancel all jobs as soon as one fails? matrix: + name: ["macos", "windows", "linux"] + feature: ["default", "threads"] # Order this way because macOS typically has the longest duration, followed by Windows, so it benefits total workflow execution time. # Additionally, the 'linux (msrv *)' special case will then be listed next to the other 'linux' jobs. # Note: Windows uses '--target x86_64-pc-windows-msvc' by default as Cargo argument. @@ -150,10 +148,10 @@ jobs: godot-binary: ${{ matrix.godot-binary }} - name: "Compile tests" - run: cargo test $GDEXT_FEATURES --no-run ${{ matrix.rust-extra-args }} + run: cargo test --features "${{ matrix.feature }}" --no-run ${{ matrix.rust-extra-args }} - name: "Test" - run: cargo test $GDEXT_FEATURES ${{ matrix.rust-extra-args }} + run: cargo test --features "${{ matrix.feature }}" ${{ matrix.rust-extra-args }} godot-itest: @@ -164,6 +162,8 @@ jobs: strategy: fail-fast: false # cancel all jobs as soon as one fails? matrix: + name: ["macos", "windows", "linux"] + feature: ["default", "threads"] # Order this way because macOS typically has the longest duration, followed by Windows, so it benefits total workflow execution time. # Additionally, the 'linux (msrv *)' special case will then be listed next to the other 'linux' jobs. # Note: Windows uses '--target x86_64-pc-windows-msvc' by default as Cargo argument. @@ -234,7 +234,7 @@ jobs: artifact-name: godot-${{ matrix.name }} godot-binary: ${{ matrix.godot-binary }} godot-args: ${{ matrix.godot-args }} - rust-extra-args: ${{ matrix.rust-extra-args }} + rust-extra-args: --features "${{ matrix.feature }}" ${{ matrix.rust-extra-args }} rust-toolchain: ${{ matrix.rust-toolchain }} rust-env-rustflags: ${{ matrix.rust-env-rustflags }} with-llvm: ${{ matrix.with-llvm }} diff --git a/godot-core/Cargo.toml b/godot-core/Cargo.toml index 2898c12ca..23381e447 100644 --- a/godot-core/Cargo.toml +++ b/godot-core/Cargo.toml @@ -13,6 +13,7 @@ trace = [] codegen-fmt = ["godot-ffi/codegen-fmt"] codegen-full = ["godot-codegen/codegen-full"] double-precision = ["godot-codegen/double-precision"] +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 b05d29ee4..18e06fcf4 100644 --- a/godot/Cargo.toml +++ b/godot/Cargo.toml @@ -12,6 +12,7 @@ default = ["codegen-full"] formatted = ["godot-core/codegen-fmt"] trace = ["godot-core/trace"] double-precision = ["godot-core/double-precision"] +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 8930d8ddb..852f6ed8d 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"] }