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

[Merged by Bors] - Reduce the use of atomics in the render phase #7084

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
b411da6
Flatten EntityPhaseItem into PhaseItem
james7132 Dec 7, 2022
702b2c4
Add WorldQuery associated types to RenderCommand
james7132 Dec 7, 2022
2c099b1
Pass WorldQuery into RenderCommand
james7132 Dec 8, 2022
8e4f9ff
Flatten EntityRenderCommand out
james7132 Dec 8, 2022
45f227a
Cleanup
james7132 Dec 8, 2022
03d6ca3
Fix CI
james7132 Dec 8, 2022
22fd3c7
Use QueryState::get_manual over get
james7132 Dec 8, 2022
4a5638f
Unwrap instead of return
james7132 Dec 8, 2022
1a4083a
Merge branch 'main' into flatten-render-commands
james7132 Dec 12, 2022
5efcff3
Formatting
james7132 Dec 12, 2022
54cd232
Merge branch 'main' into flatten-render-commands
james7132 Dec 14, 2022
1756709
Remove spurious files
james7132 Dec 14, 2022
aa146b3
Fix CI
james7132 Dec 14, 2022
68f4a36
Merge branch 'main' into flatten-render-commands
james7132 Dec 25, 2022
4d4d9b4
Update Draw::prepare documentation
james7132 Dec 25, 2022
c09c358
WorldQuery -> ItemWorldQUery
james7132 Dec 25, 2022
f63f670
Fix CI
james7132 Dec 26, 2022
d034b09
Merge branch 'main' into flatten-render-commands
james7132 Jan 3, 2023
0a84aaa
Fix CI
james7132 Jan 3, 2023
592dd8e
Add SystemState::get_manual(_mut)
james7132 Jan 3, 2023
ef78c5b
Merge branch 'main' into less-atomic-render-commands
james7132 Jan 4, 2023
0a79eec
Add doc comment for `validate_world`
james7132 Jan 9, 2023
266583c
Merge branch 'main' into less-atomic-render-commands
james7132 Jan 17, 2023
84a2d7f
Merge branch 'less-atomic-render-commands' of github.com:james7132/be…
james7132 Jan 17, 2023
b90341e
Improve wording on get_manual variant's docs
james7132 Jan 17, 2023
ce70df9
Document update_archetypes
james7132 Jan 17, 2023
15b8527
Formatting
james7132 Jan 17, 2023
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
70 changes: 67 additions & 3 deletions crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,15 +170,17 @@ impl<Param: SystemParam> SystemState<Param> {
where
Param: ReadOnlySystemParam,
{
self.validate_world_and_update_archetypes(world);
self.validate_world(world);
self.update_archetypes(world);
// SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with.
unsafe { self.get_unchecked_manual(world) }
}

/// Retrieve the mutable [`SystemParam`] values.
#[inline]
pub fn get_mut<'w, 's>(&'s mut self, world: &'w mut World) -> SystemParamItem<'w, 's, Param> {
self.validate_world_and_update_archetypes(world);
self.validate_world(world);
self.update_archetypes(world);
// SAFETY: World is uniquely borrowed and matches the World this SystemState was created with.
unsafe { self.get_unchecked_manual(world) }
}
Expand All @@ -196,8 +198,20 @@ impl<Param: SystemParam> SystemState<Param> {
self.world_id == world.id()
}

fn validate_world_and_update_archetypes(&mut self, world: &World) {
/// Asserts that the [`SystemState`] matches the provided [`World`].
#[inline]
fn validate_world(&self, world: &World) {
assert!(self.matches_world(world), "Encountered a mismatched World. A SystemState cannot be used with Worlds other than the one it was created with.");
}

/// Updates the state's internal view of the `world`'s archetypes. If this is not called before fetching the parameters,
/// the results may not accurately reflect what is in the `world`.
///
/// This is only required if [`SystemState::get_manual`] or [`SystemState::get_manual_mut`] is being called, and it only needs to
/// be called if the `world` has been structurally mutated (i.e. added/removed a component or resource). Users using
/// [`SystemState::get`] or [`SystemState::get_mut`] do not need to call this as it will be automatically called for them.
#[inline]
pub fn update_archetypes(&mut self, world: &World) {
james7132 marked this conversation as resolved.
Show resolved Hide resolved
let archetypes = world.archetypes();
let new_generation = archetypes.generation();
let old_generation = std::mem::replace(&mut self.archetype_generation, new_generation);
Expand All @@ -212,6 +226,43 @@ impl<Param: SystemParam> SystemState<Param> {
}
}

/// Retrieve the [`SystemParam`] values. This can only be called when all parameters are read-only.
/// This will not update the state's view of the world's archetypes automatically nor increment the
/// world's change tick.
///
/// For this to return accurate results, ensure [`SystemState::update_archetypes`] is called before this
/// function.
Comment on lines +233 to +234
Copy link
Member

Choose a reason for hiding this comment

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

In what way will the results be inaccurate? Is it just that any newly-added archetypes will be skipped? Not suggesting a change necessarily, just asking why this function is safe.

Copy link
Member Author

@james7132 james7132 Jan 17, 2023

Choose a reason for hiding this comment

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

New archetypes are ignored. Queries, for example, will just not return the target entity in those new archetypes. The prior archetypes are still accessed correctly, so this shouldn't cause any UB if not updated.

///
/// Users should strongly prefer to use [`SystemState::get`] over this function.
#[inline]
pub fn get_manual<'w, 's>(&'s mut self, world: &'w World) -> SystemParamItem<'w, 's, Param>
where
Param: ReadOnlySystemParam,
{
self.validate_world(world);
let change_tick = world.read_change_tick();
// SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with.
unsafe { self.fetch(world, change_tick) }
}

/// Retrieve the mutable [`SystemParam`] values. This will not update the state's view of the world's archetypes
/// automatically nor increment the world's change tick.
///
/// For this to return accurate results, ensure [`SystemState::update_archetypes`] is called before this
/// function.
///
/// Users should strongly prefer to use [`SystemState::get_mut`] over this function.
#[inline]
pub fn get_manual_mut<'w, 's>(
&'s mut self,
world: &'w mut World,
) -> SystemParamItem<'w, 's, Param> {
self.validate_world(world);
let change_tick = world.change_tick();
// SAFETY: World is uniquely borrowed and matches the World this SystemState was created with.
unsafe { self.fetch(world, change_tick) }
}

/// Retrieve the [`SystemParam`] values. This will not update archetypes automatically.
///
/// # Safety
Expand All @@ -224,6 +275,19 @@ impl<Param: SystemParam> SystemState<Param> {
world: &'w World,
) -> SystemParamItem<'w, 's, Param> {
let change_tick = world.increment_change_tick();
self.fetch(world, change_tick)
}

/// # Safety
/// This call might access any of the input parameters in a way that violates Rust's mutability rules. Make sure the data
/// access is safe in the context of global [`World`] access. The passed-in [`World`] _must_ be the [`World`] the [`SystemState`] was
/// created with.
#[inline]
unsafe fn fetch<'w, 's>(
&'s mut self,
world: &'w World,
change_tick: u32,
) -> SystemParamItem<'w, 's, Param> {
let param = Param::get_param(&mut self.param_state, &self.meta, world, change_tick);
self.meta.last_change_tick = change_tick;
param
Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_render/src/render_phase/draw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ where
/// Prepares the render command to be used. This is called once and only once before the phase
/// begins. There may be zero or more `draw` calls following a call to this function.
fn prepare(&mut self, world: &'_ World) {
self.state.update_archetypes(world);
self.view.update_archetypes(world);
self.entity.update_archetypes(world);
}
Expand All @@ -256,7 +257,7 @@ where
view: Entity,
item: &P,
) {
let param = self.state.get(world);
let param = self.state.get_manual(world);
let view = self.view.get_manual(world, view).unwrap();
let entity = self.entity.get_manual(world, item.entity()).unwrap();
// TODO: handle/log `RenderCommand` failure
Expand Down