From bbb7e856775938aabd2230074de70aa3193ea674 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 10 Nov 2022 02:45:05 -0800 Subject: [PATCH 01/48] Abort on dropping NonSend in non-origin thread. --- crates/bevy_ecs/src/storage/resource.rs | 23 +++++++++++++++++++++++ crates/bevy_ecs/src/world/mod.rs | 20 ++++++++++++-------- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index edfa51e12857a..775c96b24822c 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -2,7 +2,9 @@ use crate::archetype::ArchetypeComponentId; use crate::component::{ComponentId, ComponentTicks, Components}; use crate::storage::{Column, SparseSet}; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; +use bevy_utils::tracing::error; use std::cell::UnsafeCell; +use std::thread::ThreadId; /// The type-erased backing storage and metadata for a single resource within a [`World`]. /// @@ -10,6 +12,22 @@ use std::cell::UnsafeCell; pub struct ResourceData { column: Column, id: ArchetypeComponentId, + origin_thread_id: Option, +} + +impl Drop for ResourceData { + fn drop(&mut self) { + if let Some(origin_thread_id) = self.origin_thread_id { + if origin_thread_id != std::thread::current().id() { + error!( + "Attempted to drop non-send resource from thread {:?} on a thread {:?}. This is not allowed. Aborting.", + origin_thread_id, + std::thread::current().id() + ); + std::process::abort(); + } + } + } } impl ResourceData { @@ -162,10 +180,14 @@ impl Resources { /// /// # Panics /// Will panic if `component_id` is not valid for the provided `components` + /// + /// # Safety + /// `is_send` must be accurate for the Resource that is being pub(crate) fn initialize_with( &mut self, component_id: ComponentId, components: &Components, + is_send: bool, f: impl FnOnce() -> ArchetypeComponentId, ) -> &mut ResourceData { self.resources.get_or_insert_with(component_id, || { @@ -173,6 +195,7 @@ impl Resources { ResourceData { column: Column::with_capacity(component_info, 1), id: f(), + origin_thread_id: is_send.then(|| std::thread::current().id()), } }) } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 1aab56a9f4ae7..6028cd9cc4dc1 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -739,7 +739,7 @@ impl World { OwningPtr::make(value, |ptr| { // SAFETY: component_id was just initialized and corresponds to resource of type R unsafe { - self.insert_resource_by_id(component_id, ptr); + self.insert_resource_by_id(component_id, true, ptr); } }); } @@ -779,7 +779,7 @@ impl World { OwningPtr::make(value, |ptr| { // SAFETY: component_id was just initialized and corresponds to resource of type R unsafe { - self.insert_resource_by_id(component_id, ptr); + self.insert_resource_by_id(component_id, false, ptr); } }); } @@ -1311,31 +1311,35 @@ impl World { /// /// # Safety /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world - /// `component_id` must exist in this [`World`] + /// `component_id` must exist in this [`World`]. `is_send` must correspond to whether the resource + /// adheres to invariants of `Send`. #[inline] pub unsafe fn insert_resource_by_id( &mut self, component_id: ComponentId, + is_send: bool, value: OwningPtr<'_>, ) { let change_tick = self.change_tick(); // SAFETY: component_id is valid, ensured by caller - self.initialize_resource_internal(component_id) + self.initialize_resource_internal(component_id, is_send) .insert(value, change_tick); } /// # Safety /// `component_id` must be valid for this world + /// `is_send` should correspond for the resource being initialized. #[inline] unsafe fn initialize_resource_internal( &mut self, component_id: ComponentId, + is_send: bool, ) -> &mut ResourceData { let archetype_component_count = &mut self.archetypes.archetype_component_count; self.storages .resources - .initialize_with(component_id, &self.components, || { + .initialize_with(component_id, &self.components, is_send, || { let id = ArchetypeComponentId::new(*archetype_component_count); *archetype_component_count += 1; id @@ -1345,14 +1349,14 @@ impl World { pub(crate) fn initialize_resource(&mut self) -> ComponentId { let component_id = self.components.init_resource::(); // SAFETY: resource initialized above - unsafe { self.initialize_resource_internal(component_id) }; + unsafe { self.initialize_resource_internal(component_id, true) }; component_id } pub(crate) fn initialize_non_send_resource(&mut self) -> ComponentId { let component_id = self.components.init_non_send::(); // SAFETY: resource initialized above - unsafe { self.initialize_resource_internal(component_id) }; + unsafe { self.initialize_resource_internal(component_id, false) }; component_id } @@ -1794,7 +1798,7 @@ mod tests { OwningPtr::make(value, |ptr| { // SAFETY: value is valid for the component layout unsafe { - world.insert_resource_by_id(component_id, ptr); + world.insert_resource_by_id(component_id, true, ptr); } }); From 0f4b73dc5a8faf9f6c928ac1b10dd31c7568fa54 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 10 Nov 2022 02:56:13 -0800 Subject: [PATCH 02/48] Fix safety comments --- crates/bevy_ecs/src/storage/resource.rs | 2 +- crates/bevy_ecs/src/world/mod.rs | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 775c96b24822c..1732d188ad6de 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -183,7 +183,7 @@ impl Resources { /// /// # Safety /// `is_send` must be accurate for the Resource that is being - pub(crate) fn initialize_with( + pub(crate) unsafe fn initialize_with( &mut self, component_id: ComponentId, components: &Components, diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 6028cd9cc4dc1..f1aae6480cef9 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1322,7 +1322,7 @@ impl World { ) { let change_tick = self.change_tick(); - // SAFETY: component_id is valid, ensured by caller + // SAFETY: component_id and is_send are valid, ensured by caller self.initialize_resource_internal(component_id, is_send) .insert(value, change_tick); } @@ -1337,6 +1337,7 @@ impl World { is_send: bool, ) -> &mut ResourceData { let archetype_component_count = &mut self.archetypes.archetype_component_count; + // SAFETY: Caller guarentees that `is_send` matches the underlying type. self.storages .resources .initialize_with(component_id, &self.components, is_send, || { @@ -1348,14 +1349,14 @@ impl World { pub(crate) fn initialize_resource(&mut self) -> ComponentId { let component_id = self.components.init_resource::(); - // SAFETY: resource initialized above + // SAFETY: resource initialized above, resource type must be Send unsafe { self.initialize_resource_internal(component_id, true) }; component_id } pub(crate) fn initialize_non_send_resource(&mut self) -> ComponentId { let component_id = self.components.init_non_send::(); - // SAFETY: resource initialized above + // SAFETY: resource initialized above, resource type might not be send unsafe { self.initialize_resource_internal(component_id, false) }; component_id } From e83e8e385bb740506be3e22884a66b51fd1736a7 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 10 Nov 2022 02:56:29 -0800 Subject: [PATCH 03/48] Invert is_send --- crates/bevy_ecs/src/storage/resource.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 1732d188ad6de..7671a48e14d4e 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -195,7 +195,7 @@ impl Resources { ResourceData { column: Column::with_capacity(component_info, 1), id: f(), - origin_thread_id: is_send.then(|| std::thread::current().id()), + origin_thread_id: (!is_send).then(|| std::thread::current().id()), } }) } From e46ec0c5c8e6895e3e1258ad7d6b4808986cf283 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 10 Nov 2022 03:04:40 -0800 Subject: [PATCH 04/48] Factor out drop-abort --- crates/bevy_ecs/src/storage/resource.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 7671a48e14d4e..b25a965ac57cb 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -17,6 +17,13 @@ pub struct ResourceData { impl Drop for ResourceData { fn drop(&mut self) { + self.validate_drop(); + } +} + +impl ResourceData { + #[inline] + fn validate_drop(&self) { if let Some(origin_thread_id) = self.origin_thread_id { if origin_thread_id != std::thread::current().id() { error!( @@ -28,9 +35,7 @@ impl Drop for ResourceData { } } } -} -impl ResourceData { /// Returns true if the resource is populated. #[inline] pub fn is_present(&self) -> bool { @@ -78,6 +83,9 @@ impl ResourceData { /// [`World::validate_non_send_access_untyped`]: crate::world::World::validate_non_send_access_untyped #[inline] pub(crate) unsafe fn insert(&mut self, value: OwningPtr<'_>, change_tick: u32) { + if self.origin_thread_id.is_some() { + self.origin_thread_id = Some(std::thread::current().id()); + } if self.is_present() { self.column.replace(0, value, change_tick); } else { @@ -133,6 +141,7 @@ impl ResourceData { /// [`World::validate_non_send_access_untyped`]: crate::world::World::validate_non_send_access_untyped #[inline] pub(crate) unsafe fn remove_and_drop(&mut self) { + self.validate_drop(); self.column.clear(); } } @@ -182,7 +191,7 @@ impl Resources { /// Will panic if `component_id` is not valid for the provided `components` /// /// # Safety - /// `is_send` must be accurate for the Resource that is being + /// `is_send` must be accurate for the Resource that is being initialized. pub(crate) unsafe fn initialize_with( &mut self, component_id: ComponentId, From 0898a540864ecdaae28176ba85ccf0be8d28e211 Mon Sep 17 00:00:00 2001 From: james7132 Date: Tue, 15 Nov 2022 20:05:22 -0800 Subject: [PATCH 05/48] Fix access, replace MainThreadValidator --- crates/bevy_ecs/src/lib.rs | 21 +++++++- crates/bevy_ecs/src/storage/resource.rs | 58 +++++++++++++++----- crates/bevy_ecs/src/system/system_param.rs | 4 -- crates/bevy_ecs/src/world/mod.rs | 62 ++-------------------- 4 files changed, 69 insertions(+), 76 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 04bc2ed7ffec4..5e43959d4c33c 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1311,7 +1311,7 @@ mod tests { #[test] #[should_panic( - expected = "attempted to access NonSend resource bevy_ecs::tests::NonSendA off of the main thread" + expected = "Attempted to access or drop non-send resource bevy_ecs::tests::NonSendA from thread" )] fn non_send_resource_scope_from_different_thread() { let mut world = World::default(); @@ -1330,6 +1330,25 @@ mod tests { } } + #[test] + #[should_panic( + expected = "Attempted to access or drop non-send resource bevy_ecs::tests::NonSendA from thread" + )] + fn non_send_resource_drop_from_different_thread() { + let mut world = World::default(); + world.insert_non_send_resource(NonSendA::default()); + + let thread = std::thread::spawn(move || { + // Dropping the non-send resource on a different thread + // Should result in a panic + drop(world); + }); + + if let Err(err) = thread.join() { + std::panic::resume_unwind(err); + } + } + #[test] fn insert_overwrite_drop() { let (dropck1, dropped1) = DropCk::new_pair(); diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index b25a965ac57cb..bc74a06a8c8ad 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -3,6 +3,7 @@ use crate::component::{ComponentId, ComponentTicks, Components}; use crate::storage::{Column, SparseSet}; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; use bevy_utils::tracing::error; +use std::borrow::Cow; use std::cell::UnsafeCell; use std::thread::ThreadId; @@ -11,27 +12,46 @@ use std::thread::ThreadId; /// [`World`]: crate::world::World pub struct ResourceData { column: Column, + type_name: String, id: ArchetypeComponentId, origin_thread_id: Option, } impl Drop for ResourceData { fn drop(&mut self) { - self.validate_drop(); + if self.is_present() { + self.validate_access(); + } } } impl ResourceData { #[inline] - fn validate_drop(&self) { + fn validate_access(&self) { + #[cfg(test)] + if std::thread::panicking() { + return; + } if let Some(origin_thread_id) = self.origin_thread_id { if origin_thread_id != std::thread::current().id() { - error!( - "Attempted to drop non-send resource from thread {:?} on a thread {:?}. This is not allowed. Aborting.", + // Panic in tests, as testing for aborting is nearly impossible + #[cfg(test)] + panic!( + "Attempted to access or drop non-send resource {} from thread {:?} on a thread {:?}. This is not allowed. Aborting.", + self.type_name, origin_thread_id, std::thread::current().id() ); - std::process::abort(); + #[cfg(not(test))] + { + error!( + "Attempted to access or drop non-send resource {} from thread {:?} on a thread {:?}. This is not allowed. Aborting.", + self.type_name, + origin_thread_id, + std::thread::current().id() + ); + std::process::abort(); + } } } } @@ -51,7 +71,10 @@ impl ResourceData { /// Gets a read-only pointer to the underlying resource, if available. #[inline] pub fn get_data(&self) -> Option> { - self.column.get_data(0) + self.column.get_data(0).map(|res| { + self.validate_access(); + res + }) } /// Gets a read-only reference to the change ticks of the underlying resource, if available. @@ -68,7 +91,10 @@ impl ResourceData { #[inline] pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, &UnsafeCell)> { - self.column.get(0) + self.column.get(0).map(|res| { + self.validate_access(); + res + }) } /// Inserts a value into the resource. If a value is already present @@ -83,12 +109,11 @@ impl ResourceData { /// [`World::validate_non_send_access_untyped`]: crate::world::World::validate_non_send_access_untyped #[inline] pub(crate) unsafe fn insert(&mut self, value: OwningPtr<'_>, change_tick: u32) { - if self.origin_thread_id.is_some() { - self.origin_thread_id = Some(std::thread::current().id()); - } if self.is_present() { + self.validate_access(); self.column.replace(0, value, change_tick); } else { + self.origin_thread_id = self.origin_thread_id.map(|_| std::thread::current().id()); self.column.push(value, ComponentTicks::new(change_tick)); } } @@ -110,9 +135,11 @@ impl ResourceData { change_ticks: ComponentTicks, ) { if self.is_present() { + self.validate_access(); self.column.replace_untracked(0, value); *self.column.get_ticks_unchecked(0).deref_mut() = change_ticks; } else { + self.origin_thread_id = self.origin_thread_id.map(|_| std::thread::current().id()); self.column.push(value, change_ticks); } } @@ -129,7 +156,9 @@ impl ResourceData { #[inline] #[must_use = "The returned pointer to the removed component should be used or dropped"] pub(crate) unsafe fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { - self.column.swap_remove_and_forget(0) + self.is_present() + .then(|| self.validate_access()) + .and_then(|_| self.column.swap_remove_and_forget(0)) } /// Removes a value from the resource, if present, and drops it. @@ -141,8 +170,10 @@ impl ResourceData { /// [`World::validate_non_send_access_untyped`]: crate::world::World::validate_non_send_access_untyped #[inline] pub(crate) unsafe fn remove_and_drop(&mut self) { - self.validate_drop(); - self.column.clear(); + if self.is_present() { + self.validate_access(); + self.column.clear(); + } } } @@ -203,6 +234,7 @@ impl Resources { let component_info = components.get_info(component_id).unwrap(); ResourceData { column: Column::with_capacity(component_info, 1), + type_name: String::from(component_info.name()), id: f(), origin_thread_id: (!is_send).then(|| std::thread::current().id()), } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index fa5f5bf517785..dc6dde88842c6 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -995,7 +995,6 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for NonSendState { world: &'w World, change_tick: u32, ) -> Self::Item { - world.validate_non_send_access::(); let (ptr, ticks) = world .get_resource_with_ticks(state.component_id) .unwrap_or_else(|| { @@ -1045,7 +1044,6 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for OptionNonSendState { world: &'w World, change_tick: u32, ) -> Self::Item { - world.validate_non_send_access::(); world .get_resource_with_ticks(state.0.component_id) .map(|(ptr, ticks)| NonSend { @@ -1110,7 +1108,6 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for NonSendMutState { world: &'w World, change_tick: u32, ) -> Self::Item { - world.validate_non_send_access::(); let (ptr, ticks) = world .get_resource_with_ticks(state.component_id) .unwrap_or_else(|| { @@ -1158,7 +1155,6 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for OptionNonSendMutState { world: &'w World, change_tick: u32, ) -> Self::Item { - world.validate_non_send_access::(); world .get_resource_with_ticks(state.0.component_id) .map(|(ptr, ticks)| NonSendMut { diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index f1aae6480cef9..321c8a9d10446 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -60,7 +60,6 @@ pub struct World { pub(crate) removed_components: SparseSet>, /// Access cache used by [WorldCell]. pub(crate) archetype_component_access: ArchetypeComponentAccess, - main_thread_validator: MainThreadValidator, pub(crate) change_tick: AtomicU32, pub(crate) last_change_tick: u32, } @@ -76,7 +75,6 @@ impl Default for World { bundles: Default::default(), removed_components: Default::default(), archetype_component_access: Default::default(), - main_thread_validator: Default::default(), // Default value is `1`, and `last_change_tick`s default to `0`, such that changes // are detected on first system runs and for direct world queries. change_tick: AtomicU32::new(1), @@ -774,7 +772,6 @@ impl World { /// Panics if called from a thread other than the main thread. #[inline] pub fn insert_non_send_resource(&mut self, value: R) { - self.validate_non_send_access::(); let component_id = self.components.init_non_send::(); OwningPtr::make(value, |ptr| { // SAFETY: component_id was just initialized and corresponds to resource of type R @@ -793,7 +790,6 @@ impl World { #[inline] pub fn remove_non_send_resource(&mut self) -> Option { - self.validate_non_send_access::(); // SAFETY: we are on main thread unsafe { self.remove_resource_unchecked() } } @@ -1176,11 +1172,7 @@ impl World { .get_resource_id(TypeId::of::()) .unwrap_or_else(|| panic!("resource does not exist: {}", std::any::type_name::())); // If the resource isn't send and sync, validate that we are on the main thread, so that we can access it. - let component_info = self.components().get_info(component_id).unwrap(); - if !component_info.is_send_and_sync() { - self.validate_non_send_access::(); - } - + let _component_info = self.components().get_info(component_id).unwrap(); let (ptr, mut ticks) = self .storages .resources @@ -1288,7 +1280,6 @@ impl World { &self, component_id: ComponentId, ) -> Option<&R> { - self.validate_non_send_access::(); self.get_resource_with_id(component_id) } @@ -1300,7 +1291,6 @@ impl World { &self, component_id: ComponentId, ) -> Option> { - self.validate_non_send_access::(); self.get_resource_unchecked_mut_with_id(component_id) } @@ -1361,22 +1351,6 @@ impl World { component_id } - pub(crate) fn validate_non_send_access(&self) { - assert!( - self.main_thread_validator.is_main_thread(), - "attempted to access NonSend resource {} off of the main thread", - std::any::type_name::(), - ); - } - - pub(crate) fn validate_non_send_access_untyped(&self, name: &str) { - assert!( - self.main_thread_validator.is_main_thread(), - "attempted to access NonSend resource {} off of the main thread", - name - ); - } - /// Empties queued entities and adds them to the empty [Archetype](crate::archetype::Archetype). /// This should be called before doing operations that might operate on queued entities, /// such as inserting a [Component]. @@ -1440,10 +1414,7 @@ impl World { /// use this in cases where the actual types are not known at compile time.** #[inline] pub fn get_resource_by_id(&self, component_id: ComponentId) -> Option> { - let info = self.components.get_info(component_id)?; - if !info.is_send_and_sync() { - self.validate_non_send_access_untyped(info.name()); - } + let _info = self.components.get_info(component_id)?; self.storages.resources.get(component_id)?.get_data() } @@ -1455,11 +1426,7 @@ impl World { /// use this in cases where the actual types are not known at compile time.** #[inline] pub fn get_resource_mut_by_id(&mut self, component_id: ComponentId) -> Option> { - let info = self.components.get_info(component_id)?; - if !info.is_send_and_sync() { - self.validate_non_send_access_untyped(info.name()); - } - + let _info = self.components.get_info(component_id)?; let (ptr, ticks) = self.get_resource_with_ticks(component_id)?; // SAFE: This function has exclusive access to the world so nothing aliases `ticks`. @@ -1484,10 +1451,7 @@ impl World { /// **You should prefer to use the typed API [`World::remove_resource`] where possible and only /// use this in cases where the actual types are not known at compile time.** pub fn remove_resource_by_id(&mut self, component_id: ComponentId) -> Option<()> { - let info = self.components.get_info(component_id)?; - if !info.is_send_and_sync() { - self.validate_non_send_access_untyped(info.name()); - } + let _info = self.components.get_info(component_id)?; // SAFETY: The underlying type is Send and Sync or we've already validated we're on the main thread unsafe { self.storages @@ -1575,24 +1539,6 @@ impl FromWorld for T { } } -struct MainThreadValidator { - main_thread: std::thread::ThreadId, -} - -impl MainThreadValidator { - fn is_main_thread(&self) -> bool { - self.main_thread == std::thread::current().id() - } -} - -impl Default for MainThreadValidator { - fn default() -> Self { - Self { - main_thread: std::thread::current().id(), - } - } -} - #[cfg(test)] mod tests { use super::World; From e4d61c1ca509e47533896fc13c8d1dd2fbd339ea Mon Sep 17 00:00:00 2001 From: james7132 Date: Tue, 15 Nov 2022 20:15:06 -0800 Subject: [PATCH 06/48] Update docs --- crates/bevy_ecs/src/storage/resource.rs | 45 +++++++++++++++---------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index bc74a06a8c8ad..be8cc26ac8dc7 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -2,7 +2,9 @@ use crate::archetype::ArchetypeComponentId; use crate::component::{ComponentId, ComponentTicks, Components}; use crate::storage::{Column, SparseSet}; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; +#[allow(unused_imports)] use bevy_utils::tracing::error; +#[allow(unused_imports)] use std::borrow::Cow; use std::cell::UnsafeCell; use std::thread::ThreadId; @@ -28,6 +30,7 @@ impl Drop for ResourceData { impl ResourceData { #[inline] fn validate_access(&self) { + // Avoid aborting due to double panicking on the same thread. #[cfg(test)] if std::thread::panicking() { return; @@ -100,13 +103,13 @@ impl ResourceData { /// Inserts a value into the resource. If a value is already present /// it will be replaced. /// + /// # Aborts + /// This will abort the process if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. + /// This function will panic instead in tests. + /// /// # Safety /// `value` must be valid for the underlying type for the resource. - /// - /// The underlying type must be [`Send`] or be inserted from the main thread. - /// This can be validated with [`World::validate_non_send_access_untyped`]. - /// - /// [`World::validate_non_send_access_untyped`]: crate::world::World::validate_non_send_access_untyped #[inline] pub(crate) unsafe fn insert(&mut self, value: OwningPtr<'_>, change_tick: u32) { if self.is_present() { @@ -121,13 +124,13 @@ impl ResourceData { /// Inserts a value into the resource with a pre-existing change tick. If a /// value is already present it will be replaced. /// + /// # Aborts + /// This will abort the process if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. + /// This function will panic instead in tests. + /// /// # Safety /// `value` must be valid for the underlying type for the resource. - /// - /// The underlying type must be [`Send`] or be inserted from the main thread. - /// This can be validated with [`World::validate_non_send_access_untyped`]. - /// - /// [`World::validate_non_send_access_untyped`]: crate::world::World::validate_non_send_access_untyped #[inline] pub(crate) unsafe fn insert_with_ticks( &mut self, @@ -146,13 +149,16 @@ impl ResourceData { /// Removes a value from the resource, if present. /// + /// # Aborts + /// This will abort the process if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. + /// This function will panic instead in tests. + /// /// # Safety - /// The underlying type must be [`Send`] or be removed from the main thread. - /// This can be validated with [`World::validate_non_send_access_untyped`]. + /// The underlying type must be [`Send`] or be removed from the thread it was + /// inserted from. /// /// The removed value must be used or dropped. - /// - /// [`World::validate_non_send_access_untyped`]: crate::world::World::validate_non_send_access_untyped #[inline] #[must_use = "The returned pointer to the removed component should be used or dropped"] pub(crate) unsafe fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { @@ -163,11 +169,14 @@ impl ResourceData { /// Removes a value from the resource, if present, and drops it. /// - /// # Safety - /// The underlying type must be [`Send`] or be removed from the main thread. - /// This can be validated with [`World::validate_non_send_access_untyped`]. + /// # Aborts + /// This will abort the process if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. + /// This function will panic instead in tests. /// - /// [`World::validate_non_send_access_untyped`]: crate::world::World::validate_non_send_access_untyped + /// # Safety + /// The underlying type must be [`Send`] or be removed from the thread it was + /// inserted from. #[inline] pub(crate) unsafe fn remove_and_drop(&mut self) { if self.is_present() { From 944b83c008be8f097e2b57b07930477bc5f2df08 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 16 Nov 2022 10:51:07 -0800 Subject: [PATCH 07/48] Switch to always panic, use ManuallyDrop --- crates/bevy_ecs/src/storage/resource.rs | 60 +++++++++++-------------- 1 file changed, 26 insertions(+), 34 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index be8cc26ac8dc7..ae6bd3a92501a 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -2,18 +2,15 @@ use crate::archetype::ArchetypeComponentId; use crate::component::{ComponentId, ComponentTicks, Components}; use crate::storage::{Column, SparseSet}; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; -#[allow(unused_imports)] -use bevy_utils::tracing::error; -#[allow(unused_imports)] -use std::borrow::Cow; use std::cell::UnsafeCell; use std::thread::ThreadId; +use std::mem::ManuallyDrop; /// The type-erased backing storage and metadata for a single resource within a [`World`]. /// /// [`World`]: crate::world::World pub struct ResourceData { - column: Column, + column: ManuallyDrop, type_name: String, id: ArchetypeComponentId, origin_thread_id: Option, @@ -24,6 +21,10 @@ impl Drop for ResourceData { if self.is_present() { self.validate_access(); } + // SAFETY: Drop is only called once upon dropping the ResourceData + // and is inaccessible after this as the parent ResourceData has + // been dropped. + unsafe { ManuallyDrop::drop(&mut self.column); } } } @@ -31,30 +32,18 @@ impl ResourceData { #[inline] fn validate_access(&self) { // Avoid aborting due to double panicking on the same thread. - #[cfg(test)] if std::thread::panicking() { return; } if let Some(origin_thread_id) = self.origin_thread_id { if origin_thread_id != std::thread::current().id() { // Panic in tests, as testing for aborting is nearly impossible - #[cfg(test)] panic!( "Attempted to access or drop non-send resource {} from thread {:?} on a thread {:?}. This is not allowed. Aborting.", self.type_name, origin_thread_id, std::thread::current().id() ); - #[cfg(not(test))] - { - error!( - "Attempted to access or drop non-send resource {} from thread {:?} on a thread {:?}. This is not allowed. Aborting.", - self.type_name, - origin_thread_id, - std::thread::current().id() - ); - std::process::abort(); - } } } } @@ -72,6 +61,10 @@ impl ResourceData { } /// Gets a read-only pointer to the underlying resource, if available. + /// + /// # Panics + /// This will panic if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. #[inline] pub fn get_data(&self) -> Option> { self.column.get_data(0).map(|res| { @@ -92,6 +85,9 @@ impl ResourceData { .map(|ticks| unsafe { ticks.deref() }) } + /// # Panics + /// This will panic if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. #[inline] pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, &UnsafeCell)> { self.column.get(0).map(|res| { @@ -102,11 +98,10 @@ impl ResourceData { /// Inserts a value into the resource. If a value is already present /// it will be replaced. - /// - /// # Aborts - /// This will abort the process if a value is present, the underlying type is + /// + /// # Panics + /// This will panic if a value is present, the underlying type is /// `!Send`, and is not accessed from the original thread it was inserted in. - /// This function will panic instead in tests. /// /// # Safety /// `value` must be valid for the underlying type for the resource. @@ -123,11 +118,10 @@ impl ResourceData { /// Inserts a value into the resource with a pre-existing change tick. If a /// value is already present it will be replaced. - /// - /// # Aborts - /// This will abort the process if a value is present, the underlying type is + /// + /// # Panics + /// This will panic if a value is present, the underlying type is /// `!Send`, and is not accessed from the original thread it was inserted in. - /// This function will panic instead in tests. /// /// # Safety /// `value` must be valid for the underlying type for the resource. @@ -148,11 +142,10 @@ impl ResourceData { } /// Removes a value from the resource, if present. - /// - /// # Aborts - /// This will abort the process if a value is present, the underlying type is + /// + /// # Panics + /// This will panic if a value is present, the underlying type is /// `!Send`, and is not accessed from the original thread it was inserted in. - /// This function will panic instead in tests. /// /// # Safety /// The underlying type must be [`Send`] or be removed from the thread it was @@ -168,11 +161,10 @@ impl ResourceData { } /// Removes a value from the resource, if present, and drops it. - /// - /// # Aborts - /// This will abort the process if a value is present, the underlying type is + /// + /// # Panics + /// This will panic if a value is present, the underlying type is /// `!Send`, and is not accessed from the original thread it was inserted in. - /// This function will panic instead in tests. /// /// # Safety /// The underlying type must be [`Send`] or be removed from the thread it was @@ -242,7 +234,7 @@ impl Resources { self.resources.get_or_insert_with(component_id, || { let component_info = components.get_info(component_id).unwrap(); ResourceData { - column: Column::with_capacity(component_info, 1), + column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), type_name: String::from(component_info.name()), id: f(), origin_thread_id: (!is_send).then(|| std::thread::current().id()), From 9b712df514a7e1085a6e45acf949a5cc30539a36 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 16 Nov 2022 10:56:07 -0800 Subject: [PATCH 08/48] Formatting --- crates/bevy_ecs/src/storage/resource.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index ae6bd3a92501a..68bffa6a829c1 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -3,8 +3,8 @@ use crate::component::{ComponentId, ComponentTicks, Components}; use crate::storage::{Column, SparseSet}; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; use std::cell::UnsafeCell; -use std::thread::ThreadId; use std::mem::ManuallyDrop; +use std::thread::ThreadId; /// The type-erased backing storage and metadata for a single resource within a [`World`]. /// @@ -22,9 +22,11 @@ impl Drop for ResourceData { self.validate_access(); } // SAFETY: Drop is only called once upon dropping the ResourceData - // and is inaccessible after this as the parent ResourceData has + // and is inaccessible after this as the parent ResourceData has // been dropped. - unsafe { ManuallyDrop::drop(&mut self.column); } + unsafe { + ManuallyDrop::drop(&mut self.column); + } } } @@ -98,7 +100,7 @@ impl ResourceData { /// Inserts a value into the resource. If a value is already present /// it will be replaced. - /// + /// /// # Panics /// This will panic if a value is present, the underlying type is /// `!Send`, and is not accessed from the original thread it was inserted in. @@ -118,7 +120,7 @@ impl ResourceData { /// Inserts a value into the resource with a pre-existing change tick. If a /// value is already present it will be replaced. - /// + /// /// # Panics /// This will panic if a value is present, the underlying type is /// `!Send`, and is not accessed from the original thread it was inserted in. @@ -142,7 +144,7 @@ impl ResourceData { } /// Removes a value from the resource, if present. - /// + /// /// # Panics /// This will panic if a value is present, the underlying type is /// `!Send`, and is not accessed from the original thread it was inserted in. @@ -161,7 +163,7 @@ impl ResourceData { } /// Removes a value from the resource, if present, and drops it. - /// + /// /// # Panics /// This will panic if a value is present, the underlying type is /// `!Send`, and is not accessed from the original thread it was inserted in. From ba2713641d40e3c00176913878dae663980e4280 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 1 Dec 2022 20:05:30 -0800 Subject: [PATCH 09/48] Split out into NonSendResources --- crates/bevy_ecs/src/storage/mod.rs | 3 + .../bevy_ecs/src/storage/non_send_resource.rs | 252 ++++++++++++++++++ crates/bevy_ecs/src/storage/resource.rs | 55 +--- 3 files changed, 259 insertions(+), 51 deletions(-) create mode 100644 crates/bevy_ecs/src/storage/non_send_resource.rs diff --git a/crates/bevy_ecs/src/storage/mod.rs b/crates/bevy_ecs/src/storage/mod.rs index 6e848a042b492..981cfdf4fa904 100644 --- a/crates/bevy_ecs/src/storage/mod.rs +++ b/crates/bevy_ecs/src/storage/mod.rs @@ -2,10 +2,12 @@ mod blob_vec; mod resource; +mod non_send_resource; mod sparse_set; mod table; pub use resource::*; +pub use non_send_resource::*; pub use sparse_set::*; pub use table::*; @@ -15,4 +17,5 @@ pub struct Storages { pub sparse_sets: SparseSets, pub tables: Tables, pub resources: Resources, + pub non_send_resource: NonSendResources, } diff --git a/crates/bevy_ecs/src/storage/non_send_resource.rs b/crates/bevy_ecs/src/storage/non_send_resource.rs new file mode 100644 index 0000000000000..2e08c556494af --- /dev/null +++ b/crates/bevy_ecs/src/storage/non_send_resource.rs @@ -0,0 +1,252 @@ +use crate::archetype::ArchetypeComponentId; +use crate::component::{ComponentId, ComponentTicks, Components}; +use crate::storage::{Column, SparseSet}; +use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; +use std::cell::UnsafeCell; +use std::mem::ManuallyDrop; +use std::thread::ThreadId; + +/// The type-erased backing storage and metadata for a single resource within a [`World`]. +/// +/// [`World`]: crate::world::World +pub struct NonSendResourceData { + column: ManuallyDrop, + type_name: String, + id: ArchetypeComponentId, + origin_thread_id: Option, +} + +impl Drop for NonSendResourceData { + fn drop(&mut self) { + if self.is_present() { + self.validate_access(); + } + // SAFETY: Drop is only called once upon dropping the ResourceData + // and is inaccessible after this as the parent ResourceData has + // been dropped. + unsafe { + ManuallyDrop::drop(&mut self.column); + } + } +} + +impl NonSendResourceData { + #[inline] + fn validate_access(&self) { + // Avoid aborting due to double panicking on the same thread. + if std::thread::panicking() { + return; + } + if let Some(origin_thread_id) = self.origin_thread_id { + if origin_thread_id != std::thread::current().id() { + // Panic in tests, as testing for aborting is nearly impossible + panic!( + "Attempted to access or drop non-send resource {} from thread {:?} on a thread {:?}. This is not allowed. Aborting.", + self.type_name, + origin_thread_id, + std::thread::current().id() + ); + } + } + } + + /// Returns true if the resource is populated. + #[inline] + pub fn is_present(&self) -> bool { + !self.column.is_empty() + } + + /// Gets the [`ArchetypeComponentId`] for the resource. + #[inline] + pub fn id(&self) -> ArchetypeComponentId { + self.id + } + + /// Gets a read-only pointer to the underlying resource, if available. + /// + /// # Panics + /// This will panic if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. + #[inline] + pub fn get_data(&self) -> Option> { + self.column.get_data(0).map(|res| { + self.validate_access(); + res + }) + } + + /// Gets a read-only reference to the change ticks of the underlying resource, if available. + #[inline] + pub fn get_ticks(&self) -> Option<&ComponentTicks> { + self.column + .get_ticks(0) + // SAFETY: + // - This borrow's lifetime is bounded by the lifetime on self. + // - A read-only borrow on self can only exist while a mutable borrow doesn't + // exist. + .map(|ticks| unsafe { ticks.deref() }) + } + + /// # Panics + /// This will panic if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. + #[inline] + pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, &UnsafeCell)> { + self.column.get(0).map(|res| { + self.validate_access(); + res + }) + } + + /// Inserts a value into the resource. If a value is already present + /// it will be replaced. + /// + /// # Panics + /// This will panic if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. + /// + /// # Safety + /// `value` must be valid for the underlying type for the resource. + #[inline] + pub(crate) unsafe fn insert(&mut self, value: OwningPtr<'_>, change_tick: u32) { + if self.is_present() { + self.validate_access(); + self.column.replace(0, value, change_tick); + } else { + self.origin_thread_id = self.origin_thread_id.map(|_| std::thread::current().id()); + self.column.push(value, ComponentTicks::new(change_tick)); + } + } + + /// Inserts a value into the resource with a pre-existing change tick. If a + /// value is already present it will be replaced. + /// + /// # Panics + /// This will panic if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. + /// + /// # Safety + /// `value` must be valid for the underlying type for the resource. + #[inline] + pub(crate) unsafe fn insert_with_ticks( + &mut self, + value: OwningPtr<'_>, + change_ticks: ComponentTicks, + ) { + if self.is_present() { + self.validate_access(); + self.column.replace_untracked(0, value); + *self.column.get_ticks_unchecked(0).deref_mut() = change_ticks; + } else { + self.origin_thread_id = self.origin_thread_id.map(|_| std::thread::current().id()); + self.column.push(value, change_ticks); + } + } + + /// Removes a value from the resource, if present. + /// + /// # Panics + /// This will panic if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. + /// + /// # Safety + /// The underlying type must be [`Send`] or be removed from the thread it was + /// inserted from. + /// + /// The removed value must be used or dropped. + #[inline] + #[must_use = "The returned pointer to the removed component should be used or dropped"] + pub(crate) unsafe fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { + self.is_present() + .then(|| self.validate_access()) + .and_then(|_| self.column.swap_remove_and_forget(0)) + } + + /// Removes a value from the resource, if present, and drops it. + /// + /// # Panics + /// This will panic if a value is present, the underlying type is + /// `!Send`, and is not accessed from the original thread it was inserted in. + /// + /// # Safety + /// The underlying type must be [`Send`] or be removed from the thread it was + /// inserted from. + #[inline] + pub(crate) unsafe fn remove_and_drop(&mut self) { + if self.is_present() { + self.validate_access(); + self.column.clear(); + } + } +} + +/// The backing store for all [`Resource`]s stored in the [`World`]. +/// +/// [`Resource`]: crate::system::Resource +/// [`World`]: crate::world::World +#[derive(Default)] +pub struct NonSendResources { + resources: SparseSet, +} + +impl NonSendResources { + /// The total number of resources stored in the [`World`] + /// + /// [`World`]: crate::world::World + #[inline] + pub fn len(&self) -> usize { + self.resources.len() + } + + /// Returns true if there are no resources stored in the [`World`], + /// false otherwise. + /// + /// [`World`]: crate::world::World + #[inline] + pub fn is_empty(&self) -> bool { + self.resources.is_empty() + } + + /// Gets read-only access to a resource, if it exists. + #[inline] + pub fn get(&self, component_id: ComponentId) -> Option<&NonSendResourceData> { + self.resources.get(component_id) + } + + /// Gets mutable access to a resource, if it exists. + #[inline] + pub(crate) fn get_mut(&mut self, component_id: ComponentId) -> Option<&mut NonSendResourceData> { + self.resources.get_mut(component_id) + } + + /// Fetches or initializes a new resource and returns back it's underlying column. + /// + /// # Panics + /// Will panic if `component_id` is not valid for the provided `components` + /// + /// # Safety + /// `is_send` must be accurate for the Resource that is being initialized. + pub(crate) unsafe fn initialize_with( + &mut self, + component_id: ComponentId, + components: &Components, + is_send: bool, + f: impl FnOnce() -> ArchetypeComponentId, + ) -> &mut NonSendResourceData { + self.resources.get_or_insert_with(component_id, || { + let component_info = components.get_info(component_id).unwrap(); + NonSendResourceData { + column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), + type_name: String::from(component_info.name()), + id: f(), + origin_thread_id: (!is_send).then(|| std::thread::current().id()), + } + }) + } + + pub(crate) fn check_change_ticks(&mut self, change_tick: u32) { + for info in self.resources.values_mut() { + info.column.check_change_ticks(change_tick); + } + } +} diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 944d943e23440..97f93ada93602 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -10,46 +10,11 @@ use std::thread::ThreadId; /// /// [`World`]: crate::world::World pub struct ResourceData { - column: ManuallyDrop, - type_name: String, + column: Column, id: ArchetypeComponentId, - origin_thread_id: Option, -} - -impl Drop for ResourceData { - fn drop(&mut self) { - if self.is_present() { - self.validate_access(); - } - // SAFETY: Drop is only called once upon dropping the ResourceData - // and is inaccessible after this as the parent ResourceData has - // been dropped. - unsafe { - ManuallyDrop::drop(&mut self.column); - } - } } impl ResourceData { - #[inline] - fn validate_access(&self) { - // Avoid aborting due to double panicking on the same thread. - if std::thread::panicking() { - return; - } - if let Some(origin_thread_id) = self.origin_thread_id { - if origin_thread_id != std::thread::current().id() { - // Panic in tests, as testing for aborting is nearly impossible - panic!( - "Attempted to access or drop non-send resource {} from thread {:?} on a thread {:?}. This is not allowed. Aborting.", - self.type_name, - origin_thread_id, - std::thread::current().id() - ); - } - } - } - /// Returns true if the resource is populated. #[inline] pub fn is_present(&self) -> bool { @@ -69,10 +34,7 @@ impl ResourceData { /// `!Send`, and is not accessed from the original thread it was inserted in. #[inline] pub fn get_data(&self) -> Option> { - self.column.get_data(0).map(|res| { - self.validate_access(); - res - }) + self.column.get_data(0) } /// Gets a read-only reference to the change ticks of the underlying resource, if available. @@ -104,10 +66,8 @@ impl ResourceData { #[inline] pub(crate) unsafe fn insert(&mut self, value: OwningPtr<'_>, change_tick: u32) { if self.is_present() { - self.validate_access(); self.column.replace(0, value, change_tick); } else { - self.origin_thread_id = self.origin_thread_id.map(|_| std::thread::current().id()); self.column.push(value, ComponentTicks::new(change_tick)); } } @@ -128,12 +88,10 @@ impl ResourceData { change_ticks: ComponentTicks, ) { if self.is_present() { - self.validate_access(); self.column.replace_untracked(0, value); *self.column.get_added_ticks_unchecked(0).deref_mut() = change_ticks.added; *self.column.get_changed_ticks_unchecked(0).deref_mut() = change_ticks.changed; } else { - self.origin_thread_id = self.origin_thread_id.map(|_| std::thread::current().id()); self.column.push(value, change_ticks); } } @@ -152,9 +110,7 @@ impl ResourceData { #[inline] #[must_use = "The returned pointer to the removed component should be used or dropped"] pub(crate) unsafe fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { - self.is_present() - .then(|| self.validate_access()) - .and_then(|_| self.column.swap_remove_and_forget(0)) + self.column.swap_remove_and_forget(0) } /// Removes a value from the resource, if present, and drops it. @@ -169,7 +125,6 @@ impl ResourceData { #[inline] pub(crate) unsafe fn remove_and_drop(&mut self) { if self.is_present() { - self.validate_access(); self.column.clear(); } } @@ -236,10 +191,8 @@ impl Resources { self.resources.get_or_insert_with(component_id, || { let component_info = components.get_info(component_id).unwrap(); ResourceData { - column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), - type_name: String::from(component_info.name()), + column: Column::with_capacity(component_info, 1), id: f(), - origin_thread_id: (!is_send).then(|| std::thread::current().id()), } }) } From c3708d4aa1adcf4de80ddcfb2d9ae209fd253dc0 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 1 Dec 2022 18:49:58 -0800 Subject: [PATCH 10/48] Cleanup docs and type definitions --- .../bevy_ecs/src/storage/non_send_resource.rs | 60 +++++++++---------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/crates/bevy_ecs/src/storage/non_send_resource.rs b/crates/bevy_ecs/src/storage/non_send_resource.rs index 2e08c556494af..9f7521db6e030 100644 --- a/crates/bevy_ecs/src/storage/non_send_resource.rs +++ b/crates/bevy_ecs/src/storage/non_send_resource.rs @@ -13,7 +13,7 @@ pub struct NonSendResourceData { column: ManuallyDrop, type_name: String, id: ArchetypeComponentId, - origin_thread_id: Option, + origin_thread_id: ThreadId, } impl Drop for NonSendResourceData { @@ -37,32 +37,30 @@ impl NonSendResourceData { if std::thread::panicking() { return; } - if let Some(origin_thread_id) = self.origin_thread_id { - if origin_thread_id != std::thread::current().id() { - // Panic in tests, as testing for aborting is nearly impossible - panic!( - "Attempted to access or drop non-send resource {} from thread {:?} on a thread {:?}. This is not allowed. Aborting.", - self.type_name, - origin_thread_id, - std::thread::current().id() - ); - } + if self.origin_thread_id != std::thread::current().id() { + // Panic in tests, as testing for aborting is nearly impossible + panic!( + "Attempted to access or drop non-send resource {} from thread {:?} on a thread {:?}. This is not allowed. Aborting.", + self.type_name, + self.origin_thread_id, + std::thread::current().id() + ); } } - /// Returns true if the resource is populated. + /// Returns true if the `!Send` resource is populated. #[inline] pub fn is_present(&self) -> bool { !self.column.is_empty() } - /// Gets the [`ArchetypeComponentId`] for the resource. + /// Gets the [`ArchetypeComponentId`] for the `!Send` resource. #[inline] pub fn id(&self) -> ArchetypeComponentId { self.id } - /// Gets a read-only pointer to the underlying resource, if available. + /// Gets a read-only pointer to the underlying `!Send` resource, if available. /// /// # Panics /// This will panic if a value is present, the underlying type is @@ -88,8 +86,8 @@ impl NonSendResourceData { } /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. + /// This will panic if a value is present and is not accessed from the original thread + /// it was inserted in. #[inline] pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, &UnsafeCell)> { self.column.get(0).map(|res| { @@ -98,12 +96,12 @@ impl NonSendResourceData { }) } - /// Inserts a value into the resource. If a value is already present + /// Inserts a value into the `!Send` resource. If a value is already present /// it will be replaced. /// /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. + /// This will panic if a value is present and is not accessed from the original thread + /// it was inserted in. /// /// # Safety /// `value` must be valid for the underlying type for the resource. @@ -113,13 +111,13 @@ impl NonSendResourceData { self.validate_access(); self.column.replace(0, value, change_tick); } else { - self.origin_thread_id = self.origin_thread_id.map(|_| std::thread::current().id()); + self.origin_thread_id = std::thread::current().id(); self.column.push(value, ComponentTicks::new(change_tick)); } } - /// Inserts a value into the resource with a pre-existing change tick. If a - /// value is already present it will be replaced. + /// Inserts a value into the `!Send` resource with a pre-existing change tick. + /// If a value is already present it will be replaced. /// /// # Panics /// This will panic if a value is present, the underlying type is @@ -138,12 +136,12 @@ impl NonSendResourceData { self.column.replace_untracked(0, value); *self.column.get_ticks_unchecked(0).deref_mut() = change_ticks; } else { - self.origin_thread_id = self.origin_thread_id.map(|_| std::thread::current().id()); + self.origin_thread_id = std::thread::current().id(); self.column.push(value, change_ticks); } } - /// Removes a value from the resource, if present. + /// Removes a value from the `!Send` resource, if present. /// /// # Panics /// This will panic if a value is present, the underlying type is @@ -180,7 +178,7 @@ impl NonSendResourceData { } } -/// The backing store for all [`Resource`]s stored in the [`World`]. +/// The backing store for all `!Send` [`Resource`]s stored in the [`World`]. /// /// [`Resource`]: crate::system::Resource /// [`World`]: crate::world::World @@ -190,7 +188,7 @@ pub struct NonSendResources { } impl NonSendResources { - /// The total number of resources stored in the [`World`] + /// The total number of `!Send` resources stored in the [`World`] /// /// [`World`]: crate::world::World #[inline] @@ -198,7 +196,7 @@ impl NonSendResources { self.resources.len() } - /// Returns true if there are no resources stored in the [`World`], + /// Returns true if there are no `!Send` resources stored in the [`World`], /// false otherwise. /// /// [`World`]: crate::world::World @@ -207,19 +205,19 @@ impl NonSendResources { self.resources.is_empty() } - /// Gets read-only access to a resource, if it exists. + /// Gets read-only access to a `!Send` resource, if it exists. #[inline] pub fn get(&self, component_id: ComponentId) -> Option<&NonSendResourceData> { self.resources.get(component_id) } - /// Gets mutable access to a resource, if it exists. + /// Gets mutable access to a `!Send` resource, if it exists. #[inline] pub(crate) fn get_mut(&mut self, component_id: ComponentId) -> Option<&mut NonSendResourceData> { self.resources.get_mut(component_id) } - /// Fetches or initializes a new resource and returns back it's underlying column. + /// Fetches or initializes a new `!Send` resource and returns back it's underlying column. /// /// # Panics /// Will panic if `component_id` is not valid for the provided `components` @@ -239,7 +237,7 @@ impl NonSendResources { column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), type_name: String::from(component_info.name()), id: f(), - origin_thread_id: (!is_send).then(|| std::thread::current().id()), + origin_thread_id: std::thread::current().id(), } }) } From 16ae49180a73a8a8bf3ae4aa302b31b1fdf74aab Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 1 Dec 2022 20:04:26 -0800 Subject: [PATCH 11/48] Fix CI --- crates/bevy_ecs/src/lib.rs | 6 +- .../src/schedule/ambiguity_detection.rs | 4 + crates/bevy_ecs/src/storage/mod.rs | 6 +- .../bevy_ecs/src/storage/non_send_resource.rs | 12 +- crates/bevy_ecs/src/storage/resource.rs | 6 - crates/bevy_ecs/src/system/system_param.rs | 12 +- crates/bevy_ecs/src/world/mod.rs | 227 +++++++++++++++--- 7 files changed, 217 insertions(+), 56 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index d0e05604394c8..1f7e9a04dedb3 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1325,9 +1325,9 @@ mod tests { fn non_send_resource_scope() { let mut world = World::default(); world.insert_non_send_resource(NonSendA::default()); - world.resource_scope(|world: &mut World, mut value: Mut| { + world.non_send_scope(|world: &mut World, mut value: Mut| { value.0 += 1; - assert!(!world.contains_resource::()); + assert!(!world.contains_non_send::()); }); assert_eq!(world.non_send_resource::().0, 1); } @@ -1343,7 +1343,7 @@ mod tests { let thread = std::thread::spawn(move || { // Accessing the non-send resource on a different thread // Should result in a panic - world.resource_scope(|_: &mut World, mut value: Mut| { + world.non_send_scope(|_: &mut World, mut value: Mut| { value.0 += 1; }); }); diff --git a/crates/bevy_ecs/src/schedule/ambiguity_detection.rs b/crates/bevy_ecs/src/schedule/ambiguity_detection.rs index 558754c9dfcd5..5e1269309154d 100644 --- a/crates/bevy_ecs/src/schedule/ambiguity_detection.rs +++ b/crates/bevy_ecs/src/schedule/ambiguity_detection.rs @@ -335,6 +335,7 @@ mod tests { fn read_only() { let mut world = World::new(); world.insert_resource(R); + world.insert_non_send_resource(R); world.spawn(A); world.init_resource::>(); @@ -394,6 +395,7 @@ mod tests { fn nonsend() { let mut world = World::new(); world.insert_resource(R); + world.insert_non_send_resource(R); let mut test_stage = SystemStage::parallel(); test_stage @@ -497,6 +499,7 @@ mod tests { fn ignore_all_ambiguities() { let mut world = World::new(); world.insert_resource(R); + world.insert_non_send_resource(R); let mut test_stage = SystemStage::parallel(); test_stage @@ -513,6 +516,7 @@ mod tests { fn ambiguous_with_label() { let mut world = World::new(); world.insert_resource(R); + world.insert_non_send_resource(R); #[derive(SystemLabel)] struct IgnoreMe; diff --git a/crates/bevy_ecs/src/storage/mod.rs b/crates/bevy_ecs/src/storage/mod.rs index 981cfdf4fa904..215ecd0bbdf53 100644 --- a/crates/bevy_ecs/src/storage/mod.rs +++ b/crates/bevy_ecs/src/storage/mod.rs @@ -1,13 +1,13 @@ //! Storage layouts for ECS data. mod blob_vec; -mod resource; mod non_send_resource; +mod resource; mod sparse_set; mod table; -pub use resource::*; pub use non_send_resource::*; +pub use resource::*; pub use sparse_set::*; pub use table::*; @@ -17,5 +17,5 @@ pub struct Storages { pub sparse_sets: SparseSets, pub tables: Tables, pub resources: Resources, - pub non_send_resource: NonSendResources, + pub non_send_resources: NonSendResources, } diff --git a/crates/bevy_ecs/src/storage/non_send_resource.rs b/crates/bevy_ecs/src/storage/non_send_resource.rs index 9f7521db6e030..b1f53406bf086 100644 --- a/crates/bevy_ecs/src/storage/non_send_resource.rs +++ b/crates/bevy_ecs/src/storage/non_send_resource.rs @@ -86,7 +86,7 @@ impl NonSendResourceData { } /// # Panics - /// This will panic if a value is present and is not accessed from the original thread + /// This will panic if a value is present and is not accessed from the original thread /// it was inserted in. #[inline] pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, &UnsafeCell)> { @@ -100,7 +100,7 @@ impl NonSendResourceData { /// it will be replaced. /// /// # Panics - /// This will panic if a value is present and is not accessed from the original thread + /// This will panic if a value is present and is not accessed from the original thread /// it was inserted in. /// /// # Safety @@ -116,7 +116,7 @@ impl NonSendResourceData { } } - /// Inserts a value into the `!Send` resource with a pre-existing change tick. + /// Inserts a value into the `!Send` resource with a pre-existing change tick. /// If a value is already present it will be replaced. /// /// # Panics @@ -213,7 +213,10 @@ impl NonSendResources { /// Gets mutable access to a `!Send` resource, if it exists. #[inline] - pub(crate) fn get_mut(&mut self, component_id: ComponentId) -> Option<&mut NonSendResourceData> { + pub(crate) fn get_mut( + &mut self, + component_id: ComponentId, + ) -> Option<&mut NonSendResourceData> { self.resources.get_mut(component_id) } @@ -228,7 +231,6 @@ impl NonSendResources { &mut self, component_id: ComponentId, components: &Components, - is_send: bool, f: impl FnOnce() -> ArchetypeComponentId, ) -> &mut NonSendResourceData { self.resources.get_or_insert_with(component_id, || { diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 97f93ada93602..801c1f5678abb 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -3,8 +3,6 @@ use crate::component::{ComponentId, ComponentTicks, Components, TickCells}; use crate::storage::{Column, SparseSet}; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; use std::cell::UnsafeCell; -use std::mem::ManuallyDrop; -use std::thread::ThreadId; /// The type-erased backing storage and metadata for a single resource within a [`World`]. /// @@ -178,14 +176,10 @@ impl Resources { /// /// # Panics /// Will panic if `component_id` is not valid for the provided `components` - /// - /// # Safety - /// `is_send` must be accurate for the Resource that is being initialized. pub(crate) unsafe fn initialize_with( &mut self, component_id: ComponentId, components: &Components, - is_send: bool, f: impl FnOnce() -> ArchetypeComponentId, ) -> &mut ResourceData { self.resources.get_or_insert_with(component_id, || { diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 63cfd24e8be7d..c48131b07eabd 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -990,7 +990,7 @@ unsafe impl SystemParamState for NonSendState { .add_unfiltered_read(component_id); let archetype_component_id = world - .get_resource_archetype_component_id(component_id) + .get_non_send_archetype_component_id(component_id) .unwrap(); system_meta .archetype_component_access @@ -1013,7 +1013,7 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for NonSendState { change_tick: u32, ) -> Self::Item { let (ptr, ticks) = world - .get_resource_with_ticks(state.component_id) + .get_non_send_with_ticks(state.component_id) .unwrap_or_else(|| { panic!( "Non-send resource requested by {} does not exist: {}", @@ -1062,7 +1062,7 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for OptionNonSendState { change_tick: u32, ) -> Self::Item { world - .get_resource_with_ticks(state.0.component_id) + .get_non_send_with_ticks(state.0.component_id) .map(|(ptr, ticks)| NonSend { value: ptr.deref(), ticks: ticks.read(), @@ -1105,7 +1105,7 @@ unsafe impl SystemParamState for NonSendMutState { .add_unfiltered_write(component_id); let archetype_component_id = world - .get_resource_archetype_component_id(component_id) + .get_non_send_archetype_component_id(component_id) .unwrap(); system_meta .archetype_component_access @@ -1128,7 +1128,7 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for NonSendMutState { change_tick: u32, ) -> Self::Item { let (ptr, ticks) = world - .get_resource_with_ticks(state.component_id) + .get_non_send_with_ticks(state.component_id) .unwrap_or_else(|| { panic!( "Non-send resource requested by {} does not exist: {}", @@ -1171,7 +1171,7 @@ impl<'w, 's, T: 'static> SystemParamFetch<'w, 's> for OptionNonSendMutState { change_tick: u32, ) -> Self::Item { world - .get_resource_with_ticks(state.0.component_id) + .get_non_send_with_ticks(state.0.component_id) .map(|(ptr, ticks)| NonSendMut { value: ptr.assert_unique().deref_mut(), ticks: Ticks::from_tick_cells(ticks, system_meta.last_change_tick, change_tick), diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 2dad8f7a0a6c7..27f8ec4e4c0c0 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -16,7 +16,7 @@ use crate::{ }, entity::{AllocAtWithoutReplacement, Entities, Entity}, query::{QueryState, ReadOnlyWorldQuery, WorldQuery}, - storage::{ResourceData, SparseSet, Storages}, + storage::{NonSendResourceData, ResourceData, SparseSet, Storages}, system::Resource, }; use bevy_ptr::{OwningPtr, Ptr}; @@ -751,7 +751,7 @@ impl World { /// Panics if called from a thread other than the main thread. #[inline] pub fn init_non_send_resource(&mut self) { - if !self.contains_resource::() { + if !self.contains_non_send::() { let resource = R::from_world(self); self.insert_non_send_resource(resource); } @@ -780,33 +780,34 @@ impl World { /// Removes the resource of a given type and returns it, if it exists. Otherwise returns [None]. #[inline] pub fn remove_resource(&mut self) -> Option { - // SAFETY: R is Send + Sync - unsafe { self.remove_resource_unchecked() } + let component_id = self.components.get_resource_id(TypeId::of::())?; + // SAFETY: the resource is of type R and the value is returned back to the caller. + unsafe { + let (ptr, _) = self.storages.resources.get_mut(component_id)?.remove()?; + Some(ptr.read::()) + } } + /// # Panic + /// Will panic if removing `NonSend` resources from threads other than where they were + /// inserted from as they cannot be sent across threads #[inline] pub fn remove_non_send_resource(&mut self) -> Option { - // SAFETY: we are on main thread - unsafe { self.remove_resource_unchecked() } - } - - #[inline] - /// # Safety - /// Only remove `NonSend` resources from the main thread - /// as they cannot be sent across threads - #[allow(unused_unsafe)] - pub unsafe fn remove_resource_unchecked(&mut self) -> Option { let component_id = self.components.get_resource_id(TypeId::of::())?; // SAFETY: the resource is of type R and the value is returned back to the caller. unsafe { - let (ptr, _) = self.storages.resources.get_mut(component_id)?.remove()?; + let (ptr, _) = self + .storages + .non_send_resources + .get_mut(component_id)? + .remove()?; Some(ptr.read::()) } } /// Returns `true` if a resource of type `R` exists. Otherwise returns `false`. #[inline] - pub fn contains_resource(&self) -> bool { + pub fn contains_resource(&self) -> bool { self.components .get_resource_id(TypeId::of::()) .and_then(|component_id| self.storages.resources.get(component_id)) @@ -814,6 +815,16 @@ impl World { .unwrap_or(false) } + /// Returns `true` if a resource of type `R` exists. Otherwise returns `false`. + #[inline] + pub fn contains_non_send(&self) -> bool { + self.components + .get_resource_id(TypeId::of::()) + .and_then(|component_id| self.storages.non_send_resources.get(component_id)) + .map(|info| info.is_present()) + .unwrap_or(false) + } + pub fn is_resource_added(&self) -> bool { self.components .get_resource_id(TypeId::of::()) @@ -997,6 +1008,18 @@ impl World { self.storages.resources.get(component_id)?.get_with_ticks() } + // Shorthand helper function for getting the data and change ticks for a resource. + #[inline] + pub(crate) fn get_non_send_with_ticks( + &self, + component_id: ComponentId, + ) -> Option<(Ptr<'_>, &UnsafeCell)> { + self.storages + .non_send_resources + .get(component_id)? + .get_with_ticks() + } + // Shorthand helper function for getting the [`ArchetypeComponentId`] for a resource. #[inline] pub(crate) fn get_resource_archetype_component_id( @@ -1007,6 +1030,16 @@ impl World { Some(resource.id()) } + // Shorthand helper function for getting the [`ArchetypeComponentId`] for a resource. + #[inline] + pub(crate) fn get_non_send_archetype_component_id( + &self, + component_id: ComponentId, + ) -> Option { + let resource = self.storages.non_send_resources.get(component_id)?; + Some(resource.id()) + } + /// For a given batch of ([Entity], [Bundle]) pairs, either spawns each [Entity] with the given /// bundle (if the entity does not exist), or inserts the [Bundle] (if the entity already exists). /// This is faster than doing equivalent operations one-by-one. @@ -1153,13 +1186,7 @@ impl World { /// }); /// assert_eq!(world.get_resource::().unwrap().0, 2); /// ``` - pub fn resource_scope< - R: 'static, /* The resource doesn't need to be Send nor Sync. */ - U, - >( - &mut self, - f: impl FnOnce(&mut World, Mut) -> U, - ) -> U { + pub fn resource_scope(&mut self, f: impl FnOnce(&mut World, Mut) -> U) -> U { let last_change_tick = self.last_change_tick(); let change_tick = self.change_tick(); @@ -1213,6 +1240,82 @@ impl World { result } + /// Temporarily removes the requested `!Send` resource from this [`World`], then re-adds it before + /// returning. + /// + /// This enables safe simultaneous mutable access to both a resource and the rest of the [`World`]. + /// For more complex access patterns, consider using [`SystemState`](crate::system::SystemState). + /// + /// # Example + /// ``` + /// use bevy_ecs::prelude::*; + /// #[derive(Resource)] + /// struct A(u32); + /// #[derive(Component)] + /// struct B(u32); + /// let mut world = World::new(); + /// world.insert_resource(A(1)); + /// let entity = world.spawn(B(1)).id(); + /// + /// world.resource_scope(|world, mut a: Mut| { + /// let b = world.get_mut::(entity).unwrap(); + /// a.0 += b.0; + /// }); + /// assert_eq!(world.get_resource::().unwrap().0, 2); + /// ``` + pub fn non_send_scope(&mut self, f: impl FnOnce(&mut World, Mut) -> U) -> U { + let last_change_tick = self.last_change_tick(); + let change_tick = self.change_tick(); + + let component_id = self + .components + .get_resource_id(TypeId::of::()) + .unwrap_or_else(|| panic!("resource does not exist: {}", std::any::type_name::())); + // If the resource isn't send and sync, validate that we are on the main thread, so that we can access it. + let _component_info = self.components().get_info(component_id).unwrap(); + let (ptr, mut ticks) = self + .storages + .non_send_resources + .get_mut(component_id) + // SAFETY: The type R is Send and Sync or we've already validated that we're on the main thread. + .and_then(|info| unsafe { info.remove() }) + .unwrap_or_else(|| panic!("resource does not exist: {}", std::any::type_name::())); + // Read the value onto the stack to avoid potential mut aliasing. + // SAFETY: pointer is of type R + let mut value = unsafe { ptr.read::() }; + let value_mut = Mut { + value: &mut value, + ticks: Ticks { + component_ticks: &mut ticks, + last_change_tick, + change_tick, + }, + }; + let result = f(self, value_mut); + assert!(!self.contains_non_send::(), + "Resource `{}` was inserted during a call to World::resource_scope.\n\ + This is not allowed as the original resource is reinserted to the world after the FnOnce param is invoked.", + std::any::type_name::()); + + OwningPtr::make(value, |ptr| { + // SAFETY: pointer is of type R + unsafe { + self.storages + .non_send_resources + .get_mut(component_id) + .map(|info| info.insert_with_ticks(ptr, ticks)) + .unwrap_or_else(|| { + panic!( + "No resource of type {} exists in the World.", + std::any::type_name::() + ) + }); + } + }); + + result + } + /// Sends an [`Event`](crate::event::Event). #[inline] pub fn send_event(&mut self, event: E) { @@ -1273,7 +1376,13 @@ impl World { &self, component_id: ComponentId, ) -> Option<&R> { - self.get_resource_with_id(component_id) + Some( + self.storages + .non_send_resources + .get(component_id)? + .get_data()? + .deref::(), + ) } /// # Safety @@ -1284,7 +1393,19 @@ impl World { &self, component_id: ComponentId, ) -> Option> { - self.get_resource_unchecked_mut_with_id(component_id) + let (ptr, ticks) = self + .storages + .non_send_resources + .get(component_id)? + .get_with_ticks()?; + Some(Mut { + value: ptr.assert_unique().deref_mut(), + ticks: Ticks { + component_ticks: ticks.deref_mut(), + last_change_tick: self.last_change_tick(), + change_tick: self.read_change_tick(), + }, + }) } /// Inserts a new resource with the given `value`. Will replace the value if it already existed. @@ -1306,24 +1427,45 @@ impl World { let change_tick = self.change_tick(); // SAFETY: component_id and is_send are valid, ensured by caller - self.initialize_resource_internal(component_id, is_send) - .insert(value, change_tick); + if is_send { + self.initialize_resource_internal(component_id) + .insert(value, change_tick); + } else { + self.initialize_non_send_internal(component_id) + .insert(value, change_tick); + } } /// # Safety /// `component_id` must be valid for this world - /// `is_send` should correspond for the resource being initialized. #[inline] unsafe fn initialize_resource_internal( &mut self, component_id: ComponentId, - is_send: bool, ) -> &mut ResourceData { let archetype_component_count = &mut self.archetypes.archetype_component_count; - // SAFETY: Caller guarentees that `is_send` matches the underlying type. self.storages .resources - .initialize_with(component_id, &self.components, is_send, || { + .initialize_with(component_id, &self.components, || { + let id = ArchetypeComponentId::new(*archetype_component_count); + *archetype_component_count += 1; + id + }) + } + + /// # Safety + /// `component_id` must be valid for this world + /// `is_send` should correspond for the resource being initialized. + #[inline] + unsafe fn initialize_non_send_internal( + &mut self, + component_id: ComponentId, + ) -> &mut NonSendResourceData { + let archetype_component_count = &mut self.archetypes.archetype_component_count; + // SAFETY: Caller guarentees that `is_send` matches the underlying type. + self.storages + .non_send_resources + .initialize_with(component_id, &self.components, || { let id = ArchetypeComponentId::new(*archetype_component_count); *archetype_component_count += 1; id @@ -1333,14 +1475,14 @@ impl World { pub(crate) fn initialize_resource(&mut self) -> ComponentId { let component_id = self.components.init_resource::(); // SAFETY: resource initialized above, resource type must be Send - unsafe { self.initialize_resource_internal(component_id, true) }; + unsafe { self.initialize_resource_internal(component_id) }; component_id } pub(crate) fn initialize_non_send_resource(&mut self) -> ComponentId { let component_id = self.components.init_non_send::(); // SAFETY: resource initialized above, resource type might not be send - unsafe { self.initialize_resource_internal(component_id, false) }; + unsafe { self.initialize_non_send_internal(component_id) }; component_id } @@ -1388,6 +1530,9 @@ impl World { self.storages.tables.check_change_ticks(change_tick); self.storages.sparse_sets.check_change_ticks(change_tick); self.storages.resources.check_change_ticks(change_tick); + self.storages + .non_send_resources + .check_change_ticks(change_tick); } pub fn clear_entities(&mut self) { @@ -1452,6 +1597,22 @@ impl World { Some(()) } + /// Removes the resource of a given type, if it exists. Otherwise returns [None]. + /// + /// **You should prefer to use the typed API [`World::remove_resource`] where possible and only + /// use this in cases where the actual types are not known at compile time.** + pub fn remove_non_send_by_id(&mut self, component_id: ComponentId) -> Option<()> { + let _info = self.components.get_info(component_id)?; + // SAFETY: The underlying type is Send and Sync or we've already validated we're on the main thread + unsafe { + self.storages + .non_send_resources + .get_mut(component_id)? + .remove_and_drop(); + } + Some(()) + } + /// Retrieves an immutable untyped reference to the given `entity`'s [Component] of the given [`ComponentId`]. /// Returns [None] if the `entity` does not have a [Component] of the given type. /// From 1a09486153bc2d85453de855d6273f104c8e2877 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 1 Dec 2022 20:11:31 -0800 Subject: [PATCH 12/48] Update safety docs. --- crates/bevy_ecs/src/storage/resource.rs | 40 ++++--------------------- 1 file changed, 5 insertions(+), 35 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 801c1f5678abb..25f4ab7ee02df 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -26,10 +26,6 @@ impl ResourceData { } /// Gets a read-only pointer to the underlying resource, if available. - /// - /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. #[inline] pub fn get_data(&self) -> Option> { self.column.get_data(0) @@ -41,26 +37,16 @@ impl ResourceData { self.column.get_ticks(0) } - /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. #[inline] pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, TickCells<'_>)> { - self.column.get(0).map(|res| { - self.validate_access(); - res - }) + self.column.get(0) } /// Inserts a value into the resource. If a value is already present /// it will be replaced. /// - /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. - /// /// # Safety - /// `value` must be valid for the underlying type for the resource. + /// `value` must be valid for the underlying type for the resource. The underlying type must be [`Send`]. #[inline] pub(crate) unsafe fn insert(&mut self, value: OwningPtr<'_>, change_tick: u32) { if self.is_present() { @@ -73,12 +59,8 @@ impl ResourceData { /// Inserts a value into the resource with a pre-existing change tick. If a /// value is already present it will be replaced. /// - /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. - /// /// # Safety - /// `value` must be valid for the underlying type for the resource. + /// `value` must be valid for the underlying type for the resource. The underlying type must be [`Send`]. #[inline] pub(crate) unsafe fn insert_with_ticks( &mut self, @@ -96,15 +78,8 @@ impl ResourceData { /// Removes a value from the resource, if present. /// - /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. - /// /// # Safety - /// The underlying type must be [`Send`] or be removed from the thread it was - /// inserted from. - /// - /// The removed value must be used or dropped. + /// The underlying type must be [`Send`]. The removed value must be used or dropped. #[inline] #[must_use = "The returned pointer to the removed component should be used or dropped"] pub(crate) unsafe fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { @@ -113,13 +88,8 @@ impl ResourceData { /// Removes a value from the resource, if present, and drops it. /// - /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. - /// /// # Safety - /// The underlying type must be [`Send`] or be removed from the thread it was - /// inserted from. + /// The underlying type must be [`Send`] inserted from. #[inline] pub(crate) unsafe fn remove_and_drop(&mut self) { if self.is_present() { From f935aa29190d9e1da5d0d084fa77a8e39a185f1c Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 1 Dec 2022 20:29:55 -0800 Subject: [PATCH 13/48] Actually fix CI --- .../bevy_ecs/src/storage/non_send_resource.rs | 18 ++++++------------ crates/bevy_ecs/src/storage/resource.rs | 3 +-- crates/bevy_ecs/src/world/mod.rs | 9 ++++++--- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/crates/bevy_ecs/src/storage/non_send_resource.rs b/crates/bevy_ecs/src/storage/non_send_resource.rs index b1f53406bf086..69d608d7cc73e 100644 --- a/crates/bevy_ecs/src/storage/non_send_resource.rs +++ b/crates/bevy_ecs/src/storage/non_send_resource.rs @@ -1,8 +1,7 @@ use crate::archetype::ArchetypeComponentId; -use crate::component::{ComponentId, ComponentTicks, Components}; +use crate::component::{ComponentId, ComponentTicks, Components, TickCells}; use crate::storage::{Column, SparseSet}; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; -use std::cell::UnsafeCell; use std::mem::ManuallyDrop; use std::thread::ThreadId; @@ -75,21 +74,15 @@ impl NonSendResourceData { /// Gets a read-only reference to the change ticks of the underlying resource, if available. #[inline] - pub fn get_ticks(&self) -> Option<&ComponentTicks> { - self.column - .get_ticks(0) - // SAFETY: - // - This borrow's lifetime is bounded by the lifetime on self. - // - A read-only borrow on self can only exist while a mutable borrow doesn't - // exist. - .map(|ticks| unsafe { ticks.deref() }) + pub fn get_ticks(&self) -> Option { + self.column.get_ticks(0) } /// # Panics /// This will panic if a value is present and is not accessed from the original thread /// it was inserted in. #[inline] - pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, &UnsafeCell)> { + pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, TickCells<'_>)> { self.column.get(0).map(|res| { self.validate_access(); res @@ -134,7 +127,8 @@ impl NonSendResourceData { if self.is_present() { self.validate_access(); self.column.replace_untracked(0, value); - *self.column.get_ticks_unchecked(0).deref_mut() = change_ticks; + *self.column.get_added_ticks_unchecked(0).deref_mut() = change_ticks.added; + *self.column.get_changed_ticks_unchecked(0).deref_mut() = change_ticks.changed; } else { self.origin_thread_id = std::thread::current().id(); self.column.push(value, change_ticks); diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 25f4ab7ee02df..dacc31e86df51 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -2,7 +2,6 @@ use crate::archetype::ArchetypeComponentId; use crate::component::{ComponentId, ComponentTicks, Components, TickCells}; use crate::storage::{Column, SparseSet}; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; -use std::cell::UnsafeCell; /// The type-erased backing storage and metadata for a single resource within a [`World`]. /// @@ -89,7 +88,7 @@ impl ResourceData { /// Removes a value from the resource, if present, and drops it. /// /// # Safety - /// The underlying type must be [`Send`] inserted from. + /// The underlying type must be [`Send`]. #[inline] pub(crate) unsafe fn remove_and_drop(&mut self) { if self.is_present() { diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 27f8ec4e4c0c0..84459c0603033 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -15,6 +15,7 @@ use crate::{ Component, ComponentDescriptor, ComponentId, ComponentInfo, Components, TickCells, }, entity::{AllocAtWithoutReplacement, Entities, Entity}, + ptr::UnsafeCellDeref, query::{QueryState, ReadOnlyWorldQuery, WorldQuery}, storage::{NonSendResourceData, ResourceData, SparseSet, Storages}, system::Resource, @@ -1013,7 +1014,7 @@ impl World { pub(crate) fn get_non_send_with_ticks( &self, component_id: ComponentId, - ) -> Option<(Ptr<'_>, &UnsafeCell)> { + ) -> Option<(Ptr<'_>, TickCells<'_>)> { self.storages .non_send_resources .get(component_id)? @@ -1286,7 +1287,8 @@ impl World { let value_mut = Mut { value: &mut value, ticks: Ticks { - component_ticks: &mut ticks, + added: &mut ticks.added, + changed: &mut ticks.changed, last_change_tick, change_tick, }, @@ -1401,7 +1403,8 @@ impl World { Some(Mut { value: ptr.assert_unique().deref_mut(), ticks: Ticks { - component_ticks: ticks.deref_mut(), + added: ticks.added.deref_mut(), + changed: ticks.changed.deref_mut(), last_change_tick: self.last_change_tick(), change_tick: self.read_change_tick(), }, From f4281e60acd46d0b741df66ce241e9f5a2d889c5 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 1 Dec 2022 20:43:23 -0800 Subject: [PATCH 14/48] Small cleanup --- .../bevy_ecs/src/storage/non_send_resource.rs | 25 ++++++++----------- crates/bevy_ecs/src/storage/resource.rs | 2 +- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/crates/bevy_ecs/src/storage/non_send_resource.rs b/crates/bevy_ecs/src/storage/non_send_resource.rs index 69d608d7cc73e..f7503420e014e 100644 --- a/crates/bevy_ecs/src/storage/non_send_resource.rs +++ b/crates/bevy_ecs/src/storage/non_send_resource.rs @@ -62,8 +62,8 @@ impl NonSendResourceData { /// Gets a read-only pointer to the underlying `!Send` resource, if available. /// /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. + /// This will panic if a value is present and is not accessed from the original thread + /// it was inserted from. #[inline] pub fn get_data(&self) -> Option> { self.column.get_data(0).map(|res| { @@ -72,7 +72,7 @@ impl NonSendResourceData { }) } - /// Gets a read-only reference to the change ticks of the underlying resource, if available. + /// Gets the change ticks of the underlying resource, if available. #[inline] pub fn get_ticks(&self) -> Option { self.column.get_ticks(0) @@ -113,8 +113,8 @@ impl NonSendResourceData { /// If a value is already present it will be replaced. /// /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. + /// This will panic if a value is present and is not accessed from the original thread + /// it was inserted from. /// /// # Safety /// `value` must be valid for the underlying type for the resource. @@ -142,8 +142,7 @@ impl NonSendResourceData { /// `!Send`, and is not accessed from the original thread it was inserted in. /// /// # Safety - /// The underlying type must be [`Send`] or be removed from the thread it was - /// inserted from. + /// The underlying type must be removed from the thread it was inserted from. /// /// The removed value must be used or dropped. #[inline] @@ -157,12 +156,11 @@ impl NonSendResourceData { /// Removes a value from the resource, if present, and drops it. /// /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. + /// This will panic if a value is present and is not accessed from the original thread + /// it was inserted from. /// /// # Safety - /// The underlying type must be [`Send`] or be removed from the thread it was - /// inserted from. + /// The underlying type must be removed from the thread it was inserted from. #[inline] pub(crate) unsafe fn remove_and_drop(&mut self) { if self.is_present() { @@ -218,10 +216,7 @@ impl NonSendResources { /// /// # Panics /// Will panic if `component_id` is not valid for the provided `components` - /// - /// # Safety - /// `is_send` must be accurate for the Resource that is being initialized. - pub(crate) unsafe fn initialize_with( + pub(crate) fn initialize_with( &mut self, component_id: ComponentId, components: &Components, diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index dacc31e86df51..4eb78b15f024f 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -145,7 +145,7 @@ impl Resources { /// /// # Panics /// Will panic if `component_id` is not valid for the provided `components` - pub(crate) unsafe fn initialize_with( + pub(crate) fn initialize_with( &mut self, component_id: ComponentId, components: &Components, From 1692da8465b9f87177624abcb2ec974e00cfba80 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 1 Dec 2022 20:44:55 -0800 Subject: [PATCH 15/48] Fix WorldCell's use of NonSend --- crates/bevy_ecs/src/world/world_cell.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/world/world_cell.rs b/crates/bevy_ecs/src/world/world_cell.rs index 5f0fb5aaa5a74..8e6699e076694 100644 --- a/crates/bevy_ecs/src/world/world_cell.rs +++ b/crates/bevy_ecs/src/world/world_cell.rs @@ -256,7 +256,7 @@ impl<'w> WorldCell<'w> { let component_id = self.world.components.get_resource_id(TypeId::of::())?; let archetype_component_id = self .world - .get_resource_archetype_component_id(component_id)?; + .get_non_send_archetype_component_id(component_id)?; WorldBorrow::try_new( // SAFETY: ComponentId matches TypeId || unsafe { self.world.get_non_send_with_id(component_id) }, @@ -289,7 +289,7 @@ impl<'w> WorldCell<'w> { let component_id = self.world.components.get_resource_id(TypeId::of::())?; let archetype_component_id = self .world - .get_resource_archetype_component_id(component_id)?; + .get_non_send_archetype_component_id(component_id)?; WorldBorrowMut::try_new( // SAFETY: ComponentId matches TypeId and access is checked by WorldBorrowMut || unsafe { self.world.get_non_send_unchecked_mut_with_id(component_id) }, From 2b11ea357ffa4edf77ff75f76b15330d949384c6 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 1 Dec 2022 23:30:55 -0800 Subject: [PATCH 16/48] Fix bevy_window --- crates/bevy_render/src/view/window.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/bevy_render/src/view/window.rs b/crates/bevy_render/src/view/window.rs index 8e6f82c66c911..96cff2cdff2e5 100644 --- a/crates/bevy_render/src/view/window.rs +++ b/crates/bevy_render/src/view/window.rs @@ -29,7 +29,6 @@ impl Plugin for WindowRenderPlugin { render_app .init_resource::() .init_resource::() - .init_resource::() .add_system_to_stage(RenderStage::Extract, extract_windows) .add_system_to_stage( RenderStage::Prepare, @@ -166,7 +165,7 @@ pub struct WindowSurfaces { pub fn prepare_windows( // By accessing a NonSend resource, we tell the scheduler to put this system on the main thread, // which is necessary for some OS s - _marker: NonSend, + _marker: Option>, mut windows: ResMut, mut window_surfaces: ResMut, render_device: Res, From 638e95c93ec2e2517a6b349d7dc5c77ed1ec8e9a Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 2 Dec 2022 14:24:58 -0800 Subject: [PATCH 17/48] Use const generics to split on Send/!Send --- crates/bevy_ecs/src/storage/mod.rs | 4 ++-- crates/bevy_ecs/src/storage/resource.rs | 18 +++++++++--------- crates/bevy_ecs/src/world/mod.rs | 4 ++-- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/storage/mod.rs b/crates/bevy_ecs/src/storage/mod.rs index 215ecd0bbdf53..c9a8633829a38 100644 --- a/crates/bevy_ecs/src/storage/mod.rs +++ b/crates/bevy_ecs/src/storage/mod.rs @@ -16,6 +16,6 @@ pub use table::*; pub struct Storages { pub sparse_sets: SparseSets, pub tables: Tables, - pub resources: Resources, - pub non_send_resources: NonSendResources, + pub resources: Resources, + pub non_send_resources: Resources, } diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 4eb78b15f024f..cf259e58ed60b 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -6,12 +6,12 @@ use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; /// The type-erased backing storage and metadata for a single resource within a [`World`]. /// /// [`World`]: crate::world::World -pub struct ResourceData { +pub struct ResourceData { column: Column, id: ArchetypeComponentId, } -impl ResourceData { +impl ResourceData { /// Returns true if the resource is populated. #[inline] pub fn is_present(&self) -> bool { @@ -102,11 +102,11 @@ impl ResourceData { /// [`Resource`]: crate::system::Resource /// [`World`]: crate::world::World #[derive(Default)] -pub struct Resources { - resources: SparseSet, +pub struct Resources{ + resources: SparseSet>, } -impl Resources { +impl Resources { /// The total number of resources stored in the [`World`] /// /// [`World`]: crate::world::World @@ -116,7 +116,7 @@ impl Resources { } /// Iterate over all resources that have been initialized, i.e. given a [`ComponentId`] - pub fn iter(&self) -> impl Iterator { + pub fn iter(&self) -> impl Iterator)> { self.resources.iter().map(|(id, data)| (*id, data)) } @@ -131,13 +131,13 @@ impl Resources { /// Gets read-only access to a resource, if it exists. #[inline] - pub fn get(&self, component_id: ComponentId) -> Option<&ResourceData> { + pub fn get(&self, component_id: ComponentId) -> Option<&ResourceData> { self.resources.get(component_id) } /// Gets mutable access to a resource, if it exists. #[inline] - pub(crate) fn get_mut(&mut self, component_id: ComponentId) -> Option<&mut ResourceData> { + pub(crate) fn get_mut(&mut self, component_id: ComponentId) -> Option<&mut ResourceData> { self.resources.get_mut(component_id) } @@ -150,7 +150,7 @@ impl Resources { component_id: ComponentId, components: &Components, f: impl FnOnce() -> ArchetypeComponentId, - ) -> &mut ResourceData { + ) -> &mut ResourceData { self.resources.get_or_insert_with(component_id, || { let component_info = components.get_info(component_id).unwrap(); ResourceData { diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 84459c0603033..ca0aa86145172 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1445,7 +1445,7 @@ impl World { unsafe fn initialize_resource_internal( &mut self, component_id: ComponentId, - ) -> &mut ResourceData { + ) -> &mut ResourceData { let archetype_component_count = &mut self.archetypes.archetype_component_count; self.storages .resources @@ -1463,7 +1463,7 @@ impl World { unsafe fn initialize_non_send_internal( &mut self, component_id: ComponentId, - ) -> &mut NonSendResourceData { + ) -> &mut ResourceData { let archetype_component_count = &mut self.archetypes.archetype_component_count; // SAFETY: Caller guarentees that `is_send` matches the underlying type. self.storages From c37f26e263a85c58d4e983c8333b672906224896 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 2 Dec 2022 14:42:15 -0800 Subject: [PATCH 18/48] Use const generics instead of duplicating code --- crates/bevy_ecs/src/storage/mod.rs | 2 - .../bevy_ecs/src/storage/non_send_resource.rs | 241 ------------------ crates/bevy_ecs/src/storage/resource.rs | 105 +++++++- crates/bevy_ecs/src/world/mod.rs | 2 +- 4 files changed, 96 insertions(+), 254 deletions(-) delete mode 100644 crates/bevy_ecs/src/storage/non_send_resource.rs diff --git a/crates/bevy_ecs/src/storage/mod.rs b/crates/bevy_ecs/src/storage/mod.rs index c9a8633829a38..b2fab3fdcb590 100644 --- a/crates/bevy_ecs/src/storage/mod.rs +++ b/crates/bevy_ecs/src/storage/mod.rs @@ -1,12 +1,10 @@ //! Storage layouts for ECS data. mod blob_vec; -mod non_send_resource; mod resource; mod sparse_set; mod table; -pub use non_send_resource::*; pub use resource::*; pub use sparse_set::*; pub use table::*; diff --git a/crates/bevy_ecs/src/storage/non_send_resource.rs b/crates/bevy_ecs/src/storage/non_send_resource.rs deleted file mode 100644 index f7503420e014e..0000000000000 --- a/crates/bevy_ecs/src/storage/non_send_resource.rs +++ /dev/null @@ -1,241 +0,0 @@ -use crate::archetype::ArchetypeComponentId; -use crate::component::{ComponentId, ComponentTicks, Components, TickCells}; -use crate::storage::{Column, SparseSet}; -use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; -use std::mem::ManuallyDrop; -use std::thread::ThreadId; - -/// The type-erased backing storage and metadata for a single resource within a [`World`]. -/// -/// [`World`]: crate::world::World -pub struct NonSendResourceData { - column: ManuallyDrop, - type_name: String, - id: ArchetypeComponentId, - origin_thread_id: ThreadId, -} - -impl Drop for NonSendResourceData { - fn drop(&mut self) { - if self.is_present() { - self.validate_access(); - } - // SAFETY: Drop is only called once upon dropping the ResourceData - // and is inaccessible after this as the parent ResourceData has - // been dropped. - unsafe { - ManuallyDrop::drop(&mut self.column); - } - } -} - -impl NonSendResourceData { - #[inline] - fn validate_access(&self) { - // Avoid aborting due to double panicking on the same thread. - if std::thread::panicking() { - return; - } - if self.origin_thread_id != std::thread::current().id() { - // Panic in tests, as testing for aborting is nearly impossible - panic!( - "Attempted to access or drop non-send resource {} from thread {:?} on a thread {:?}. This is not allowed. Aborting.", - self.type_name, - self.origin_thread_id, - std::thread::current().id() - ); - } - } - - /// Returns true if the `!Send` resource is populated. - #[inline] - pub fn is_present(&self) -> bool { - !self.column.is_empty() - } - - /// Gets the [`ArchetypeComponentId`] for the `!Send` resource. - #[inline] - pub fn id(&self) -> ArchetypeComponentId { - self.id - } - - /// Gets a read-only pointer to the underlying `!Send` resource, if available. - /// - /// # Panics - /// This will panic if a value is present and is not accessed from the original thread - /// it was inserted from. - #[inline] - pub fn get_data(&self) -> Option> { - self.column.get_data(0).map(|res| { - self.validate_access(); - res - }) - } - - /// Gets the change ticks of the underlying resource, if available. - #[inline] - pub fn get_ticks(&self) -> Option { - self.column.get_ticks(0) - } - - /// # Panics - /// This will panic if a value is present and is not accessed from the original thread - /// it was inserted in. - #[inline] - pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, TickCells<'_>)> { - self.column.get(0).map(|res| { - self.validate_access(); - res - }) - } - - /// Inserts a value into the `!Send` resource. If a value is already present - /// it will be replaced. - /// - /// # Panics - /// This will panic if a value is present and is not accessed from the original thread - /// it was inserted in. - /// - /// # Safety - /// `value` must be valid for the underlying type for the resource. - #[inline] - pub(crate) unsafe fn insert(&mut self, value: OwningPtr<'_>, change_tick: u32) { - if self.is_present() { - self.validate_access(); - self.column.replace(0, value, change_tick); - } else { - self.origin_thread_id = std::thread::current().id(); - self.column.push(value, ComponentTicks::new(change_tick)); - } - } - - /// Inserts a value into the `!Send` resource with a pre-existing change tick. - /// If a value is already present it will be replaced. - /// - /// # Panics - /// This will panic if a value is present and is not accessed from the original thread - /// it was inserted from. - /// - /// # Safety - /// `value` must be valid for the underlying type for the resource. - #[inline] - pub(crate) unsafe fn insert_with_ticks( - &mut self, - value: OwningPtr<'_>, - change_ticks: ComponentTicks, - ) { - if self.is_present() { - self.validate_access(); - self.column.replace_untracked(0, value); - *self.column.get_added_ticks_unchecked(0).deref_mut() = change_ticks.added; - *self.column.get_changed_ticks_unchecked(0).deref_mut() = change_ticks.changed; - } else { - self.origin_thread_id = std::thread::current().id(); - self.column.push(value, change_ticks); - } - } - - /// Removes a value from the `!Send` resource, if present. - /// - /// # Panics - /// This will panic if a value is present, the underlying type is - /// `!Send`, and is not accessed from the original thread it was inserted in. - /// - /// # Safety - /// The underlying type must be removed from the thread it was inserted from. - /// - /// The removed value must be used or dropped. - #[inline] - #[must_use = "The returned pointer to the removed component should be used or dropped"] - pub(crate) unsafe fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { - self.is_present() - .then(|| self.validate_access()) - .and_then(|_| self.column.swap_remove_and_forget(0)) - } - - /// Removes a value from the resource, if present, and drops it. - /// - /// # Panics - /// This will panic if a value is present and is not accessed from the original thread - /// it was inserted from. - /// - /// # Safety - /// The underlying type must be removed from the thread it was inserted from. - #[inline] - pub(crate) unsafe fn remove_and_drop(&mut self) { - if self.is_present() { - self.validate_access(); - self.column.clear(); - } - } -} - -/// The backing store for all `!Send` [`Resource`]s stored in the [`World`]. -/// -/// [`Resource`]: crate::system::Resource -/// [`World`]: crate::world::World -#[derive(Default)] -pub struct NonSendResources { - resources: SparseSet, -} - -impl NonSendResources { - /// The total number of `!Send` resources stored in the [`World`] - /// - /// [`World`]: crate::world::World - #[inline] - pub fn len(&self) -> usize { - self.resources.len() - } - - /// Returns true if there are no `!Send` resources stored in the [`World`], - /// false otherwise. - /// - /// [`World`]: crate::world::World - #[inline] - pub fn is_empty(&self) -> bool { - self.resources.is_empty() - } - - /// Gets read-only access to a `!Send` resource, if it exists. - #[inline] - pub fn get(&self, component_id: ComponentId) -> Option<&NonSendResourceData> { - self.resources.get(component_id) - } - - /// Gets mutable access to a `!Send` resource, if it exists. - #[inline] - pub(crate) fn get_mut( - &mut self, - component_id: ComponentId, - ) -> Option<&mut NonSendResourceData> { - self.resources.get_mut(component_id) - } - - /// Fetches or initializes a new `!Send` resource and returns back it's underlying column. - /// - /// # Panics - /// Will panic if `component_id` is not valid for the provided `components` - pub(crate) fn initialize_with( - &mut self, - component_id: ComponentId, - components: &Components, - f: impl FnOnce() -> ArchetypeComponentId, - ) -> &mut NonSendResourceData { - self.resources.get_or_insert_with(component_id, || { - let component_info = components.get_info(component_id).unwrap(); - NonSendResourceData { - column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), - type_name: String::from(component_info.name()), - id: f(), - origin_thread_id: std::thread::current().id(), - } - }) - } - - pub(crate) fn check_change_ticks(&mut self, change_tick: u32) { - for info in self.resources.values_mut() { - info.column.check_change_ticks(change_tick); - } - } -} diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index cf259e58ed60b..6cb62d1585091 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -2,16 +2,52 @@ use crate::archetype::ArchetypeComponentId; use crate::component::{ComponentId, ComponentTicks, Components, TickCells}; use crate::storage::{Column, SparseSet}; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; +use std::{mem::ManuallyDrop, thread::ThreadId}; /// The type-erased backing storage and metadata for a single resource within a [`World`]. /// +/// If `SEND` is false, values of this type will panic if dropped from a different thread. +/// /// [`World`]: crate::world::World pub struct ResourceData { - column: Column, + column: ManuallyDrop, + type_name: String, id: ArchetypeComponentId, + origin_thread_id: Option, +} + +impl Drop for ResourceData { + fn drop(&mut self) { + if self.is_present() { + self.validate_access(); + } + // SAFETY: Drop is only called once upon dropping the ResourceData + // and is inaccessible after this as the parent ResourceData has + // been dropped. + unsafe { + ManuallyDrop::drop(&mut self.column); + } + } } impl ResourceData { + #[inline] + fn validate_access(&self) { + // Avoid aborting due to double panicking on the same thread. + if SEND || std::thread::panicking() { + return; + } + if self.origin_thread_id != Some(std::thread::current().id()) { + // Panic in tests, as testing for aborting is nearly impossible + panic!( + "Attempted to access or drop non-send resource {} from thread {:?} on a thread {:?}. This is not allowed. Aborting.", + self.type_name, + self.origin_thread_id, + std::thread::current().id() + ); + } + } + /// Returns true if the resource is populated. #[inline] pub fn is_present(&self) -> bool { @@ -25,9 +61,16 @@ impl ResourceData { } /// Gets a read-only pointer to the underlying resource, if available. + /// + /// # Panics + /// If `SEND` is false, this will panic if a value is present and is not accessed from the + /// original thread it was inserted from. #[inline] pub fn get_data(&self) -> Option> { - self.column.get_data(0) + self.column.get_data(0).map(|res| { + self.validate_access(); + res + }) } /// Gets a read-only reference to the change ticks of the underlying resource, if available. @@ -36,21 +79,36 @@ impl ResourceData { self.column.get_ticks(0) } + /// # Panics + /// If `SEND` is false, this will panic if a value is present and is not accessed from the + /// original thread it was inserted in. #[inline] pub(crate) fn get_with_ticks(&self) -> Option<(Ptr<'_>, TickCells<'_>)> { - self.column.get(0) + self.column.get(0).map(|res| { + self.validate_access(); + res + }) } /// Inserts a value into the resource. If a value is already present /// it will be replaced. /// + /// # Panics + /// If `SEND` is false, this will panic if a value is present and is not replaced from + /// the original thread it was inserted in. + /// /// # Safety - /// `value` must be valid for the underlying type for the resource. The underlying type must be [`Send`]. + /// - `value` must be valid for the underlying type for the resource. + /// - The underlying type must be [`Send`] if `SEND` is true. #[inline] pub(crate) unsafe fn insert(&mut self, value: OwningPtr<'_>, change_tick: u32) { if self.is_present() { + self.validate_access(); self.column.replace(0, value, change_tick); } else { + if !SEND { + self.origin_thread_id = Some(std::thread::current().id()); + } self.column.push(value, ComponentTicks::new(change_tick)); } } @@ -58,8 +116,13 @@ impl ResourceData { /// Inserts a value into the resource with a pre-existing change tick. If a /// value is already present it will be replaced. /// + /// # Panics + /// If `SEND` is false, this will panic if a value is present and is not replaced from + /// the original thread it was inserted in. + /// /// # Safety - /// `value` must be valid for the underlying type for the resource. The underlying type must be [`Send`]. + /// - `value` must be valid for the underlying type for the resource. + /// - The underlying type must be [`Send`] if `SEND` is true. #[inline] pub(crate) unsafe fn insert_with_ticks( &mut self, @@ -67,31 +130,51 @@ impl ResourceData { change_ticks: ComponentTicks, ) { if self.is_present() { + self.validate_access(); self.column.replace_untracked(0, value); *self.column.get_added_ticks_unchecked(0).deref_mut() = change_ticks.added; *self.column.get_changed_ticks_unchecked(0).deref_mut() = change_ticks.changed; } else { + if !SEND { + self.origin_thread_id = Some(std::thread::current().id()); + } self.column.push(value, change_ticks); } } /// Removes a value from the resource, if present. /// + /// # Panics + /// If `SEND` is false, this will panic if a value is present and is not removed from the + /// original thread it was inserted from. + /// /// # Safety - /// The underlying type must be [`Send`]. The removed value must be used or dropped. + /// - the removed value must be used or dropped. + /// - the underlying type must be [`Send`] if `SEND` is true. #[inline] #[must_use = "The returned pointer to the removed component should be used or dropped"] pub(crate) unsafe fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { - self.column.swap_remove_and_forget(0) + if SEND { + self.column.swap_remove_and_forget(0) + } else { + self.is_present() + .then(|| self.validate_access()) + .and_then(|_| self.column.swap_remove_and_forget(0)) + } } /// Removes a value from the resource, if present, and drops it. /// + /// # Panics + /// If `SEND` is false, this will panic if a value is present and is not + /// accessed from the original thread it was inserted in. + /// /// # Safety - /// The underlying type must be [`Send`]. + /// - the underlying type must be [`Send`] if `SEND` is true. #[inline] pub(crate) unsafe fn remove_and_drop(&mut self) { if self.is_present() { + self.validate_access(); self.column.clear(); } } @@ -102,7 +185,7 @@ impl ResourceData { /// [`Resource`]: crate::system::Resource /// [`World`]: crate::world::World #[derive(Default)] -pub struct Resources{ +pub struct Resources { resources: SparseSet>, } @@ -154,8 +237,10 @@ impl Resources { self.resources.get_or_insert_with(component_id, || { let component_info = components.get_info(component_id).unwrap(); ResourceData { - column: Column::with_capacity(component_info, 1), + column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), + type_name: String::from(component_info.name()), id: f(), + origin_thread_id: SEND.then(|| std::thread::current().id()), } }) } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index ca0aa86145172..19f4886ecaa46 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -17,7 +17,7 @@ use crate::{ entity::{AllocAtWithoutReplacement, Entities, Entity}, ptr::UnsafeCellDeref, query::{QueryState, ReadOnlyWorldQuery, WorldQuery}, - storage::{NonSendResourceData, ResourceData, SparseSet, Storages}, + storage::{ResourceData, SparseSet, Storages}, system::Resource, }; use bevy_ptr::{OwningPtr, Ptr}; From ee5de65f58ef6e29ea25f1e8ca360cc1eec1dbe9 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 2 Dec 2022 15:47:36 -0800 Subject: [PATCH 19/48] Use the right branch --- crates/bevy_ecs/src/lib.rs | 7 +++++++ crates/bevy_ecs/src/storage/resource.rs | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 1f7e9a04dedb3..1eb8a710ae180 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1372,6 +1372,13 @@ mod tests { } } + #[test] + fn non_send_resource_drop_from_same_thread() { + let mut world = World::default(); + world.insert_non_send_resource(NonSendA::default()); + drop(world); + } + #[test] fn insert_overwrite_drop() { let (dropck1, dropped1) = DropCk::new_pair(); diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 6cb62d1585091..c8d6d401cb629 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -240,7 +240,7 @@ impl Resources { column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), type_name: String::from(component_info.name()), id: f(), - origin_thread_id: SEND.then(|| std::thread::current().id()), + origin_thread_id: (!SEND).then(|| std::thread::current().id()), } }) } From d195cb6cf046f0c487e597134753a6753cc1b95f Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 2 Dec 2022 20:52:41 -0800 Subject: [PATCH 20/48] Add panic sections to World's API docs --- crates/bevy_ecs/src/world/mod.rs | 40 +++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 19f4886ecaa46..6828a7756400c 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -765,8 +765,8 @@ impl World { /// Systems with `NonSend` resources are always scheduled on the main thread. /// /// # Panics - /// - /// Panics if called from a thread other than the main thread. + /// If a value is already present, this function will panic if called + /// from a thread that the original value was inserted from. #[inline] pub fn insert_non_send_resource(&mut self, value: R) { let component_id = self.components.init_non_send::(); @@ -789,9 +789,17 @@ impl World { } } - /// # Panic - /// Will panic if removing `NonSend` resources from threads other than where they were - /// inserted from as they cannot be sent across threads + /// Removes a `!Send` resource from the world and returns it, if present. + /// + /// `NonSend` resources cannot be sent across threads, + /// and do not need the `Send + Sync` bounds. + /// Systems with `NonSend` resources are always scheduled on the main thread. + /// + /// Returns `None` if a value was not previously present. + /// + /// # Panics + /// If a value is present, this function will panic if called from a thread that the + /// value was inserted from. #[inline] pub fn remove_non_send_resource(&mut self) -> Option { let component_id = self.components.get_resource_id(TypeId::of::())?; @@ -937,6 +945,8 @@ impl World { /// /// Panics if the resource does not exist. /// Use [`get_non_send_resource`](World::get_non_send_resource) instead if you want to handle this case. + /// + /// This function will panic if called from a thread that the value was inserted from. #[inline] #[track_caller] pub fn non_send_resource(&self) -> &R { @@ -957,6 +967,8 @@ impl World { /// /// Panics if the resource does not exist. /// Use [`get_non_send_resource_mut`](World::get_non_send_resource_mut) instead if you want to handle this case. + /// + /// This function will panic if called from a thread that the value was inserted from. #[inline] #[track_caller] pub fn non_send_resource_mut(&mut self) -> Mut<'_, R> { @@ -972,7 +984,10 @@ impl World { } /// Gets a reference to the non-send resource of the given type, if it exists. - /// Otherwise returns [None] + /// Otherwise returns [None]. + /// + /// # Panics + /// This function will panic if called from a thread that the value was inserted from. #[inline] pub fn get_non_send_resource(&self) -> Option<&R> { let component_id = self.components.get_resource_id(TypeId::of::())?; @@ -982,6 +997,9 @@ impl World { /// Gets a mutable reference to the non-send resource of the given type, if it exists. /// Otherwise returns [None] + /// + /// # Panics + /// This function will panic if called from a thread that the value was inserted from. #[inline] pub fn get_non_send_resource_mut(&mut self) -> Option> { // SAFETY: unique world access @@ -991,6 +1009,9 @@ impl World { /// Gets a mutable reference to the non-send resource of the given type, if it exists. /// Otherwise returns [None] /// + /// # Panics + /// This function will panic if called from a thread that the value was inserted from. + /// /// # Safety /// This will allow aliased mutable access to the given non-send resource type. The caller must /// ensure that there is either only one mutable access or multiple immutable accesses at a time. @@ -1010,6 +1031,9 @@ impl World { } // Shorthand helper function for getting the data and change ticks for a resource. + /// + /// # Panics + /// This function will panic if called from a thread that the value was inserted from. #[inline] pub(crate) fn get_non_send_with_ticks( &self, @@ -1416,6 +1440,10 @@ impl World { /// **You should prefer to use the typed API [`World::insert_resource`] where possible and only /// use this in cases where the actual types are not known at compile time.** /// + /// # Panics + /// If `is_send` is false, and a value is already present, this function will panic if called + /// from a thread that the original value was inserted from. + /// /// # Safety /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world /// `component_id` must exist in this [`World`]. `is_send` must correspond to whether the resource From 93334a0087679cd83e857274c8fc8f2eb8809e2e Mon Sep 17 00:00:00 2001 From: James Liu Date: Sat, 3 Dec 2022 12:46:13 -0800 Subject: [PATCH 21/48] Apply suggestions from code review Co-authored-by: Boxy --- crates/bevy_ecs/src/storage/resource.rs | 2 +- crates/bevy_ecs/src/world/mod.rs | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index c8d6d401cb629..abc5d269e76cc 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -240,7 +240,7 @@ impl Resources { column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), type_name: String::from(component_info.name()), id: f(), - origin_thread_id: (!SEND).then(|| std::thread::current().id()), + origin_thread_id: None, } }) } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 6828a7756400c..b2da2267d531f 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1220,7 +1220,6 @@ impl World { .get_resource_id(TypeId::of::()) .unwrap_or_else(|| panic!("resource does not exist: {}", std::any::type_name::())); // If the resource isn't send and sync, validate that we are on the main thread, so that we can access it. - let _component_info = self.components().get_info(component_id).unwrap(); let (ptr, mut ticks) = self .storages .resources @@ -1297,7 +1296,6 @@ impl World { .get_resource_id(TypeId::of::()) .unwrap_or_else(|| panic!("resource does not exist: {}", std::any::type_name::())); // If the resource isn't send and sync, validate that we are on the main thread, so that we can access it. - let _component_info = self.components().get_info(component_id).unwrap(); let (ptr, mut ticks) = self .storages .non_send_resources @@ -1583,7 +1581,6 @@ impl World { /// use this in cases where the actual types are not known at compile time.** #[inline] pub fn get_resource_by_id(&self, component_id: ComponentId) -> Option> { - let _info = self.components.get_info(component_id)?; self.storages.resources.get(component_id)?.get_data() } @@ -1595,7 +1592,6 @@ impl World { /// use this in cases where the actual types are not known at compile time.** #[inline] pub fn get_resource_mut_by_id(&mut self, component_id: ComponentId) -> Option> { - let _info = self.components.get_info(component_id)?; let (ptr, ticks) = self.get_resource_with_ticks(component_id)?; // SAFETY: This function has exclusive access to the world so nothing aliases `ticks`. @@ -1617,7 +1613,6 @@ impl World { /// **You should prefer to use the typed API [`World::remove_resource`] where possible and only /// use this in cases where the actual types are not known at compile time.** pub fn remove_resource_by_id(&mut self, component_id: ComponentId) -> Option<()> { - let _info = self.components.get_info(component_id)?; // SAFETY: The underlying type is Send and Sync or we've already validated we're on the main thread unsafe { self.storages @@ -1633,7 +1628,6 @@ impl World { /// **You should prefer to use the typed API [`World::remove_resource`] where possible and only /// use this in cases where the actual types are not known at compile time.** pub fn remove_non_send_by_id(&mut self, component_id: ComponentId) -> Option<()> { - let _info = self.components.get_info(component_id)?; // SAFETY: The underlying type is Send and Sync or we've already validated we're on the main thread unsafe { self.storages From 8f722928081a75207184c27dab2922052b94485c Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 3 Dec 2022 12:51:53 -0800 Subject: [PATCH 22/48] Remove non_send_scope --- crates/bevy_ecs/src/lib.rs | 32 ----------- crates/bevy_ecs/src/storage/blob_vec.rs | 19 ++++++- crates/bevy_ecs/src/world/mod.rs | 76 ------------------------- 3 files changed, 17 insertions(+), 110 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 1eb8a710ae180..95200beeea97d 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1321,38 +1321,6 @@ mod tests { assert_eq!(world.resource::().0, 1); } - #[test] - fn non_send_resource_scope() { - let mut world = World::default(); - world.insert_non_send_resource(NonSendA::default()); - world.non_send_scope(|world: &mut World, mut value: Mut| { - value.0 += 1; - assert!(!world.contains_non_send::()); - }); - assert_eq!(world.non_send_resource::().0, 1); - } - - #[test] - #[should_panic( - expected = "Attempted to access or drop non-send resource bevy_ecs::tests::NonSendA from thread" - )] - fn non_send_resource_scope_from_different_thread() { - let mut world = World::default(); - world.insert_non_send_resource(NonSendA::default()); - - let thread = std::thread::spawn(move || { - // Accessing the non-send resource on a different thread - // Should result in a panic - world.non_send_scope(|_: &mut World, mut value: Mut| { - value.0 += 1; - }); - }); - - if let Err(err) = thread.join() { - std::panic::resume_unwind(err); - } - } - #[test] #[should_panic( expected = "Attempted to access or drop non-send resource bevy_ecs::tests::NonSendA from thread" diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index c0bfa3302572c..5a57d38e82415 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -7,6 +7,14 @@ use std::{ use bevy_ptr::{OwningPtr, Ptr, PtrMut}; +struct CallOnDrop(F); + +impl Drop for CallOnDrop { + fn drop(&mut self) { + self.0() + } +} + /// A flat, type-erased data storage type /// /// Used to densely store homogeneous ECS data. @@ -252,9 +260,16 @@ impl BlobVec { pub unsafe fn swap_remove_and_drop_unchecked(&mut self, index: usize) { debug_assert!(index < self.len()); let drop = self.drop; - let value = self.swap_remove_and_forget_unchecked(index); + let last = self.get_unchecked_mut(self.len - 1).as_ptr(); + let target = self.get_unchecked_mut(index).as_ptr(); + // Ensure that the element is moved and the len is updated even if + // the drop call has panicked. + let _guard = CallOnDrop(|| { + std::ptr::copy::(last, target, self.item_layout.size()); + self.len -= 1; + }); if let Some(drop) = drop { - (drop)(value); + (drop)(OwningPtr::new(NonNull::new_unchecked(target))); } } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index b2da2267d531f..3638020ba8c6a 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1264,82 +1264,6 @@ impl World { result } - /// Temporarily removes the requested `!Send` resource from this [`World`], then re-adds it before - /// returning. - /// - /// This enables safe simultaneous mutable access to both a resource and the rest of the [`World`]. - /// For more complex access patterns, consider using [`SystemState`](crate::system::SystemState). - /// - /// # Example - /// ``` - /// use bevy_ecs::prelude::*; - /// #[derive(Resource)] - /// struct A(u32); - /// #[derive(Component)] - /// struct B(u32); - /// let mut world = World::new(); - /// world.insert_resource(A(1)); - /// let entity = world.spawn(B(1)).id(); - /// - /// world.resource_scope(|world, mut a: Mut| { - /// let b = world.get_mut::(entity).unwrap(); - /// a.0 += b.0; - /// }); - /// assert_eq!(world.get_resource::().unwrap().0, 2); - /// ``` - pub fn non_send_scope(&mut self, f: impl FnOnce(&mut World, Mut) -> U) -> U { - let last_change_tick = self.last_change_tick(); - let change_tick = self.change_tick(); - - let component_id = self - .components - .get_resource_id(TypeId::of::()) - .unwrap_or_else(|| panic!("resource does not exist: {}", std::any::type_name::())); - // If the resource isn't send and sync, validate that we are on the main thread, so that we can access it. - let (ptr, mut ticks) = self - .storages - .non_send_resources - .get_mut(component_id) - // SAFETY: The type R is Send and Sync or we've already validated that we're on the main thread. - .and_then(|info| unsafe { info.remove() }) - .unwrap_or_else(|| panic!("resource does not exist: {}", std::any::type_name::())); - // Read the value onto the stack to avoid potential mut aliasing. - // SAFETY: pointer is of type R - let mut value = unsafe { ptr.read::() }; - let value_mut = Mut { - value: &mut value, - ticks: Ticks { - added: &mut ticks.added, - changed: &mut ticks.changed, - last_change_tick, - change_tick, - }, - }; - let result = f(self, value_mut); - assert!(!self.contains_non_send::(), - "Resource `{}` was inserted during a call to World::resource_scope.\n\ - This is not allowed as the original resource is reinserted to the world after the FnOnce param is invoked.", - std::any::type_name::()); - - OwningPtr::make(value, |ptr| { - // SAFETY: pointer is of type R - unsafe { - self.storages - .non_send_resources - .get_mut(component_id) - .map(|info| info.insert_with_ticks(ptr, ticks)) - .unwrap_or_else(|| { - panic!( - "No resource of type {} exists in the World.", - std::any::type_name::() - ) - }); - } - }); - - result - } - /// Sends an [`Event`](crate::event::Event). #[inline] pub fn send_event(&mut self, event: E) { From b3fffd905fc829432ea8eb17c1fc544f9352a1e6 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 3 Dec 2022 12:58:29 -0800 Subject: [PATCH 23/48] Split insert_non_send_by_id from insert_resource_by_id --- crates/bevy_ecs/src/world/mod.rs | 47 ++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 3638020ba8c6a..faec899b81354 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -732,9 +732,9 @@ impl World { pub fn insert_resource(&mut self, value: R) { let component_id = self.components.init_resource::(); OwningPtr::make(value, |ptr| { - // SAFETY: component_id was just initialized and corresponds to resource of type R + // SAFETY: component_id was just initialized and corresponds to resource of type R, which is Send unsafe { - self.insert_resource_by_id(component_id, true, ptr); + self.insert_resource_by_id(component_id, ptr); } }); } @@ -773,7 +773,7 @@ impl World { OwningPtr::make(value, |ptr| { // SAFETY: component_id was just initialized and corresponds to resource of type R unsafe { - self.insert_resource_by_id(component_id, false, ptr); + self.insert_non_send_by_id(component_id, ptr); } }); } @@ -1368,25 +1368,44 @@ impl World { /// /// # Safety /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world - /// `component_id` must exist in this [`World`]. `is_send` must correspond to whether the resource - /// adheres to invariants of `Send`. + /// `component_id` must exist in this [`World`].`value` must be adhere to invariants of `Send`. #[inline] pub unsafe fn insert_resource_by_id( &mut self, component_id: ComponentId, - is_send: bool, value: OwningPtr<'_>, ) { let change_tick = self.change_tick(); - // SAFETY: component_id and is_send are valid, ensured by caller - if is_send { - self.initialize_resource_internal(component_id) - .insert(value, change_tick); - } else { - self.initialize_non_send_internal(component_id) - .insert(value, change_tick); - } + // SAFETY: component_id is valid and corresponds to a Send type, ensured by caller + self.initialize_resource_internal(component_id) + .insert(value, change_tick); + } + + /// Inserts a new `!Send` resource with the given `value`. Will replace the value if it already + /// existed. + /// + /// **You should prefer to use the typed API [`World::insert_non_send`] where possible and only + /// use this in cases where the actual types are not known at compile time.** + /// + /// # Panics + /// If a value is already present, this function will panic if called from a thread that the original + /// value was inserted from. + /// + /// # Safety + /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world + /// `component_id` must exist in this [`World`]. + #[inline] + pub unsafe fn insert_non_send_by_id( + &mut self, + component_id: ComponentId, + value: OwningPtr<'_>, + ) { + let change_tick = self.change_tick(); + + // SAFETY: component_id is valid, ensured by caller + self.initialize_non_send_internal(component_id) + .insert(value, change_tick); } /// # Safety From 54e5a504ad42f666dcc11435dfe6217347fc4b95 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 3 Dec 2022 13:06:44 -0800 Subject: [PATCH 24/48] Fix safety comment and tests --- crates/bevy_ecs/src/world/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index faec899b81354..835488a004e0b 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1862,9 +1862,9 @@ mod tests { let value: [u8; 8] = [0, 1, 2, 3, 4, 5, 6, 7]; OwningPtr::make(value, |ptr| { - // SAFETY: value is valid for the component layout + // SAFETY: value is valid for the component layout, the array is Send unsafe { - world.insert_resource_by_id(component_id, true, ptr); + world.insert_resource_by_id(component_id, ptr); } }); From 5f6e91bbd63dc3616b586fbcc2b3881dff82d715 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 3 Dec 2022 13:10:00 -0800 Subject: [PATCH 25/48] Exhaustively decompose Storages in check_change_ticks --- crates/bevy_ecs/src/world/mod.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 835488a004e0b..f1481c58aed74 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1499,12 +1499,16 @@ impl World { // Iterate over all component change ticks, clamping their age to max age // PERF: parallelize let change_tick = self.change_tick(); - self.storages.tables.check_change_ticks(change_tick); - self.storages.sparse_sets.check_change_ticks(change_tick); - self.storages.resources.check_change_ticks(change_tick); - self.storages - .non_send_resources - .check_change_ticks(change_tick); + let Storages { + ref mut tables, + ref mut sparse_sets, + ref mut resources, + ref mut non_send_resources, + } = self.storages; + tables.check_change_ticks(change_tick); + sparse_sets.check_change_ticks(change_tick); + resources.check_change_ticks(change_tick); + non_send_resources.check_change_ticks(change_tick); } pub fn clear_entities(&mut self) { From cfb81731f74682303c4b62b431b76dcb5f3e0801 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 3 Dec 2022 13:11:13 -0800 Subject: [PATCH 26/48] Remove spurious is_send in docs --- crates/bevy_ecs/src/world/mod.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index f1481c58aed74..666daf4efd398 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1362,10 +1362,6 @@ impl World { /// **You should prefer to use the typed API [`World::insert_resource`] where possible and only /// use this in cases where the actual types are not known at compile time.** /// - /// # Panics - /// If `is_send` is false, and a value is already present, this function will panic if called - /// from a thread that the original value was inserted from. - /// /// # Safety /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world /// `component_id` must exist in this [`World`].`value` must be adhere to invariants of `Send`. @@ -1427,7 +1423,6 @@ impl World { /// # Safety /// `component_id` must be valid for this world - /// `is_send` should correspond for the resource being initialized. #[inline] unsafe fn initialize_non_send_internal( &mut self, From 5900696eeda294b53c6f3a458dd33638c07ae856 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 3 Dec 2022 13:12:14 -0800 Subject: [PATCH 27/48] Update safety docs on initialize_resource_internal --- crates/bevy_ecs/src/world/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 666daf4efd398..57710b3d22c69 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1405,7 +1405,7 @@ impl World { } /// # Safety - /// `component_id` must be valid for this world + /// `component_id` must be valid for this world as a `Send` component type #[inline] unsafe fn initialize_resource_internal( &mut self, From 95e575fd8e6940fe2182febbbeea2efe3e0dca6e Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 3 Dec 2022 13:16:52 -0800 Subject: [PATCH 28/48] Check that Send resources have correct metadata on initialization --- crates/bevy_ecs/src/storage/resource.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index abc5d269e76cc..686ab4b131006 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -228,6 +228,12 @@ impl Resources { /// /// # Panics /// Will panic if `component_id` is not valid for the provided `components` + /// + /// # Safety + /// If `SEND` is true, `component_id` must be initialized in Components as a `Send` and + /// `Sync` component. + /// + /// This is checked in debug builds and will panic if violated. pub(crate) fn initialize_with( &mut self, component_id: ComponentId, @@ -236,6 +242,9 @@ impl Resources { ) -> &mut ResourceData { self.resources.get_or_insert_with(component_id, || { let component_info = components.get_info(component_id).unwrap(); + if SEND { + debug_assert!(component_info.is_send_and_sync()); + } ResourceData { column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), type_name: String::from(component_info.name()), From 2bff28c8e9f6f6c3044084bfaa5644e30b9a8162 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 3 Dec 2022 13:18:11 -0800 Subject: [PATCH 29/48] Formatting --- crates/bevy_ecs/src/world/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 57710b3d22c69..19ff51b91c049 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -773,7 +773,7 @@ impl World { OwningPtr::make(value, |ptr| { // SAFETY: component_id was just initialized and corresponds to resource of type R unsafe { - self.insert_non_send_by_id(component_id, ptr); + self.insert_non_send_by_id(component_id, ptr); } }); } @@ -1377,15 +1377,15 @@ impl World { self.initialize_resource_internal(component_id) .insert(value, change_tick); } - - /// Inserts a new `!Send` resource with the given `value`. Will replace the value if it already + + /// Inserts a new `!Send` resource with the given `value`. Will replace the value if it already /// existed. /// /// **You should prefer to use the typed API [`World::insert_non_send`] where possible and only /// use this in cases where the actual types are not known at compile time.** /// /// # Panics - /// If a value is already present, this function will panic if called from a thread that the original + /// If a value is already present, this function will panic if called from a thread that the original /// value was inserted from. /// /// # Safety From 6af4c4e827d3c8583352151d9b0032a1f45345d6 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 3 Dec 2022 13:20:57 -0800 Subject: [PATCH 30/48] Undo changes to BlobVec, how the hell did this get in here --- crates/bevy_ecs/src/storage/blob_vec.rs | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 5a57d38e82415..c0bfa3302572c 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -7,14 +7,6 @@ use std::{ use bevy_ptr::{OwningPtr, Ptr, PtrMut}; -struct CallOnDrop(F); - -impl Drop for CallOnDrop { - fn drop(&mut self) { - self.0() - } -} - /// A flat, type-erased data storage type /// /// Used to densely store homogeneous ECS data. @@ -260,16 +252,9 @@ impl BlobVec { pub unsafe fn swap_remove_and_drop_unchecked(&mut self, index: usize) { debug_assert!(index < self.len()); let drop = self.drop; - let last = self.get_unchecked_mut(self.len - 1).as_ptr(); - let target = self.get_unchecked_mut(index).as_ptr(); - // Ensure that the element is moved and the len is updated even if - // the drop call has panicked. - let _guard = CallOnDrop(|| { - std::ptr::copy::(last, target, self.item_layout.size()); - self.len -= 1; - }); + let value = self.swap_remove_and_forget_unchecked(index); if let Some(drop) = drop { - (drop)(OwningPtr::new(NonNull::new_unchecked(target))); + (drop)(value); } } From e0566260daf93489d8feaf0e8c7995e2fef64193 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 3 Dec 2022 13:30:17 -0800 Subject: [PATCH 31/48] Fix docs link --- crates/bevy_ecs/src/world/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 19ff51b91c049..a0fe20d2c4aa3 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1381,7 +1381,7 @@ impl World { /// Inserts a new `!Send` resource with the given `value`. Will replace the value if it already /// existed. /// - /// **You should prefer to use the typed API [`World::insert_non_send`] where possible and only + /// **You should prefer to use the typed API [`World::insert_non_send_resource`] where possible and only /// use this in cases where the actual types are not known at compile time.** /// /// # Panics From 58bcb66f7eb0a90c214469b2e8fba17821ace3b4 Mon Sep 17 00:00:00 2001 From: James Liu Date: Sun, 4 Dec 2022 15:02:08 -0800 Subject: [PATCH 32/48] Apply suggestions from code review Co-authored-by: Boxy --- crates/bevy_ecs/src/storage/resource.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 686ab4b131006..9ef2db54dc97c 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -99,7 +99,6 @@ impl ResourceData { /// /// # Safety /// - `value` must be valid for the underlying type for the resource. - /// - The underlying type must be [`Send`] if `SEND` is true. #[inline] pub(crate) unsafe fn insert(&mut self, value: OwningPtr<'_>, change_tick: u32) { if self.is_present() { @@ -148,12 +147,9 @@ impl ResourceData { /// If `SEND` is false, this will panic if a value is present and is not removed from the /// original thread it was inserted from. /// - /// # Safety - /// - the removed value must be used or dropped. - /// - the underlying type must be [`Send`] if `SEND` is true. #[inline] #[must_use = "The returned pointer to the removed component should be used or dropped"] - pub(crate) unsafe fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { + pub(crate) fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { if SEND { self.column.swap_remove_and_forget(0) } else { @@ -169,8 +165,6 @@ impl ResourceData { /// If `SEND` is false, this will panic if a value is present and is not /// accessed from the original thread it was inserted in. /// - /// # Safety - /// - the underlying type must be [`Send`] if `SEND` is true. #[inline] pub(crate) unsafe fn remove_and_drop(&mut self) { if self.is_present() { From 9db8041b1da87ce28348436913e74174d3a6b9e8 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 4 Dec 2022 15:00:59 -0800 Subject: [PATCH 33/48] Stronger assert --- crates/bevy_ecs/src/storage/resource.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 9ef2db54dc97c..f59ec7b35825f 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -236,9 +236,7 @@ impl Resources { ) -> &mut ResourceData { self.resources.get_or_insert_with(component_id, || { let component_info = components.get_info(component_id).unwrap(); - if SEND { - debug_assert!(component_info.is_send_and_sync()); - } + assert!(SEND == component_info.is_send_and_sync()); ResourceData { column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), type_name: String::from(component_info.name()), From d713d8440321fc160a308ad88e7ab994aff8072e Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 4 Dec 2022 15:47:12 -0800 Subject: [PATCH 34/48] Fix CI --- crates/bevy_ecs/src/storage/resource.rs | 8 ++++---- crates/bevy_ecs/src/world/mod.rs | 25 +++++++++---------------- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index f59ec7b35825f..1992f551e2a57 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -146,7 +146,6 @@ impl ResourceData { /// # Panics /// If `SEND` is false, this will panic if a value is present and is not removed from the /// original thread it was inserted from. - /// #[inline] #[must_use = "The returned pointer to the removed component should be used or dropped"] pub(crate) fn remove(&mut self) -> Option<(OwningPtr<'_>, ComponentTicks)> { @@ -164,9 +163,8 @@ impl ResourceData { /// # Panics /// If `SEND` is false, this will panic if a value is present and is not /// accessed from the original thread it was inserted in. - /// #[inline] - pub(crate) unsafe fn remove_and_drop(&mut self) { + pub(crate) fn remove_and_drop(&mut self) { if self.is_present() { self.validate_access(); self.column.clear(); @@ -236,7 +234,9 @@ impl Resources { ) -> &mut ResourceData { self.resources.get_or_insert_with(component_id, || { let component_info = components.get_info(component_id).unwrap(); - assert!(SEND == component_info.is_send_and_sync()); + if SEND { + assert!(component_info.is_send_and_sync()); + } ResourceData { column: ManuallyDrop::new(Column::with_capacity(component_info, 1)), type_name: String::from(component_info.name()), diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index a0fe20d2c4aa3..3d40ee2e1e084 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1224,8 +1224,7 @@ impl World { .storages .resources .get_mut(component_id) - // SAFETY: The type R is Send and Sync or we've already validated that we're on the main thread. - .and_then(|info| unsafe { info.remove() }) + .and_then(|info| info.remove()) .unwrap_or_else(|| panic!("resource does not exist: {}", std::any::type_name::())); // Read the value onto the stack to avoid potential mut aliasing. // SAFETY: pointer is of type R @@ -1555,13 +1554,10 @@ impl World { /// **You should prefer to use the typed API [`World::remove_resource`] where possible and only /// use this in cases where the actual types are not known at compile time.** pub fn remove_resource_by_id(&mut self, component_id: ComponentId) -> Option<()> { - // SAFETY: The underlying type is Send and Sync or we've already validated we're on the main thread - unsafe { - self.storages - .resources - .get_mut(component_id)? - .remove_and_drop(); - } + self.storages + .resources + .get_mut(component_id)? + .remove_and_drop(); Some(()) } @@ -1570,13 +1566,10 @@ impl World { /// **You should prefer to use the typed API [`World::remove_resource`] where possible and only /// use this in cases where the actual types are not known at compile time.** pub fn remove_non_send_by_id(&mut self, component_id: ComponentId) -> Option<()> { - // SAFETY: The underlying type is Send and Sync or we've already validated we're on the main thread - unsafe { - self.storages - .non_send_resources - .get_mut(component_id)? - .remove_and_drop(); - } + self.storages + .non_send_resources + .get_mut(component_id)? + .remove_and_drop(); Some(()) } From 314a86e182227f4fa245eceb78caf071267bae0d Mon Sep 17 00:00:00 2001 From: James Liu Date: Mon, 5 Dec 2022 11:25:25 -0800 Subject: [PATCH 35/48] Apply suggestions from code review Co-authored-by: Boxy --- crates/bevy_ecs/src/storage/resource.rs | 8 +---- crates/bevy_ecs/src/world/mod.rs | 47 +++++++++++-------------- 2 files changed, 22 insertions(+), 33 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 1992f551e2a57..dd85ba3884e53 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -121,7 +121,6 @@ impl ResourceData { /// /// # Safety /// - `value` must be valid for the underlying type for the resource. - /// - The underlying type must be [`Send`] if `SEND` is true. #[inline] pub(crate) unsafe fn insert_with_ticks( &mut self, @@ -220,12 +219,7 @@ impl Resources { /// /// # Panics /// Will panic if `component_id` is not valid for the provided `components` - /// - /// # Safety - /// If `SEND` is true, `component_id` must be initialized in Components as a `Send` and - /// `Sync` component. - /// - /// This is checked in debug builds and will panic if violated. + /// If `SEND` is false, this will panic if `component_id`'s `ComponentInfo` is not registered as being `Send` + `Sync`. pub(crate) fn initialize_with( &mut self, component_id: ComponentId, diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 3d40ee2e1e084..089e2db424de7 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -732,7 +732,7 @@ impl World { pub fn insert_resource(&mut self, value: R) { let component_id = self.components.init_resource::(); OwningPtr::make(value, |ptr| { - // SAFETY: component_id was just initialized and corresponds to resource of type R, which is Send + // SAFETY: component_id was just initialized and corresponds to resource of type R unsafe { self.insert_resource_by_id(component_id, ptr); } @@ -782,11 +782,9 @@ impl World { #[inline] pub fn remove_resource(&mut self) -> Option { let component_id = self.components.get_resource_id(TypeId::of::())?; - // SAFETY: the resource is of type R and the value is returned back to the caller. - unsafe { - let (ptr, _) = self.storages.resources.get_mut(component_id)?.remove()?; - Some(ptr.read::()) - } + let (ptr, _) = self.storages.resources.get_mut(component_id)?.remove()?; + // SAFETY: `component_id` was gotten via looking up the `R` type + unsafe { Some(ptr.read::()) } } /// Removes a `!Send` resource from the world and returns it, if present. @@ -946,7 +944,7 @@ impl World { /// Panics if the resource does not exist. /// Use [`get_non_send_resource`](World::get_non_send_resource) instead if you want to handle this case. /// - /// This function will panic if called from a thread that the value was inserted from. + /// This function will panic if it isn't called from the same thread that the resource was inserted from. #[inline] #[track_caller] pub fn non_send_resource(&self) -> &R { @@ -968,7 +966,7 @@ impl World { /// Panics if the resource does not exist. /// Use [`get_non_send_resource_mut`](World::get_non_send_resource_mut) instead if you want to handle this case. /// - /// This function will panic if called from a thread that the value was inserted from. + /// This function will panic if it isn't called from the same thread that the resource was inserted from. #[inline] #[track_caller] pub fn non_send_resource_mut(&mut self) -> Mut<'_, R> { @@ -987,7 +985,7 @@ impl World { /// Otherwise returns [None]. /// /// # Panics - /// This function will panic if called from a thread that the value was inserted from. + /// This function will panic if it isn't called from the same thread that the resource was inserted from. #[inline] pub fn get_non_send_resource(&self) -> Option<&R> { let component_id = self.components.get_resource_id(TypeId::of::())?; @@ -999,7 +997,7 @@ impl World { /// Otherwise returns [None] /// /// # Panics - /// This function will panic if called from a thread that the value was inserted from. + /// This function will panic if it isn't called from the same thread that the resource was inserted from. #[inline] pub fn get_non_send_resource_mut(&mut self) -> Option> { // SAFETY: unique world access @@ -1010,7 +1008,7 @@ impl World { /// Otherwise returns [None] /// /// # Panics - /// This function will panic if called from a thread that the value was inserted from. + /// This function will panic if it isn't called from the same thread that the resource was inserted from. /// /// # Safety /// This will allow aliased mutable access to the given non-send resource type. The caller must @@ -1033,7 +1031,7 @@ impl World { // Shorthand helper function for getting the data and change ticks for a resource. /// /// # Panics - /// This function will panic if called from a thread that the value was inserted from. + /// This function will panic if it isn't called from the same thread that the resource was inserted from. #[inline] pub(crate) fn get_non_send_with_ticks( &self, @@ -1363,7 +1361,7 @@ impl World { /// /// # Safety /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world - /// `component_id` must exist in this [`World`].`value` must be adhere to invariants of `Send`. + /// `component_id` must exist in this [`World`]. #[inline] pub unsafe fn insert_resource_by_id( &mut self, @@ -1372,7 +1370,7 @@ impl World { ) { let change_tick = self.change_tick(); - // SAFETY: component_id is valid and corresponds to a Send type, ensured by caller + // SAFETY: component_id is valid, ensured by caller self.initialize_resource_internal(component_id) .insert(value, change_tick); } @@ -1403,10 +1401,10 @@ impl World { .insert(value, change_tick); } - /// # Safety - /// `component_id` must be valid for this world as a `Send` component type + /// # Panics + /// Panics if `component_id` is not registered as a `Send` component type in this `World` #[inline] - unsafe fn initialize_resource_internal( + fn initialize_resource_internal( &mut self, component_id: ComponentId, ) -> &mut ResourceData { @@ -1420,15 +1418,14 @@ impl World { }) } - /// # Safety - /// `component_id` must be valid for this world + /// # Panics + /// panics if `component_id` is not registered in this world #[inline] - unsafe fn initialize_non_send_internal( + fn initialize_non_send_internal( &mut self, component_id: ComponentId, ) -> &mut ResourceData { let archetype_component_count = &mut self.archetypes.archetype_component_count; - // SAFETY: Caller guarentees that `is_send` matches the underlying type. self.storages .non_send_resources .initialize_with(component_id, &self.components, || { @@ -1440,15 +1437,13 @@ impl World { pub(crate) fn initialize_resource(&mut self) -> ComponentId { let component_id = self.components.init_resource::(); - // SAFETY: resource initialized above, resource type must be Send - unsafe { self.initialize_resource_internal(component_id) }; + self.initialize_resource_internal(component_id); component_id } pub(crate) fn initialize_non_send_resource(&mut self) -> ComponentId { let component_id = self.components.init_non_send::(); - // SAFETY: resource initialized above, resource type might not be send - unsafe { self.initialize_non_send_internal(component_id) }; + self.initialize_non_send_internal(component_id); component_id } @@ -1854,7 +1849,7 @@ mod tests { let value: [u8; 8] = [0, 1, 2, 3, 4, 5, 6, 7]; OwningPtr::make(value, |ptr| { - // SAFETY: value is valid for the component layout, the array is Send + // SAFETY: value is valid for the component layout unsafe { world.insert_resource_by_id(component_id, ptr); } From e06677acd482b801cb70366e249d659fe9c9e842 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 5 Dec 2022 11:35:49 -0800 Subject: [PATCH 36/48] Small cleanup --- crates/bevy_ecs/src/world/mod.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 089e2db424de7..3ec0fc78ae38b 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -801,15 +801,13 @@ impl World { #[inline] pub fn remove_non_send_resource(&mut self) -> Option { let component_id = self.components.get_resource_id(TypeId::of::())?; + let (ptr, _) = self + .storages + .non_send_resources + .get_mut(component_id)? + .remove()?; // SAFETY: the resource is of type R and the value is returned back to the caller. - unsafe { - let (ptr, _) = self - .storages - .non_send_resources - .get_mut(component_id)? - .remove()?; - Some(ptr.read::()) - } + unsafe { Some(ptr.read::()) } } /// Returns `true` if a resource of type `R` exists. Otherwise returns `false`. @@ -1370,7 +1368,7 @@ impl World { ) { let change_tick = self.change_tick(); - // SAFETY: component_id is valid, ensured by caller + // SAFETY: value is valid for component_id, ensured by caller self.initialize_resource_internal(component_id) .insert(value, change_tick); } @@ -1396,7 +1394,7 @@ impl World { ) { let change_tick = self.change_tick(); - // SAFETY: component_id is valid, ensured by caller + // SAFETY: value is valid for component_id, ensured by caller self.initialize_non_send_internal(component_id) .insert(value, change_tick); } From de05d57e1b013491cf304ee5f1fdaf83bc59b63b Mon Sep 17 00:00:00 2001 From: James Liu Date: Mon, 5 Dec 2022 12:17:44 -0800 Subject: [PATCH 37/48] Update crates/bevy_ecs/src/world/mod.rs Co-authored-by: Boxy --- crates/bevy_ecs/src/world/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 3ec0fc78ae38b..565cc3a4a4579 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -806,7 +806,7 @@ impl World { .non_send_resources .get_mut(component_id)? .remove()?; - // SAFETY: the resource is of type R and the value is returned back to the caller. + // SAFETY: `component_id` was gotten via looking up the `R` type unsafe { Some(ptr.read::()) } } From db4c6d76e1ff3646f8b80efcdc92343ae2d359c8 Mon Sep 17 00:00:00 2001 From: James Liu Date: Wed, 7 Dec 2022 21:18:02 -0800 Subject: [PATCH 38/48] Apply suggestions from code review Co-authored-by: Boxy --- crates/bevy_ecs/src/storage/resource.rs | 2 +- crates/bevy_ecs/src/world/mod.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 140369da5f73d..98cec31a649b9 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -37,7 +37,7 @@ impl ResourceData { #[inline] fn validate_access(&self) { // Avoid aborting due to double panicking on the same thread. - if SEND || std::thread::panicking() { + if SEND { return; } if self.origin_thread_id != Some(std::thread::current().id()) { diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 13345e52c5152..254f7d67d409a 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -805,8 +805,8 @@ impl World { /// Returns `None` if a value was not previously present. /// /// # Panics - /// If a value is present, this function will panic if called from a thread that the - /// value was inserted from. + /// If a value is present, this function will panic if called from a different + /// thread than where the value was inserted from. #[inline] pub fn remove_non_send_resource(&mut self) -> Option { let component_id = self.components.get_resource_id(TypeId::of::())?; @@ -1389,8 +1389,8 @@ impl World { /// use this in cases where the actual types are not known at compile time.** /// /// # Panics - /// If a value is already present, this function will panic if called from a thread that the original - /// value was inserted from. + /// If a value is already present, this function will panic if not called from the same + /// thread that the original value was inserted from. /// /// # Safety /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world From 5e182fc5824fb27966b98ccf3aa237122b923278 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 7 Dec 2022 21:22:11 -0800 Subject: [PATCH 39/48] Move std::thread::panicking check to Drop impl --- crates/bevy_ecs/src/storage/resource.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 98cec31a649b9..83f35db20d80b 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -19,6 +19,12 @@ pub struct ResourceData { impl Drop for ResourceData { fn drop(&mut self) { if self.is_present() { + // If this thread is already panicking, panicking again will cause + // the entire process to abort. In this case we choose to avoid + // dropping or checking this altogether and just leak the column. + if std::thread::panicking() { + return; + } self.validate_access(); } // SAFETY: Drop is only called once upon dropping the ResourceData @@ -36,7 +42,6 @@ impl ResourceData { #[inline] fn validate_access(&self) { - // Avoid aborting due to double panicking on the same thread. if SEND { return; } From 263ed859adc1101c561d80e72f84c6de07ad013a Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 7 Dec 2022 21:26:08 -0800 Subject: [PATCH 40/48] Update safety comment about validate_access in Drop --- crates/bevy_ecs/src/storage/resource.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 83f35db20d80b..9689e9bbc5b6f 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -29,7 +29,8 @@ impl Drop for ResourceData { } // SAFETY: Drop is only called once upon dropping the ResourceData // and is inaccessible after this as the parent ResourceData has - // been dropped. + // been dropped. The validate_access call above will check that the + // data is dropped on the thread it was inserted from. unsafe { ManuallyDrop::drop(&mut self.column); } From e4a0a9a8a144d47c878caebccb742cd14f5270a5 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 7 Dec 2022 21:34:30 -0800 Subject: [PATCH 41/48] Add the missing get_non_send_by_id APIs --- crates/bevy_ecs/src/world/mod.rs | 40 ++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 254f7d67d409a..0c8a54e984c41 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1558,6 +1558,46 @@ impl World { }) } + /// Gets a `!Send` resource to the resource with the id [`ComponentId`] if it exists. + /// The returned pointer must not be used to modify the resource, and must not be + /// dereferenced after the immutable borrow of the [`World`] ends. + /// + /// **You should prefer to use the typed API [`World::get_resource`] where possible and only + /// use this in cases where the actual types are not known at compile time.** + /// + /// # Panics + /// This function will panic if it isn't called from the same thread that the resource was inserted from. + #[inline] + pub fn get_non_send_by_id(&self, component_id: ComponentId) -> Option> { + self.storages.non_send_resources.get(component_id)?.get_data() + } + + /// Gets a `!Send` resource to the resource with the id [`ComponentId`] if it exists. + /// The returned pointer may be used to modify the resource, as long as the mutable borrow + /// of the [`World`] is still valid. + /// + /// **You should prefer to use the typed API [`World::get_resource_mut`] where possible and only + /// use this in cases where the actual types are not known at compile time.** + /// + /// # Panics + /// This function will panic if it isn't called from the same thread that the resource was inserted from. + #[inline] + pub fn get_non_send_mut_by_id(&mut self, component_id: ComponentId) -> Option> { + let change_tick = self.change_tick(); + let (ptr, ticks) = self.get_non_send_with_ticks(component_id)?; + + // SAFETY: This function has exclusive access to the world so nothing aliases `ticks`. + // - index is in-bounds because the column is initialized and non-empty + // - no other reference to the ticks of the same row can exist at the same time + let ticks = unsafe { Ticks::from_tick_cells(ticks, self.last_change_tick(), change_tick) }; + + Some(MutUntyped { + // SAFETY: This function has exclusive access to the world so nothing aliases `ptr`. + value: unsafe { ptr.assert_unique() }, + ticks, + }) + } + /// Removes the resource of a given type, if it exists. Otherwise returns [None]. /// /// **You should prefer to use the typed API [`World::remove_resource`] where possible and only From 5be8450708dc70984f8006769fc71b3d7d4634de Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 7 Dec 2022 21:40:23 -0800 Subject: [PATCH 42/48] Formatting --- crates/bevy_ecs/src/storage/resource.rs | 4 ++-- crates/bevy_ecs/src/world/mod.rs | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 9689e9bbc5b6f..40f5554591337 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -20,7 +20,7 @@ impl Drop for ResourceData { fn drop(&mut self) { if self.is_present() { // If this thread is already panicking, panicking again will cause - // the entire process to abort. In this case we choose to avoid + // the entire process to abort. In this case we choose to avoid // dropping or checking this altogether and just leak the column. if std::thread::panicking() { return; @@ -29,7 +29,7 @@ impl Drop for ResourceData { } // SAFETY: Drop is only called once upon dropping the ResourceData // and is inaccessible after this as the parent ResourceData has - // been dropped. The validate_access call above will check that the + // been dropped. The validate_access call above will check that the // data is dropped on the thread it was inserted from. unsafe { ManuallyDrop::drop(&mut self.column); diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 0c8a54e984c41..895007831963f 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1569,7 +1569,10 @@ impl World { /// This function will panic if it isn't called from the same thread that the resource was inserted from. #[inline] pub fn get_non_send_by_id(&self, component_id: ComponentId) -> Option> { - self.storages.non_send_resources.get(component_id)?.get_data() + self.storages + .non_send_resources + .get(component_id)? + .get_data() } /// Gets a `!Send` resource to the resource with the id [`ComponentId`] if it exists. From 587ea78864ff162ec9de40ab6a805b27279a8653 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 7 Dec 2022 21:51:05 -0800 Subject: [PATCH 43/48] Add test for distinct storage and missing panic comments --- crates/bevy_ecs/src/lib.rs | 9 +++++++++ crates/bevy_ecs/src/world/mod.rs | 6 ++++++ 2 files changed, 15 insertions(+) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 33db47cf1b604..30e6e39916a65 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -1182,6 +1182,15 @@ mod tests { assert_eq!(*world.non_send_resource_mut::(), 456); } + #[test] + fn non_send_resource_points_to_distinct_data() { + let mut world = World::default(); + world.insert_resource(A(123)); + world.insert_non_send_resource(A(456)); + assert_eq!(*world.resource::(), A(123)); + assert_eq!(*world.non_send_resource::(), A(456)); + } + #[test] #[should_panic] fn non_send_resource_panic() { diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 895007831963f..7cc403aac2d6b 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1617,6 +1617,9 @@ impl World { /// /// **You should prefer to use the typed API [`World::remove_resource`] where possible and only /// use this in cases where the actual types are not known at compile time.** + /// + /// # Panics + /// This function will panic if it isn't called from the same thread that the resource was inserted from. pub fn remove_non_send_by_id(&mut self, component_id: ComponentId) -> Option<()> { self.storages .non_send_resources @@ -1630,6 +1633,9 @@ impl World { /// /// **You should prefer to use the typed API [`World::get_mut`] where possible and only /// use this in cases where the actual types are not known at compile time.** + /// + /// # Panics + /// This function will panic if it isn't called from the same thread that the resource was inserted from. #[inline] pub fn get_by_id(&self, entity: Entity, component_id: ComponentId) -> Option> { let info = self.components().get_info(component_id)?; From 68de55f6ef10b24e6b513696a991f56269243601 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 8 Dec 2022 18:52:26 -0800 Subject: [PATCH 44/48] Make the safety comments on _by_id APIs more accurate --- crates/bevy_ecs/src/world/mod.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 7cc403aac2d6b..2e48cde959f2b 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1367,8 +1367,7 @@ impl World { /// use this in cases where the actual types are not known at compile time.** /// /// # Safety - /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world - /// `component_id` must exist in this [`World`]. + /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world. #[inline] pub unsafe fn insert_resource_by_id( &mut self, @@ -1393,8 +1392,7 @@ impl World { /// thread that the original value was inserted from. /// /// # Safety - /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world - /// `component_id` must exist in this [`World`]. + /// The value referenced by `value` must be valid for the given [`ComponentId`] of this world. #[inline] pub unsafe fn insert_non_send_by_id( &mut self, From 4e8412f4feabf19597ca69d4a14afbf70af9de93 Mon Sep 17 00:00:00 2001 From: James Liu Date: Mon, 12 Dec 2022 03:17:41 -0800 Subject: [PATCH 45/48] Reword panic docs. Co-authored-by: Boxy --- crates/bevy_ecs/src/world/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 2e48cde959f2b..199857e2e2549 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -775,7 +775,7 @@ impl World { /// /// # Panics /// If a value is already present, this function will panic if called - /// from a thread that the original value was inserted from. + /// from a different thread than where the original value was inserted from. #[inline] pub fn insert_non_send_resource(&mut self, value: R) { let component_id = self.components.init_non_send::(); From 87c88ab586d8355dedc14ff6488236b1121ce2b2 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 12 Dec 2022 03:28:30 -0800 Subject: [PATCH 46/48] Fix CI --- crates/bevy_ecs/src/system/system_param.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 363a24a4c470b..b73cf99e0352b 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1021,7 +1021,6 @@ unsafe impl SystemParamState for NonSendState { world: &'w World, change_tick: u32, ) -> Self::Item<'w, 's> { - world.validate_non_send_access::(); let (ptr, ticks) = world .get_non_send_with_ticks(state.component_id) .unwrap_or_else(|| { @@ -1069,7 +1068,6 @@ unsafe impl SystemParamState for OptionNonSendState { world: &'w World, change_tick: u32, ) -> Self::Item<'w, 's> { - world.validate_non_send_access::(); world .get_non_send_with_ticks(state.0.component_id) .map(|(ptr, ticks)| NonSend { @@ -1134,7 +1132,6 @@ unsafe impl SystemParamState for NonSendMutState { world: &'w World, change_tick: u32, ) -> Self::Item<'w, 's> { - world.validate_non_send_access::(); let (ptr, ticks) = world .get_non_send_with_ticks(state.component_id) .unwrap_or_else(|| { @@ -1176,7 +1173,6 @@ unsafe impl SystemParamState for OptionNonSendMutState { world: &'w World, change_tick: u32, ) -> Self::Item<'w, 's> { - world.validate_non_send_access::(); world .get_non_send_with_ticks(state.0.component_id) .map(|(ptr, ticks)| NonSendMut { From 8b8a73520c4d995062066d8e9f049f1572cbc284 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 19 Dec 2022 11:44:41 -0800 Subject: [PATCH 47/48] Revert changes with NonSendMarker --- crates/bevy_render/src/view/window.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_render/src/view/window.rs b/crates/bevy_render/src/view/window.rs index 96cff2cdff2e5..c5b0499b08534 100644 --- a/crates/bevy_render/src/view/window.rs +++ b/crates/bevy_render/src/view/window.rs @@ -29,6 +29,7 @@ impl Plugin for WindowRenderPlugin { render_app .init_resource::() .init_resource::() + .init_non_send_resource::() .add_system_to_stage(RenderStage::Extract, extract_windows) .add_system_to_stage( RenderStage::Prepare, @@ -165,7 +166,7 @@ pub struct WindowSurfaces { pub fn prepare_windows( // By accessing a NonSend resource, we tell the scheduler to put this system on the main thread, // which is necessary for some OS s - _marker: Option>, + _marker: NonSend, mut windows: ResMut, mut window_surfaces: ResMut, render_device: Res, From 2c5be6d8dad070f50f0e6220d2739ff94d21e2e1 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 2 Jan 2023 11:43:00 -0800 Subject: [PATCH 48/48] Formatting --- crates/bevy_ecs/src/world/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 8d485e4c71e3a..701ccdc73ce80 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -15,8 +15,8 @@ use crate::{ Component, ComponentDescriptor, ComponentId, ComponentInfo, Components, TickCells, }, entity::{AllocAtWithoutReplacement, Entities, Entity, EntityLocation}, - ptr::UnsafeCellDeref, event::{Event, Events}, + ptr::UnsafeCellDeref, query::{QueryState, ReadOnlyWorldQuery, WorldQuery}, storage::{ResourceData, SparseSet, Storages}, system::Resource,