From fb74ca3d4644cf31afefe62ecd5e566794211290 Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Thu, 22 Sep 2022 20:01:54 +0000 Subject: [PATCH] Add ambiguity detection tests (#6053) # Objective - Add unit tests for ambiguity detection reporting. - Incremental implementation of #4299. ## Solution - Refactor ambiguity detection internals to make it testable. As a bonus, this should make it easier to extend in the future. ## Notes * This code was copy-pasted from #4299 and modified. Credit goes to @alice-i-cecile and @afonsolage, though I'm not sure who wrote what at this point. --- .../src/schedule/ambiguity_detection.rs | 475 +++++++++++++++--- 1 file changed, 417 insertions(+), 58 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/ambiguity_detection.rs b/crates/bevy_ecs/src/schedule/ambiguity_detection.rs index bca2450eb6..a9aa1de58b 100644 --- a/crates/bevy_ecs/src/schedule/ambiguity_detection.rs +++ b/crates/bevy_ecs/src/schedule/ambiguity_detection.rs @@ -5,75 +5,216 @@ use crate::component::ComponentId; use crate::schedule::{SystemContainer, SystemStage}; use crate::world::World; +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +struct SystemOrderAmbiguity { + segment: SystemStageSegment, + // Note: In order for comparisons to work correctly, + // `system_names` and `conflicts` must be sorted at all times. + system_names: [String; 2], + conflicts: Vec, +} + +/// Which part of a [`SystemStage`] was a [`SystemOrderAmbiguity`] detected in? +#[derive(Debug, PartialEq, Eq, Clone, Copy, PartialOrd, Ord, Hash)] +enum SystemStageSegment { + Parallel, + ExclusiveAtStart, + ExclusiveBeforeCommands, + ExclusiveAtEnd, +} + +impl SystemStageSegment { + pub fn desc(&self) -> &'static str { + match self { + SystemStageSegment::Parallel => "Parallel systems", + SystemStageSegment::ExclusiveAtStart => "Exclusive systems at start of stage", + SystemStageSegment::ExclusiveBeforeCommands => { + "Exclusive systems before commands of stage" + } + SystemStageSegment::ExclusiveAtEnd => "Exclusive systems at end of stage", + } + } +} + +impl SystemOrderAmbiguity { + fn from_raw( + system_a_index: usize, + system_b_index: usize, + component_ids: Vec, + segment: SystemStageSegment, + stage: &SystemStage, + world: &World, + ) -> Self { + use crate::schedule::graph_utils::GraphNode; + use SystemStageSegment::*; + + // TODO: blocked on https://github.com/bevyengine/bevy/pull/4166 + // We can't grab the system container generically, because .parallel_systems() + // and the exclusive equivalent return a different type, + // and SystemContainer is not object-safe + let (system_a_name, system_b_name) = match segment { + Parallel => { + let system_container = stage.parallel_systems(); + ( + system_container[system_a_index].name(), + system_container[system_b_index].name(), + ) + } + ExclusiveAtStart => { + let system_container = stage.exclusive_at_start_systems(); + ( + system_container[system_a_index].name(), + system_container[system_b_index].name(), + ) + } + ExclusiveBeforeCommands => { + let system_container = stage.exclusive_before_commands_systems(); + ( + system_container[system_a_index].name(), + system_container[system_b_index].name(), + ) + } + ExclusiveAtEnd => { + let system_container = stage.exclusive_at_end_systems(); + ( + system_container[system_a_index].name(), + system_container[system_b_index].name(), + ) + } + }; + + let mut system_names = [system_a_name.to_string(), system_b_name.to_string()]; + system_names.sort(); + + let mut conflicts: Vec<_> = component_ids + .iter() + .map(|id| world.components().get_info(*id).unwrap().name().to_owned()) + .collect(); + conflicts.sort(); + + Self { + system_names, + conflicts, + segment, + } + } +} + impl SystemStage { - /// Logs execution order ambiguities between systems. System orders must be fresh. + /// Logs execution order ambiguities between systems. + /// + /// The output may be incorrect if this stage has not been initialized with `world`. pub fn report_ambiguities(&self, world: &World) { debug_assert!(!self.systems_modified); use std::fmt::Write; - fn write_display_names_of_pairs( - string: &mut String, - systems: &[impl SystemContainer], - mut ambiguities: Vec<(usize, usize, Vec)>, - world: &World, - ) { - for (index_a, index_b, conflicts) in ambiguities.drain(..) { - writeln!( - string, - " -- {:?} and {:?}", - systems[index_a].name(), - systems[index_b].name() - ) - .unwrap(); - if !conflicts.is_empty() { - let names = conflicts - .iter() - .map(|id| world.components().get_info(*id).unwrap().name()) - .collect::>(); - writeln!(string, " conflicts: {:?}", names).unwrap(); - } - } - } - let parallel = find_ambiguities(&self.parallel); - let at_start = find_ambiguities(&self.exclusive_at_start); - let before_commands = find_ambiguities(&self.exclusive_before_commands); - let at_end = find_ambiguities(&self.exclusive_at_end); - if !(parallel.is_empty() - && at_start.is_empty() - && before_commands.is_empty() - && at_end.is_empty()) - { + let ambiguities = self.ambiguities(world); + if !ambiguities.is_empty() { let mut string = "Execution order ambiguities detected, you might want to \ add an explicit dependency relation between some of these systems:\n" .to_owned(); - if !parallel.is_empty() { - writeln!(string, " * Parallel systems:").unwrap(); - write_display_names_of_pairs(&mut string, &self.parallel, parallel, world); - } - if !at_start.is_empty() { - writeln!(string, " * Exclusive systems at start of stage:").unwrap(); - write_display_names_of_pairs( - &mut string, - &self.exclusive_at_start, - at_start, - world, - ); - } - if !before_commands.is_empty() { - writeln!(string, " * Exclusive systems before commands of stage:").unwrap(); - write_display_names_of_pairs( - &mut string, - &self.exclusive_before_commands, - before_commands, - world, - ); - } - if !at_end.is_empty() { - writeln!(string, " * Exclusive systems at end of stage:").unwrap(); - write_display_names_of_pairs(&mut string, &self.exclusive_at_end, at_end, world); + + let mut last_segment_kind = None; + for SystemOrderAmbiguity { + system_names: [system_a, system_b], + conflicts, + segment, + } in &ambiguities + { + // If the ambiguity occurred in a different segment than the previous one, write a header for the segment. + if last_segment_kind != Some(segment) { + writeln!(string, " * {}:", segment.desc()).unwrap(); + last_segment_kind = Some(segment); + } + + writeln!(string, " -- {:?} and {:?}", system_a, system_b).unwrap(); + + if !conflicts.is_empty() { + writeln!(string, " conflicts: {conflicts:?}").unwrap(); + } } + info!("{}", string); } } + + /// Returns all execution order ambiguities between systems. + /// + /// Returns 4 vectors of ambiguities for each stage, in the following order: + /// - parallel + /// - exclusive at start, + /// - exclusive before commands + /// - exclusive at end + /// + /// The result may be incorrect if this stage has not been initialized with `world`. + fn ambiguities(&self, world: &World) -> Vec { + let parallel = find_ambiguities(&self.parallel).into_iter().map( + |(system_a_index, system_b_index, component_ids)| { + SystemOrderAmbiguity::from_raw( + system_a_index, + system_b_index, + component_ids.to_vec(), + SystemStageSegment::Parallel, + self, + world, + ) + }, + ); + + let at_start = find_ambiguities(&self.exclusive_at_start).into_iter().map( + |(system_a_index, system_b_index, component_ids)| { + SystemOrderAmbiguity::from_raw( + system_a_index, + system_b_index, + component_ids, + SystemStageSegment::ExclusiveAtStart, + self, + world, + ) + }, + ); + + let before_commands = find_ambiguities(&self.exclusive_before_commands) + .into_iter() + .map(|(system_a_index, system_b_index, component_ids)| { + SystemOrderAmbiguity::from_raw( + system_a_index, + system_b_index, + component_ids, + SystemStageSegment::ExclusiveBeforeCommands, + self, + world, + ) + }); + + let at_end = find_ambiguities(&self.exclusive_at_end).into_iter().map( + |(system_a_index, system_b_index, component_ids)| { + SystemOrderAmbiguity::from_raw( + system_a_index, + system_b_index, + component_ids, + SystemStageSegment::ExclusiveAtEnd, + self, + world, + ) + }, + ); + + let mut ambiguities: Vec<_> = at_start + .chain(parallel) + .chain(before_commands) + .chain(at_end) + .collect(); + ambiguities.sort(); + ambiguities + } + + /// Returns the number of system order ambiguities between systems in this stage. + /// + /// The result may be incorrect if this stage has not been initialized with `world`. + #[cfg(test)] + fn ambiguity_count(&self, world: &World) -> usize { + self.ambiguities(world).len() + } } /// Returns vector containing all pairs of indices of systems with ambiguous execution order, @@ -138,3 +279,221 @@ fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec< } ambiguities } + +#[cfg(test)] +mod tests { + // Required to make the derive macro behave + use crate as bevy_ecs; + use crate::event::Events; + use crate::prelude::*; + + #[derive(Resource)] + struct R; + + #[derive(Component)] + struct A; + + #[derive(Component)] + struct B; + + // An event type + struct E; + + fn empty_system() {} + fn res_system(_res: Res) {} + fn resmut_system(_res: ResMut) {} + fn nonsend_system(_ns: NonSend) {} + fn nonsendmut_system(_ns: NonSendMut) {} + fn read_component_system(_query: Query<&A>) {} + fn write_component_system(_query: Query<&mut A>) {} + fn with_filtered_component_system(_query: Query<&mut A, With>) {} + fn without_filtered_component_system(_query: Query<&mut A, Without>) {} + fn event_reader_system(_reader: EventReader) {} + fn event_writer_system(_writer: EventWriter) {} + fn event_resource_system(_events: ResMut>) {} + fn read_world_system(_world: &World) {} + fn write_world_system(_world: &mut World) {} + + // Tests for conflict detection + + #[test] + fn one_of_everything() { + let mut world = World::new(); + world.insert_resource(R); + world.spawn().insert(A); + world.init_resource::>(); + + let mut test_stage = SystemStage::parallel(); + test_stage + // nonsendmut system deliberately conflicts with resmut system + .add_system(resmut_system) + .add_system(write_component_system) + .add_system(event_writer_system); + + test_stage.run(&mut world); + + assert_eq!(test_stage.ambiguity_count(&world), 0); + } + + #[test] + fn read_only() { + let mut world = World::new(); + world.insert_resource(R); + world.spawn().insert(A); + world.init_resource::>(); + + let mut test_stage = SystemStage::parallel(); + test_stage + .add_system(empty_system) + .add_system(empty_system) + .add_system(res_system) + .add_system(res_system) + .add_system(nonsend_system) + .add_system(nonsend_system) + .add_system(read_component_system) + .add_system(read_component_system) + .add_system(event_reader_system) + .add_system(event_reader_system) + .add_system(read_world_system) + .add_system(read_world_system); + + test_stage.run(&mut world); + + assert_eq!(test_stage.ambiguity_count(&world), 0); + } + + #[test] + fn read_world() { + let mut world = World::new(); + world.insert_resource(R); + world.spawn().insert(A); + world.init_resource::>(); + + let mut test_stage = SystemStage::parallel(); + test_stage + .add_system(resmut_system) + .add_system(write_component_system) + .add_system(event_writer_system) + .add_system(read_world_system); + + test_stage.run(&mut world); + + assert_eq!(test_stage.ambiguity_count(&world), 3); + } + + #[test] + fn resources() { + let mut world = World::new(); + world.insert_resource(R); + + let mut test_stage = SystemStage::parallel(); + test_stage.add_system(resmut_system).add_system(res_system); + + test_stage.run(&mut world); + + assert_eq!(test_stage.ambiguity_count(&world), 1); + } + + #[test] + fn nonsend() { + let mut world = World::new(); + world.insert_resource(R); + + let mut test_stage = SystemStage::parallel(); + test_stage + .add_system(nonsendmut_system) + .add_system(nonsend_system); + + test_stage.run(&mut world); + + assert_eq!(test_stage.ambiguity_count(&world), 1); + } + + #[test] + fn components() { + let mut world = World::new(); + world.spawn().insert(A); + + let mut test_stage = SystemStage::parallel(); + test_stage + .add_system(read_component_system) + .add_system(write_component_system); + + test_stage.run(&mut world); + + assert_eq!(test_stage.ambiguity_count(&world), 1); + } + + #[test] + #[ignore = "Known failing but fix is non-trivial: https://github.com/bevyengine/bevy/issues/4381"] + fn filtered_components() { + let mut world = World::new(); + world.spawn().insert(A); + + let mut test_stage = SystemStage::parallel(); + test_stage + .add_system(with_filtered_component_system) + .add_system(without_filtered_component_system); + + test_stage.run(&mut world); + + assert_eq!(test_stage.ambiguity_count(&world), 0); + } + + #[test] + fn events() { + let mut world = World::new(); + world.init_resource::>(); + + let mut test_stage = SystemStage::parallel(); + test_stage + // All of these systems clash + .add_system(event_reader_system) + .add_system(event_writer_system) + .add_system(event_resource_system); + + test_stage.run(&mut world); + + assert_eq!(test_stage.ambiguity_count(&world), 3); + } + + #[test] + fn exclusive() { + let mut world = World::new(); + world.insert_resource(R); + world.spawn().insert(A); + world.init_resource::>(); + + let mut test_stage = SystemStage::parallel(); + test_stage + // All 3 of these conflict with each other + .add_system(write_world_system.exclusive_system()) + .add_system(write_world_system.exclusive_system().at_end()) + .add_system(res_system.exclusive_system()) + // These do not, as they're in different segments of the stage + .add_system(write_world_system.exclusive_system().at_start()) + .add_system(write_world_system.exclusive_system().before_commands()); + + test_stage.run(&mut world); + + assert_eq!(test_stage.ambiguity_count(&world), 3); + } + + // Tests for silencing and resolving ambiguities + + #[test] + fn before_and_after() { + let mut world = World::new(); + world.init_resource::>(); + + let mut test_stage = SystemStage::parallel(); + test_stage + .add_system(event_reader_system.before(event_writer_system)) + .add_system(event_writer_system) + .add_system(event_resource_system.after(event_writer_system)); + + test_stage.run(&mut world); + + assert_eq!(test_stage.ambiguity_count(&world), 0); + } +}