Skip to content

Optimize param validation through get_param(...) -> Option<Out> #15606

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

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions benches/benches/bevy_ecs/fragmentation/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ fn iter_frag_empty(c: &mut Criterion) {
spawn_empty_frag_archetype::<Table>(&mut world);
let mut q: SystemState<Query<(Entity, &Table)>> =
SystemState::<Query<(Entity, &Table<0>)>>::new(&mut world);
let query = q.get(&world);
let query = q.get(&world).unwrap();
b.iter(move || {
let mut res = 0;
query.iter().for_each(|(e, t)| {
Expand All @@ -39,7 +39,7 @@ fn iter_frag_empty(c: &mut Criterion) {
spawn_empty_frag_archetype::<Sparse>(&mut world);
let mut q: SystemState<Query<(Entity, &Sparse)>> =
SystemState::<Query<(Entity, &Sparse<0>)>>::new(&mut world);
let query = q.get(&world);
let query = q.get(&world).unwrap();
b.iter(move || {
let mut res = 0;
query.iter().for_each(|(e, t)| {
Expand Down
8 changes: 4 additions & 4 deletions benches/benches/bevy_ecs/world/world_get.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ pub fn query_get(criterion: &mut Criterion) {
.collect();
entities.shuffle(&mut deterministic_rand());
let mut query = SystemState::<Query<&Table>>::new(&mut world);
let query = query.get(&world);
let query = query.get(&world).unwrap();

bencher.iter(|| {
let mut count = 0;
Expand All @@ -288,7 +288,7 @@ pub fn query_get(criterion: &mut Criterion) {
.collect();
entities.shuffle(&mut deterministic_rand());
let mut query = SystemState::<Query<&Sparse>>::new(&mut world);
let query = query.get(&world);
let query = query.get(&world).unwrap();

bencher.iter(|| {
let mut count = 0;
Expand Down Expand Up @@ -319,7 +319,7 @@ pub fn query_get_many<const N: usize>(criterion: &mut Criterion) {
entity_groups.shuffle(&mut deterministic_rand());

let mut query = SystemState::<Query<&Table>>::new(&mut world);
let query = query.get(&world);
let query = query.get(&world).unwrap();

bencher.iter(|| {
let mut count = 0;
Expand All @@ -342,7 +342,7 @@ pub fn query_get_many<const N: usize>(criterion: &mut Criterion) {
entity_groups.shuffle(&mut deterministic_rand());

let mut query = SystemState::<Query<&Sparse>>::new(&mut world);
let query = query.get(&world);
let query = query.get(&world).unwrap();

bencher.iter(|| {
let mut count = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ fn main() {

let mut system_state = SystemState::<Query<&mut Foo>>::new(&mut world);
{
let mut query = system_state.get_mut(&mut world);
let mut query = system_state.get_mut(&mut world).unwrap();
dbg!("hi");
{
let data: &Foo = query.get(e).unwrap();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ fn main() {
world.spawn(Foo(10));

let mut system_state = SystemState::<Query<(&mut Foo, &Bar)>>::new(&mut world);
let mut query = system_state.get_mut(&mut world);
let mut query = system_state.get_mut(&mut world).unwrap();

{
let mut lens_a = query.transmute_lens::<&mut Foo>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ struct State {

impl State {
fn get_component(&mut self, world: &mut World, entity: Entity) {
let q1 = self.state_r.get(&world);
let q1 = self.state_r.get(&world).unwrap();
let a1 = q1.get(entity).unwrap();

let mut q2 = self.state_w.get_mut(world);
let mut q2 = self.state_w.get_mut(world).unwrap();
//~^ E0502
let _ = q2.get_mut(entity).unwrap();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ struct State {

impl State {
fn get_components(&mut self, world: &mut World) {
let q1 = self.state_r.get(&world);
let q1 = self.state_r.get(&world).unwrap();
let a1 = q1.iter().next().unwrap();

let mut q2 = self.state_w.get_mut(world);
let mut q2 = self.state_w.get_mut(world).unwrap();
//~^ E0502
let _ = q2.iter_mut().next().unwrap();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ fn main() {

let mut system_state = SystemState::<Query<&mut A>>::new(&mut world);
{
let mut query = system_state.get_mut(&mut world);
let mut query = system_state.get_mut(&mut world).unwrap();
let mut_vec = query.iter_mut().collect::<Vec<bevy_ecs::prelude::Mut<A>>>();
assert_eq!(
// this should fail to compile due to the later use of mut_vec
Expand Down
54 changes: 28 additions & 26 deletions crates/bevy_ecs/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,9 +316,11 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
// SAFETY: systems run without conflicts with other systems.
// Conflicting params in ParamSet are not accessible at the same time
// ParamSets are guaranteed to not conflict with other SystemParams
unsafe {
let maybe_param = unsafe {
#param::get_param(&mut self.param_states.#index, &self.system_meta, self.world, self.change_tick)
}
};
// `ParamSet::get_param` ensures all sub-params are accessible.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this true? It looks like get_param below does an unconditional Some(param). Did you mean to call validate_param there?

It would be nice to keep some way of delaying validation on the sub-parameters. Like, the safe PipeSystem example will want to to delay validation of the second parameter, for the same reason you're doing so for the real PipeSystem. Maybe that could be spelled ParamSet<(A::Param, Option<B::Param>)>? Although doing a blanket impl on Option would change the semantics of Option<Single>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't tell now, this was made quite a while ago.
We'll definitely want someone experience check the ParamSet, DynamicSystemParam, etc.

maybe_param.unwrap()
}
});
}
Expand Down Expand Up @@ -380,28 +382,30 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
<(#(#param,)*) as SystemParam>::queue(state, system_meta, world.reborrow());
}

#[inline]
unsafe fn validate_param<'w, 's>(
state: &'s Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell<'w>,
) -> bool {
<(#(#param,)*) as SystemParam>::validate_param(state, system_meta, world)
}

#[inline]
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
ParamSet {
) -> Option<Self::Item<'w, 's>> {
let param = ParamSet {
param_states: state,
system_meta: system_meta.clone(),
world,
change_tick,
}
};
Some(param)
}

#[inline]
unsafe fn validate_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> bool {
<(#(#param,)*) as SystemParam>::validate_param(state, system_meta, world, change_tick)
}
}

Expand Down Expand Up @@ -631,27 +635,25 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
}

#[inline]
unsafe fn validate_param<'w, 's>(
state: &'s Self::State,
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &#path::system::SystemMeta,
world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
) -> bool {
<(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&state.state, system_meta, world)
change_tick: #path::component::Tick,
) -> Option<Self::Item<'w, 's>> {
let (#(#tuple_patterns,)*) = <(#(#tuple_types,)*) as #path::system::SystemParam>::get_param(&mut state.state, system_meta, world, change_tick)?;
let param = #struct_name { #(#fields: #field_locals,)* };
Some(param)
}

#[inline]
unsafe fn get_param<'w, 's>(
unsafe fn validate_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &#path::system::SystemMeta,
world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
change_tick: #path::component::Tick,
) -> Self::Item<'w, 's> {
let (#(#tuple_patterns,)*) = <
(#(#tuple_types,)*) as #path::system::SystemParam
>::get_param(&mut state.state, system_meta, world, change_tick);
#struct_name {
#(#fields: #field_locals,)*
}
) -> bool {
<(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&mut state.state, system_meta, world, change_tick)
}
}

Expand Down
22 changes: 11 additions & 11 deletions crates/bevy_ecs/src/change_detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,17 +164,17 @@ pub trait DetectChangesMut: DetectChanges {
/// # world.insert_resource(Score(1));
/// # let mut score_changed = IntoSystem::into_system(resource_changed::<Score>);
/// # score_changed.initialize(&mut world);
/// # score_changed.run((), &mut world);
/// # score_changed.run((), &mut world).unwrap();
/// #
/// # let mut schedule = Schedule::default();
/// # schedule.add_systems(reset_score);
/// #
/// # // first time `reset_score` runs, the score is changed.
/// # schedule.run(&mut world);
/// # assert!(score_changed.run((), &mut world));
/// # assert!(score_changed.run((), &mut world).unwrap());
/// # // second time `reset_score` runs, the score is not changed.
/// # schedule.run(&mut world);
/// # assert!(!score_changed.run((), &mut world));
/// # assert!(!score_changed.run((), &mut world).unwrap());
/// ```
#[inline]
#[track_caller]
Expand Down Expand Up @@ -235,23 +235,23 @@ pub trait DetectChangesMut: DetectChanges {
/// # world.insert_resource(Score(1));
/// # let mut score_changed = IntoSystem::into_system(resource_changed::<Score>);
/// # score_changed.initialize(&mut world);
/// # score_changed.run((), &mut world);
/// # score_changed.run((), &mut world).unwrap();
/// #
/// # let mut score_changed_event = IntoSystem::into_system(on_event::<ScoreChanged>);
/// # score_changed_event.initialize(&mut world);
/// # score_changed_event.run((), &mut world);
/// # score_changed_event.run((), &mut world).unwrap();
/// #
/// # let mut schedule = Schedule::default();
/// # schedule.add_systems(reset_score);
/// #
/// # // first time `reset_score` runs, the score is changed.
/// # schedule.run(&mut world);
/// # assert!(score_changed.run((), &mut world));
/// # assert!(score_changed_event.run((), &mut world));
/// # assert!(score_changed.run((), &mut world).unwrap());
/// # assert!(score_changed_event.run((), &mut world).unwrap());
/// # // second time `reset_score` runs, the score is not changed.
/// # schedule.run(&mut world);
/// # assert!(!score_changed.run((), &mut world));
/// # assert!(!score_changed_event.run((), &mut world));
/// # assert!(!score_changed.run((), &mut world).unwrap());
/// # assert!(!score_changed_event.run((), &mut world).unwrap());
/// ```
#[inline]
#[must_use = "If you don't need to handle the previous value, use `set_if_neq` instead."]
Expand Down Expand Up @@ -1256,7 +1256,7 @@ mod tests {

// world: 1, system last ran: 0, component changed: 1
// The spawn will be detected since it happened after the system "last ran".
assert!(change_detected_system.run((), &mut world));
assert!(change_detected_system.run((), &mut world).unwrap());

// world: 1 + MAX_CHANGE_AGE
let change_tick = world.change_tick.get_mut();
Expand All @@ -1266,7 +1266,7 @@ mod tests {
// Since we clamp things to `MAX_CHANGE_AGE` for determinism,
// `ComponentTicks::is_changed` will now see `MAX_CHANGE_AGE > MAX_CHANGE_AGE`
// and return `false`.
assert!(!change_expired_system.run((), &mut world));
assert!(!change_expired_system.run((), &mut world).unwrap());
}

#[test]
Expand Down
16 changes: 8 additions & 8 deletions crates/bevy_ecs/src/event/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,20 +522,20 @@ mod tests {
});
reader.initialize(&mut world);

let last = reader.run((), &mut world);
let last = reader.run((), &mut world).unwrap();
assert!(last.is_none(), "EventReader should be empty");

world.send_event(TestEvent { i: 0 });
let last = reader.run((), &mut world);
let last = reader.run((), &mut world).unwrap();
assert_eq!(last, Some(TestEvent { i: 0 }));

world.send_event(TestEvent { i: 1 });
world.send_event(TestEvent { i: 2 });
world.send_event(TestEvent { i: 3 });
let last = reader.run((), &mut world);
let last = reader.run((), &mut world).unwrap();
assert_eq!(last, Some(TestEvent { i: 3 }));

let last = reader.run((), &mut world);
let last = reader.run((), &mut world).unwrap();
assert!(last.is_none(), "EventReader should be empty");
}

Expand All @@ -552,20 +552,20 @@ mod tests {
});
mutator.initialize(&mut world);

let last = mutator.run((), &mut world);
let last = mutator.run((), &mut world).unwrap();
assert!(last.is_none(), "EventMutator should be empty");

world.send_event(TestEvent { i: 0 });
let last = mutator.run((), &mut world);
let last = mutator.run((), &mut world).unwrap();
assert_eq!(last, Some(TestEvent { i: 0 }));

world.send_event(TestEvent { i: 1 });
world.send_event(TestEvent { i: 2 });
world.send_event(TestEvent { i: 3 });
let last = mutator.run((), &mut world);
let last = mutator.run((), &mut world).unwrap();
assert_eq!(last, Some(TestEvent { i: 3 }));

let last = mutator.run((), &mut world);
let last = mutator.run((), &mut world).unwrap();
assert!(last.is_none(), "EventMutator should be empty");
}

Expand Down
3 changes: 1 addition & 2 deletions crates/bevy_ecs/src/observer/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,8 +374,7 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
// - system is the same type erased system from above
unsafe {
(*system).update_archetype_component_access(world);
if (*system).validate_param_unsafe(world) {
(*system).run_unsafe(trigger, world);
if (*system).run_unsafe(trigger, world).is_some() {
(*system).queue_deferred(world.into_deferred());
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,7 @@ mod tests {

// system param
let mut q = SystemState::<Query<&mut Foo>>::new(&mut world);
let q = q.get_mut(&mut world);
let q = q.get_mut(&mut world).unwrap();
let _: Option<&Foo> = q.iter().next();
let _: Option<[&Foo; 2]> = q.iter_combinations::<2>().next();
let _: Option<&Foo> = q.iter_many([e]).next();
Expand Down
Loading