Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Remove Sync bound on Component and Resource #4680

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/bevy_ecs/macros/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
ast.generics
.make_where_clause()
.predicates
.push(parse_quote! { Self: Send + Sync + 'static });
.push(parse_quote! { Self: Send + 'static });

let struct_name = &ast.ident;
let (impl_generics, type_generics, where_clause) = &ast.generics.split_for_impl();
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ use std::{any::TypeId, collections::HashMap};
/// bundle, in the _exact_ order that [`Bundle::get_components`] is called.
/// - [`Bundle::from_components`] must call `func` exactly once for each [`ComponentId`] returned by
/// [`Bundle::component_ids`].
pub unsafe trait Bundle: Send + Sync + 'static {
pub unsafe trait Bundle: Send + 'static {
/// Gets this [`Bundle`]'s component ids, in the order of this bundle's [`Component`]s
fn component_ids(components: &mut Components, storages: &mut Storages) -> Vec<ComponentId>;

Expand Down
21 changes: 11 additions & 10 deletions crates/bevy_ecs/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use std::{
/// A component is data associated with an [`Entity`](crate::entity::Entity). Each entity can have
/// multiple different types of components, but only one of them per type.
///
/// Any type that is `Send + Sync + 'static` can implement `Component` using `#[derive(Component)]`.
/// Any type that is `Send + 'static` can implement `Component` using `#[derive(Component)]`.
///
/// In order to use foreign types as components, wrap them using a newtype pattern.
/// ```
Expand All @@ -32,7 +32,7 @@ use std::{
/// as one of the arguments.
///
/// Components can be grouped together into a [`Bundle`](crate::bundle::Bundle).
pub trait Component: Send + Sync + 'static {
pub trait Component: Send + 'static {
type Storage: ComponentStorage;
}

Expand Down Expand Up @@ -121,8 +121,8 @@ impl ComponentInfo {
}

#[inline]
pub fn is_send_and_sync(&self) -> bool {
self.descriptor.is_send_and_sync
pub fn is_send(&self) -> bool {
self.descriptor.is_send
}

fn new(id: ComponentId, descriptor: ComponentDescriptor) -> Self {
Expand Down Expand Up @@ -162,8 +162,9 @@ pub struct ComponentDescriptor {
// associated rust component type if one exists.
storage_type: StorageType,
// SAFETY: This must remain private. It must only be set to "true" if this component is
// actually Send + Sync
is_send_and_sync: bool,
// actually Send
// TODO: Add is_sync to this when applicable.
is_send: bool,
type_id: Option<TypeId>,
layout: Layout,
// SAFETY: this function must be safe to call with pointers pointing to items of the type
Expand All @@ -177,7 +178,7 @@ impl std::fmt::Debug for ComponentDescriptor {
f.debug_struct("ComponentDescriptor")
.field("name", &self.name)
.field("storage_type", &self.storage_type)
.field("is_send_and_sync", &self.is_send_and_sync)
.field("is_send", &self.is_send)
.field("type_id", &self.type_id)
.field("layout", &self.layout)
.finish()
Expand All @@ -194,7 +195,7 @@ impl ComponentDescriptor {
Self {
name: std::any::type_name::<T>().to_string(),
storage_type: T::Storage::STORAGE_TYPE,
is_send_and_sync: true,
is_send: true,
type_id: Some(TypeId::of::<T>()),
layout: Layout::new::<T>(),
drop: Self::drop_ptr::<T>,
Expand All @@ -210,7 +211,7 @@ impl ComponentDescriptor {
// PERF: `SparseStorage` may actually be a more
// reasonable choice as `storage_type` for resources.
storage_type: StorageType::Table,
is_send_and_sync: true,
is_send: true,
type_id: Some(TypeId::of::<T>()),
layout: Layout::new::<T>(),
drop: Self::drop_ptr::<T>,
Expand All @@ -221,7 +222,7 @@ impl ComponentDescriptor {
Self {
name: std::any::type_name::<T>().to_string(),
storage_type,
is_send_and_sync: false,
is_send: false,
type_id: Some(TypeId::of::<T>()),
layout: Layout::new::<T>(),
drop: Self::drop_ptr::<T>,
Expand Down
41 changes: 33 additions & 8 deletions crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,31 @@ use std::{cell::UnsafeCell, marker::PhantomData};
///
/// # bevy_ecs::system::assert_is_system(my_system);
/// ```
///
/// ## Non-Sync Queries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs are great; add a copy of this to Res and NonSend?

We should also try to deduplicate these docs with the include_str! doc macro trick; these are subtle and critical enough that I really don't want that duplication.

///
/// Non-`Sync` components are supported, but cannot be accessed as `&T`,
/// as the type cannot be safely read from multiple threads at the same
/// time. These components must be accessed via a `&mut T` to guarentee
/// that the system has exclusive access to the components.
///
/// ```compile_fail,E0277
/// # use bevy_ecs::prelude::*;
/// # use std::cell::Cell;
///
/// #[derive(Component)]
/// struct NotSync(Cell<usize>);
///
/// // This will not compile!
/// fn my_system(query: Query<&NotSync>) {
/// for _ in query.iter() {}
/// }
///
/// // This will compile!
/// fn my_system_mut(query: Query<&mut NotSync>) {
/// for _ in query.iter() {}
/// }
/// ```
pub trait WorldQuery: for<'w> WorldQueryGats<'w, _State = Self::State> {
type State: FetchState;

Expand Down Expand Up @@ -541,7 +566,7 @@ impl<'w> Fetch<'w> for EntityFetch<'w> {
}
}

impl<T: Component> WorldQuery for &T {
impl<T: Component + Sync> WorldQuery for &T {
type State = ReadState<T>;

fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self> {
Expand All @@ -553,12 +578,12 @@ impl<T: Component> WorldQuery for &T {
#[doc(hidden)]
pub struct ReadState<T> {
component_id: ComponentId,
marker: PhantomData<T>,
marker: PhantomData<fn() -> T>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This aligns with my mental model of how this should be modelled, but this is subtle enough that it deserves a comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't really think it's needed. This is very much the standard way to express 'this represents something which can produce a T'.

It's no more strange than the initial use of PhantomData imo - maybe we should have a comment that we need to be able to 'reconstruct' the original T, so store it here. But we don't need to comment specifically on it being fn()->T

}

// SAFETY: component access and archetype component access are properly updated to reflect that T is
// read
unsafe impl<T: Component> FetchState for ReadState<T> {
unsafe impl<T: Component + Sync> FetchState for ReadState<T> {
fn init(world: &mut World) -> Self {
let component_id = world.init_component::<T>();
ReadState {
Expand Down Expand Up @@ -620,15 +645,15 @@ impl<T> Clone for ReadFetch<'_, T> {
}

/// SAFETY: access is read only
unsafe impl<'w, T: Component> ReadOnlyFetch for ReadFetch<'w, T> {}
unsafe impl<'w, T: Component + Sync> ReadOnlyFetch for ReadFetch<'w, T> {}

impl<'w, T: Component> WorldQueryGats<'w> for &T {
impl<'w, T: Component + Sync> WorldQueryGats<'w> for &T {
type Fetch = ReadFetch<'w, T>;
type ReadOnlyFetch = ReadFetch<'w, T>;
type _State = ReadState<T>;
}

impl<'w, T: Component> Fetch<'w> for ReadFetch<'w, T> {
impl<'w, T: Component + Sync> Fetch<'w> for ReadFetch<'w, T> {
type Item = &'w T;
type State = ReadState<T>;

Expand Down Expand Up @@ -790,7 +815,7 @@ impl<T> Clone for ReadOnlyWriteFetch<'_, T> {
#[doc(hidden)]
pub struct WriteState<T> {
component_id: ComponentId,
marker: PhantomData<T>,
marker: PhantomData<fn() -> T>,
}

// SAFETY: component access and archetype component access are properly updated to reflect that T is
Expand Down Expand Up @@ -1256,7 +1281,7 @@ impl<T: Component> WorldQuery for ChangeTrackers<T> {
#[doc(hidden)]
pub struct ChangeTrackersState<T> {
component_id: ComponentId,
marker: PhantomData<T>,
marker: PhantomData<fn() -> T>,
}

// SAFETY: component access and archetype component access are properly updated to reflect that T is
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/query/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub struct WithFetch<T> {
#[doc(hidden)]
pub struct WithState<T> {
component_id: ComponentId,
marker: PhantomData<T>,
marker: PhantomData<fn() -> T>,
}

// SAFETY: no component access or archetype component access
Expand Down Expand Up @@ -207,7 +207,7 @@ pub struct WithoutFetch<T> {
#[doc(hidden)]
pub struct WithoutState<T> {
component_id: ComponentId,
marker: PhantomData<T>,
marker: PhantomData<fn() -> T>,
}

// SAFETY: no component access or archetype component access
Expand Down Expand Up @@ -497,7 +497,7 @@ macro_rules! impl_tick_filter {
$(#[$state_meta])*
pub struct $state_name<T> {
component_id: ComponentId,
marker: PhantomData<T>,
marker: PhantomData<fn() -> T>,
}

impl<T: Component> WorldQuery for $name<T> {
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/schedule/stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use std::fmt::Debug;
use super::IntoSystemDescriptor;

/// A type that can run as a step of a [`Schedule`](super::Schedule).
pub trait Stage: Downcast + Send + Sync {
pub trait Stage: Downcast + Send {
/// Runs the stage; this happens once per update.
/// Implementors must initialize all of their state and systems before running the first time.
fn run(&mut self, world: &mut World);
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_ecs/src/system/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::marker::PhantomData;
use super::Resource;

/// A [`World`] mutation.
pub trait Command: Send + Sync + 'static {
pub trait Command: Send + 'static {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll also want to do the same transformation for Send at some point.

Note that doing that requires we keep proper main thread management - it requires a lot of care.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the Send + Sync necessary when the target is wasm?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually we'll want Web Worker-based multithreading, so it will still be required.

fn write(self, world: &mut World);
}

Expand Down Expand Up @@ -261,7 +261,7 @@ impl<'w, 's> Commands<'w, 's> {
/// worked out to share an ID space (which doesn't happen by default).
pub fn insert_or_spawn_batch<I, B>(&mut self, bundles_iter: I)
where
I: IntoIterator + Send + Sync + 'static,
I: IntoIterator + Send + 'static,
I::IntoIter: Iterator<Item = (Entity, B)>,
B: Bundle,
{
Expand Down Expand Up @@ -640,7 +640,7 @@ where

pub struct InsertOrSpawnBatch<I, B>
where
I: IntoIterator + Send + Sync + 'static,
I: IntoIterator + Send + 'static,
B: Bundle,
I::IntoIter: Iterator<Item = (Entity, B)>,
{
Expand All @@ -649,7 +649,7 @@ where

impl<I, B> Command for InsertOrSpawnBatch<I, B>
where
I: IntoIterator + Send + Sync + 'static,
I: IntoIterator + Send + 'static,
B: Bundle,
I::IntoIter: Iterator<Item = (Entity, B)>,
{
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/exclusive_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
};
use std::borrow::Cow;

pub trait ExclusiveSystem: Send + Sync + 'static {
pub trait ExclusiveSystem: Send + 'static {
fn name(&self) -> Cow<'static, str>;

fn run(&mut self, world: &mut World);
Expand Down
5 changes: 4 additions & 1 deletion crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,10 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> {
/// # bevy_ecs::system::assert_is_system(print_selected_character_name_system);
/// ```
#[inline]
pub fn get_component<T: Component>(&self, entity: Entity) -> Result<&T, QueryComponentError> {
pub fn get_component<T: Component + Sync>(
&self,
entity: Entity,
) -> Result<&T, QueryComponentError> {
let world = self.world;
let entity_ref = world
.get_entity(entity)
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::borrow::Cow;
/// Systems are executed in parallel, in opportunistic order; data access is managed automatically.
/// It's possible to specify explicit execution order between specific systems,
/// see [`SystemDescriptor`](crate::schedule::SystemDescriptor).
pub trait System: Send + Sync + 'static {
pub trait System: Send + 'static {
/// The system's input. See [`In`](crate::system::In) for
/// [`FunctionSystem`](crate::system::FunctionSystem)s.
type In;
Expand Down
Loading