Relax `Resource: Sync` bound by joseph-gio · Pull Request #6572 · bevyengine/bevy

joseph-gio · 2022-11-13T00:40:21Z

Objective

Allow non-Sync types to be used as resources, without having to resort to NonSend<>.

Currently, non-Sync types are conflated with non-Send types, which results in overly restrictive scheduling. Any system that uses non-Send types must be run on the main thread, which increases thread contention and prevents parallelization. This is unnecessary, as Send + !Sync resources could be accessed from any thread, as long as it's only one at a time.

Solution

Relax the bound Resource: Sync.
Resource trait tracks whether each type is Sync. The scheduler ensures that shared access to non-Sync types does not occur.

Example

// This type can't be shared between threads, but it *can* be sent.
#[derive(Resource, Deref)]
#[resource(is_sync = false)] // compile fails if you forget this
pub struct MyResource(std::cell::Cell<u32>);

fn my_system(x: Res<MyResource>) {
    x.set(x.get() + 1);
}

Future Work

For the time being, systems with read-only access to a non-Sync resource are run on the main thread. This is overly restrictive: we will need to update our Access model to properly support non-Sync reads.

Changelog

Resource now supports types that do not implement std::marker::Sync.

Migration Guide

The Resource trait is now unsafe to implement manually. Use of the derive macro is recommended.

// Before
struct MyResource(...);
impl Resource for MyResource {}

// After
struct MyResource(...);
unsafe impl Resource for MyResource {
    // Undefined Behavior if this is wrong. 
    const IS_SYNC: bool = true;
}

Any generic code that relied on Resource: Sync will now need to specify the Sync bound.

// Before:
fn my_system<T: Resource>(
    my_resource: Res<T>,
    q: Query<...>,
) {
    // Since we access `my_resource` across threads (because of par_for_each), we need it to be `Sync`.
    q.par_for_each(|...| {
        do_something_with(&my_resource);
    })
}

// After:
fn my_system<T: Resource + Sync>( // Just add this bound.
    ...
) {
    ...
}

maniwani · 2022-11-13T19:52:01Z

Can we avoid having to specify the Sync bound if Access knows which ComponentId or ArchetypeComponentId are Sync?

(Edit: Nevermind, I missed the "Future Work" section.)

joseph-gio · 2022-11-15T01:20:19Z

Should be ready for review now. Note that this PR is blocked by #6603 .

crates/bevy_tasks/src/task_pool.rs

crates/bevy_transform/src/systems.rs

james7132 · 2022-11-15T02:22:33Z

crates/bevy_ecs/src/system/system_param.rs

    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self {
+        // If the resource can't be shared between threads, make sure it runs on the main thread.
+        // FIXME: We should be able to register access as 'exclusive reads' to prevent shared access
+        // without forcing the system to run on the main thread.


IMO, there's no point to merging this PR as is without this. You would otherwise just use a NonSend for the same effect.

You can treat a Res<T: !Sync> the same as ResMut<T: !Sync>. Move the if !T::IS_SYNC down below, and change the non-Sync branch to add a write instead.

IMO, there's no point to merging this PR as is without this. You would otherwise just use a NonSend for the same effect.

I want to keep this PR as small as possible, so it's easier to review. If this gets merged, I'll open a follow-up PR that makes the Access changes.

These changes are easily separable, so I see no reason to roll them up into one PR. Especially since the Access changes are just an optimization.

Also, this already allows ResMut<T: !Sync> to be used off of the main thread.

You can treat a Res<T: !Sync> the same as ResMut<T: !Sync>. Move the if !T::IS_SYNC down below, and change the non-Sync branch to add a write instead.

This won't work, it will mess up ambiguity detection.

crates/bevy_ecs/src/world/mod.rs

james7132 · 2022-11-15T02:36:48Z

crates/bevy_ecs/macros/src/component.rs

-        impl #impl_generics #bevy_ecs_path::system::Resource for #struct_name #type_generics #where_clause {
+        // SAFETY: The trait bounds on this impl guarantee that the non-`Sync` types will not have `IS_SYNC = true`.
+        unsafe impl #impl_generics #bevy_ecs_path::system::Resource for #struct_name #type_generics #where_clause {
+            const IS_SYNC: bool = #is_sync;


These should definitely have some compile fail tests to ensure that this breaks when this contract is broken.

Separately, we should see if the compile failures are readable by normal users.

I added a compile fail test. ~~The error message isn't great~~, but it isn't any worse than before this PR.

I tried a few ways of providing a more descriptive error message, but they all caused problems in edge cases.

After investigating further, the error message is actually good for concrete types. It shows up when you define the struct, and points you directly to the field that makes the struct !Sync.

It's less good for generic types, because the error doesn't occur until you try to use the resource.

// Concrete types: #[derive(Resource)] // error message shows up here, right where we want it. struct MyResource(Cell<u32>);

// Generic types: #[derive(Resource)] struct MyResource<T>(Cell<T>); // The error message doesn't show up until here, which is un-ideal. fn my_system(_: Res<MyResource<u32>>) {}

IMO this isn't perfect, but it's perfectly acceptable.

This reverts commit 47a144a.

joseph-gio · 2022-11-21T16:51:31Z

This is no longer blocked by #6603.

joseph-gio · 2022-12-05T23:17:39Z

I'm closing this in favor of #6864 -- this all seems a bit unnecessary when you could just wrap your !Sync types in a SyncCell.

james7132 · 2022-12-07T02:58:23Z

For what it's worth, I do think !Sync components by themselves are valuable. SyncCell forces mutable access which may force users into triggering change detection, even when it's really just for read-only access. This is bypasssable, but it's not exactly great UX.

hymm · 2022-12-07T05:29:05Z

could we create our own wrapper that would only trigger change detection on mutable access to the inner value?

james7132 · 2022-12-07T21:20:52Z

.We'd need to be aware that the type is !Sync in some way. Not sure how to do that in a workable way in Mut<T>.

# Objective It's not clear to users how to handle `!Sync` types as components and resources in the absence of engine level support for them. ## Solution Added a section to `Component`'s and `Resource`'s type level docs on available options for making a type `Sync` when it holds `!Sync` fields, linking `bevy_utils::synccell::SyncCell` and the currently unstable `std::sync::Exclusive`. Also added a compile_fail doctest that illustrates how to apply `SyncCell`. These will break when/if #6572 gets merged, at which point these docs should be updated.

…ine#6864) # Objective It's not clear to users how to handle `!Sync` types as components and resources in the absence of engine level support for them. ## Solution Added a section to `Component`'s and `Resource`'s type level docs on available options for making a type `Sync` when it holds `!Sync` fields, linking `bevy_utils::synccell::SyncCell` and the currently unstable `std::sync::Exclusive`. Also added a compile_fail doctest that illustrates how to apply `SyncCell`. These will break when/if bevyengine#6572 gets merged, at which point these docs should be updated.

BoxyUwU self-requested a review November 13, 2022 01:39

joseph-gio marked this pull request as ready for review November 15, 2022 01:19

james7132 requested changes Nov 15, 2022

View reviewed changes

joseph-gio added 12 commits November 21, 2022 11:26

Allow resources to be non-Sync

7759ef3

update derive macro

6eb006c

make EventLoopProxy a non-sync Resource

88b3aff

allow non-Sync extract resources

8da746e

Revert "make EventLoopProxy a non-sync Resource"

b620431

This reverts commit 47a144a.

add tests for non-Sync resources

bb738c0

split ComponentDescriptor::is_send_and_sync

2a89a71

add more systems to a test

13aaaa3

improve a comment

f33c147

use a Mutex instead of a RwLock

de1ee4c

Send -> Sync

57e0ca5

remove a flawed test

c3b681c

joseph-gio force-pushed the non-sync-resource branch from 4afed44 to c3b681c Compare November 21, 2022 16:27

add a compile_fail test

c24083a

james7132 mentioned this pull request Dec 5, 2022

[Merged by Bors] - Document options for !Sync types for Component and Resources #6864

Closed

joseph-gio closed this Dec 5, 2022

joseph-gio deleted the non-sync-resource branch September 20, 2023 05:20

Uh oh!

Conversation

joseph-gio commented Nov 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Example

Future Work

Changelog

Migration Guide

Uh oh!

maniwani commented Nov 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseph-gio commented Nov 15, 2022

Uh oh!

Uh oh!

Uh oh!

james7132 Nov 15, 2022

Choose a reason for hiding this comment

Uh oh!

joseph-gio Nov 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

james7132 Nov 15, 2022

Choose a reason for hiding this comment

Uh oh!

joseph-gio Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joseph-gio Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

joseph-gio commented Nov 21, 2022

Uh oh!

joseph-gio commented Dec 5, 2022

Uh oh!

james7132 commented Dec 7, 2022

Uh oh!

hymm commented Dec 7, 2022

Uh oh!

james7132 commented Dec 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joseph-gio commented Nov 13, 2022 •

edited

Loading

maniwani commented Nov 13, 2022 •

edited

Loading

joseph-gio Nov 21, 2022 •

edited

Loading