Skip to content

Commit

Permalink
Remove Sync bound from Local (bevyengine#5483)
Browse files Browse the repository at this point in the history
# Objective

Currently, `Local` has a `Sync` bound. Theoretically this is unnecessary as a local can only ever be accessed from its own system, ensuring exclusive access on one thread. This PR removes this restriction.

## Solution

- By removing the `Resource` bound from `Local` and adding the new `SyncCell` threading primative, `Local` can have the `Sync` bound removed.

## Changelog

### Added

- Added `SyncCell` to `bevy_utils`

### Changed

- Removed `Resource` bound from `Local`
- `Local` is now wrapped in a `SyncCell`

## Migration Guide

- Any code relying on `Local<T>` having `T: Resource` may have to be changed, but this is unlikely.

Co-authored-by: PROMETHIA-27 <42193387+PROMETHIA-27@users.noreply.github.com>
  • Loading branch information
2 people authored and nicopap committed Sep 12, 2022
1 parent 6ac5800 commit d144763
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 8 deletions.
17 changes: 9 additions & 8 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub use bevy_ecs_macros::Resource;
pub use bevy_ecs_macros::SystemParam;
use bevy_ecs_macros::{all_tuples, impl_param_set};
use bevy_ptr::UnsafeCellDeref;
use bevy_utils::synccell::SyncCell;
use std::{
borrow::Cow,
fmt::Debug,
Expand Down Expand Up @@ -674,10 +675,10 @@ impl<'w, 's> SystemParamFetch<'w, 's> for WorldState {
/// // .add_system(reset_to_system(my_config))
/// # assert_is_system(reset_to_system(Config(10)));
/// ```
pub struct Local<'a, T: FromWorld + Send + Sync + 'static>(&'a mut T);
pub struct Local<'a, T: FromWorld + Send + 'static>(&'a mut T);

// SAFETY: Local only accesses internal state
unsafe impl<T: Send + Sync + 'static> ReadOnlySystemParamFetch for LocalState<T> {}
unsafe impl<T: Send + 'static> ReadOnlySystemParamFetch for LocalState<T> {}

impl<'a, T: FromWorld + Send + Sync + 'static> Debug for Local<'a, T>
where
Expand Down Expand Up @@ -706,20 +707,20 @@ impl<'a, T: FromWorld + Send + Sync + 'static> DerefMut for Local<'a, T> {

/// The [`SystemParamState`] of [`Local<T>`].
#[doc(hidden)]
pub struct LocalState<T: Send + Sync + 'static>(T);
pub struct LocalState<T: Send + 'static>(SyncCell<T>);

impl<'a, T: Send + Sync + 'static + FromWorld> SystemParam for Local<'a, T> {
impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> {
type Fetch = LocalState<T>;
}

// SAFETY: only local state is accessed
unsafe impl<T: FromWorld + Send + Sync + 'static> SystemParamState for LocalState<T> {
unsafe impl<T: FromWorld + Send + 'static> SystemParamState for LocalState<T> {
fn init(world: &mut World, _system_meta: &mut SystemMeta) -> Self {
Self(T::from_world(world))
Self(SyncCell::new(T::from_world(world)))
}
}

impl<'w, 's, T: Send + Sync + 'static + FromWorld> SystemParamFetch<'w, 's> for LocalState<T> {
impl<'w, 's, T: FromWorld + Send + 'static> SystemParamFetch<'w, 's> for LocalState<T> {
type Item = Local<'s, T>;

#[inline]
Expand All @@ -729,7 +730,7 @@ impl<'w, 's, T: Send + Sync + 'static + FromWorld> SystemParamFetch<'w, 's> for
_world: &'w World,
_change_tick: u32,
) -> Self::Item {
Local(&mut state.0)
Local(state.0.get())
}
}

Expand Down
1 change: 1 addition & 0 deletions crates/bevy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub mod futures;
pub mod label;
mod short_names;
pub use short_names::get_short_name;
pub mod synccell;

mod default;
mod float_ord;
Expand Down
39 changes: 39 additions & 0 deletions crates/bevy_utils/src/synccell.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/// See [`Exclusive`](https://github.com/rust-lang/rust/issues/98407) for stdlib's upcoming implementation,
/// which should replace this one entirely.
///
/// Provides a wrapper that allows making any type unconditionally [`Sync`] by only providing mutable access.
#[repr(transparent)]
pub struct SyncCell<T: ?Sized> {
inner: T,
}

impl<T: Sized> SyncCell<T> {
/// Construct a new instance of a `SyncCell` from the given value.
pub fn new(inner: T) -> Self {
Self { inner }
}

/// Deconstruct this `SyncCell` into its inner value.
pub fn to_inner(Self { inner }: Self) -> T {
inner
}
}

impl<T: ?Sized> SyncCell<T> {
/// Get a reference to this `SyncCell`'s inner value.
pub fn get(&mut self) -> &mut T {
&mut self.inner
}

/// Build a mutable reference to a `SyncCell` from a mutable reference
/// to its inner value, to skip constructing with [`new()`](SyncCell::new()).
pub fn from_mut(r: &'_ mut T) -> &'_ mut SyncCell<T> {
// SAFETY: repr is transparent, so refs have the same layout; and `SyncCell` properties are `&mut`-agnostic
unsafe { &mut *(r as *mut T as *mut SyncCell<T>) }
}
}

// SAFETY: `Sync` only allows multithreaded access via immutable reference.
// As `SyncCell` requires an exclusive reference to access the wrapped value,
// marking this type as `Sync` does not actually allow threaded access to the inner value.
unsafe impl<T: ?Sized> Sync for SyncCell<T> {}

0 comments on commit d144763

Please sign in to comment.