Relax Resource: Sync bound

[JoJoJet]

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]

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.)

[JoJoJet]

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

[james7132]

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.

[JoJoJet]

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.

[james7132]

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.

[JoJoJet]

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.

[JoJoJet]

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.

[JoJoJet]

This is no longer blocked by #6603.

[JoJoJet]

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]

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]

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

[james7132]

.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>.