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

Improve safety for the multi-threaded executor using UnsafeWorldCell #8292

Merged
merged 33 commits into from
May 29, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Apr 2, 2023

Objective

Fix #7833.

Safety comments in the multi-threaded executor don't really talk about system world accesses, which makes it unclear if the code is actually valid.

Solution

Update the System trait to use UnsafeWorldCell. This type's API is written in a way that makes it much easier to cleanly maintain safety invariants. Use this type throughout the multi-threaded executor, with a liberal use of safety comments.


Migration Guide

The System trait now uses UnsafeWorldCell instead of &World. This type provides a robust API for interior mutable world access.

  • The method run_unsafe uses this type to manage world mutations across multiple threads.
  • The method update_archetype_component_access uses this type to ensure that only world metadata can be used.
let mut system = IntoSystem::into_system(my_system);
system.initialize(&mut world);

// Before:
system.update_archetype_component_access(&world);
unsafe { system.run_unsafe(&world) }

// After:
system.update_archetype_component_access(world.as_unsafe_world_cell_readonly());
unsafe { system.run_unsafe(world.as_unsafe_world_cell()) }

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Apr 2, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Apr 2, 2023
@JoJoJet JoJoJet added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 3, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Apr 3, 2023

Upon closer inspection, I noticed a couple more functions with undocumented invariants that had to be marked as unsafe. Also, I accidentally made update_archetype_component_access unsound, so I'm marking this PR as a draft until I resolve that.

@JoJoJet JoJoJet marked this pull request as draft April 3, 2023 14:47
@JoJoJet
Copy link
Member Author

JoJoJet commented Apr 4, 2023

I've resolved the issues I mentioned before.

@JoJoJet JoJoJet marked this pull request as ready for review April 4, 2023 19:08
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Sorry this took so long to get around to reviewing this. Generally looks good to me. Just a few nits on comments and inlining.

crates/bevy_ecs/src/system/system.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system.rs Outdated Show resolved Hide resolved
@@ -476,7 +476,7 @@ where
}

#[inline]
unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out {
unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out {
Copy link
Member

Choose a reason for hiding this comment

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

Really suprised we can elide the lifetime here.

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference is to always write UnsafeWorldCell<'_> to make it obvious this is happening.

Co-authored-by: James Liu <contact@jamessliu.com>
@JoJoJet JoJoJet requested a review from james7132 May 9, 2023 14:23
Copy link
Contributor

@jakobhellermann jakobhellermann left a comment

Choose a reason for hiding this comment

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

The safety comments look good to me.

I usually prefer to write UnsafeWorldCell<'_> to make it clear that a fresh lifetime is involved, could you make that change here as well?

@@ -476,7 +476,7 @@ where
}

#[inline]
unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out {
unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out {
Copy link
Contributor

Choose a reason for hiding this comment

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

My preference is to always write UnsafeWorldCell<'_> to make it obvious this is happening.

@JoJoJet
Copy link
Member Author

JoJoJet commented May 28, 2023

I usually prefer to write UnsafeWorldCell<'_> to make it clear that a fresh lifetime is involved, could you make that change here as well?

I can do that if you prefer it strongly, but IMO that's just an uglier way of writing the same thing. Being explicit about lifetimes only adds clarity if you're going to name them -- otherwise, elision is preferred. I don't think that UnsafeWorldCell<'_> carries any more information than UnsafeWorldCell.

Anyway, thank you for the review.

@jakobhellermann
Copy link
Contributor

It's not a strong preference, we can keep it as is.

@JoJoJet JoJoJet added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 28, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 29, 2023
Merged via the queue into bevyengine:main with commit 85a918a May 29, 2023
@JoJoJet JoJoJet deleted the system-unsafe-world-cell branch June 11, 2023 00:10
alice-i-cecile pushed a commit that referenced this pull request Jun 15, 2023
# Objective

Follow-up to #6404 and #8292.

Mutating the world through a shared reference is surprising, and it
makes the meaning of `&World` unclear: sometimes it gives read-only
access to the entire world, and sometimes it gives interior mutable
access to only part of it.

This is an up-to-date version of #6972.

## Solution

Use `UnsafeWorldCell` for all interior mutability. Now, `&World`
*always* gives you read-only access to the entire world.

---

## Changelog

TODO - do we still care about changelogs?

## Migration Guide

Mutating any world data using `&World` is now considered unsound -- the
type `UnsafeWorldCell` must be used to achieve interior mutability. The
following methods now accept `UnsafeWorldCell` instead of `&World`:

- `QueryState`: `get_unchecked`, `iter_unchecked`,
`iter_combinations_unchecked`, `for_each_unchecked`,
`get_single_unchecked`, `get_single_unchecked_manual`.
- `SystemState`: `get_unchecked_manual`

```rust
let mut world = World::new();
let mut query = world.query::<&mut T>();

// Before:
let t1 = query.get_unchecked(&world, entity_1);
let t2 = query.get_unchecked(&world, entity_2);

// After:
let world_cell = world.as_unsafe_world_cell();
let t1 = query.get_unchecked(world_cell, entity_1);
let t2 = query.get_unchecked(world_cell, entity_2);
```

The methods `QueryState::validate_world` and
`SystemState::matches_world` now take a `WorldId` instead of `&World`:

```rust
// Before:
query_state.validate_world(&world);

// After:
query_state.validate_world(world.id());
```

The methods `QueryState::update_archetypes` and
`SystemState::update_archetypes` now take `UnsafeWorldCell` instead of
`&World`:

```rust
// Before:
query_state.update_archetypes(&world);

// After:
query_state.update_archetypes(world.as_unsafe_world_cell_readonly());
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Safety comments in MultiThreadedExecutor are incorrect
4 participants