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.

Sign up for GitHub

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] - Move ambiguity detection into its own file #5918

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Move ambiguity detection into its own file #5918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5d5900c
Remove basic machinery
alice-i-cecile Sep 8, 2022
edd8129
Remove ambiguity sets from algorithm
alice-i-cecile Sep 8, 2022
2819a02
Move ambiguity detection code into its own file
alice-i-cecile Sep 8, 2022
2bd7c43
Merge branch 'main' into ambiguity-detection-file
alice-i-cecile Sep 9, 2022
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
140 changes: 140 additions & 0 deletions crates/bevy_ecs/src/schedule/ambiguity_detection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
use bevy_utils::tracing::info;
use fixedbitset::FixedBitSet;

use crate::component::ComponentId;
use crate::schedule::{SystemContainer, SystemStage};
use crate::world::World;

impl SystemStage {
/// Logs execution order ambiguities between systems. System orders must be fresh.
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<ComponentId>)>,
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::<Vec<_>>();
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 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);
}
info!("{}", string);
}
}
}

/// Returns vector containing all pairs of indices of systems with ambiguous execution order,
/// along with specific components that have triggered the warning.
/// Systems must be topologically sorted beforehand.
fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec<ComponentId>)> {
let mut all_dependencies = Vec::<FixedBitSet>::with_capacity(systems.len());
let mut all_dependants = Vec::<FixedBitSet>::with_capacity(systems.len());
for (index, container) in systems.iter().enumerate() {
let mut dependencies = FixedBitSet::with_capacity(systems.len());
for &dependency in container.dependencies() {
dependencies.union_with(&all_dependencies[dependency]);
dependencies.insert(dependency);
all_dependants[dependency].insert(index);
}

all_dependants.push(FixedBitSet::with_capacity(systems.len()));
all_dependencies.push(dependencies);
}
for index in (0..systems.len()).rev() {
let mut dependants = FixedBitSet::with_capacity(systems.len());
for dependant in all_dependants[index].ones() {
dependants.union_with(&all_dependants[dependant]);
dependants.insert(dependant);
}
all_dependants[index] = dependants;
}
let mut all_relations = all_dependencies
.drain(..)
.zip(all_dependants.drain(..))
.enumerate()
.map(|(index, (dependencies, dependants))| {
let mut relations = FixedBitSet::with_capacity(systems.len());
relations.union_with(&dependencies);
relations.union_with(&dependants);
relations.insert(index);
relations
})
.collect::<Vec<FixedBitSet>>();
let mut ambiguities = Vec::new();
let full_bitset: FixedBitSet = (0..systems.len()).collect();
let mut processed = FixedBitSet::with_capacity(systems.len());
for (index_a, relations) in all_relations.drain(..).enumerate() {
// TODO: prove that `.take(index_a)` would be correct here, and uncomment it if so.
for index_b in full_bitset.difference(&relations)
// .take(index_a)
{
if !processed.contains(index_b) {
let a_access = systems[index_a].component_access();
let b_access = systems[index_b].component_access();
if let (Some(a), Some(b)) = (a_access, b_access) {
let conflicts = a.get_conflicts(b);
if !conflicts.is_empty() {
ambiguities.push((index_a, index_b, conflicts));
}
} else {
ambiguities.push((index_a, index_b, Vec::new()));
}
}
}
processed.insert(index_a);
}
ambiguities
}
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/schedule/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
//! When using Bevy ECS, systems are usually not run directly, but are inserted into a
//! [`Stage`], which then lives within a [`Schedule`].

mod ambiguity_detection;
mod executor;
mod executor_parallel;
pub mod graph_utils;
Expand Down
151 changes: 7 additions & 144 deletions crates/bevy_ecs/src/schedule/stage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,8 @@ use crate::{
world::{World, WorldId},
};
use bevy_ecs_macros::Resource;
use bevy_utils::{
tracing::{info, warn},
HashMap, HashSet,
};
use bevy_utils::{tracing::warn, HashMap, HashSet};
use downcast_rs::{impl_downcast, Downcast};
use fixedbitset::FixedBitSet;
use std::fmt::Debug;

use super::IntoSystemDescriptor;

Expand Down Expand Up @@ -67,16 +62,16 @@ pub struct SystemStage {
/// Topologically sorted run criteria of systems.
run_criteria: Vec<RunCriteriaContainer>,
/// Topologically sorted exclusive systems that want to be run at the start of the stage.
exclusive_at_start: Vec<ExclusiveSystemContainer>,
pub(super) exclusive_at_start: Vec<ExclusiveSystemContainer>,
/// Topologically sorted exclusive systems that want to be run after parallel systems but
/// before the application of their command buffers.
exclusive_before_commands: Vec<ExclusiveSystemContainer>,
pub(super) exclusive_before_commands: Vec<ExclusiveSystemContainer>,
/// Topologically sorted exclusive systems that want to be run at the end of the stage.
exclusive_at_end: Vec<ExclusiveSystemContainer>,
pub(super) exclusive_at_end: Vec<ExclusiveSystemContainer>,
/// Topologically sorted parallel systems.
parallel: Vec<ParallelSystemContainer>,
pub(super) parallel: Vec<ParallelSystemContainer>,
/// Determines if the stage was modified and needs to rebuild its graphs and orders.
systems_modified: bool,
pub(super) systems_modified: bool,
/// Determines if the stage's executor was changed.
executor_modified: bool,
/// Newly inserted run criteria that will be initialized at the next opportunity.
Expand Down Expand Up @@ -453,7 +448,7 @@ impl SystemStage {
&& self.uninitialized_before_commands.is_empty()
&& self.uninitialized_at_end.is_empty()
);
fn unwrap_dependency_cycle_error<Node: GraphNode, Output, Labels: Debug>(
fn unwrap_dependency_cycle_error<Node: GraphNode, Output, Labels: std::fmt::Debug>(
result: Result<Output, DependencyGraphError<Labels>>,
nodes: &[Node],
nodes_description: &'static str,
Expand Down Expand Up @@ -505,75 +500,6 @@ impl SystemStage {
);
}

/// Logs execution order ambiguities between systems. System orders must be fresh.
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<ComponentId>)>,
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::<Vec<_>>();
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 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);
}
info!("{}", string);
}
}

fn check_uses_resource(&self, resource_id: ComponentId, world: &World) {
debug_assert!(!self.systems_modified);
for system in &self.parallel {
Expand Down Expand Up @@ -709,69 +635,6 @@ fn process_systems(
Ok(())
}

/// Returns vector containing all pairs of indices of systems with ambiguous execution order,
/// along with specific components that have triggered the warning.
/// Systems must be topologically sorted beforehand.
fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec<ComponentId>)> {
let mut all_dependencies = Vec::<FixedBitSet>::with_capacity(systems.len());
let mut all_dependants = Vec::<FixedBitSet>::with_capacity(systems.len());
for (index, container) in systems.iter().enumerate() {
let mut dependencies = FixedBitSet::with_capacity(systems.len());
for &dependency in container.dependencies() {
dependencies.union_with(&all_dependencies[dependency]);
dependencies.insert(dependency);
all_dependants[dependency].insert(index);
}

all_dependants.push(FixedBitSet::with_capacity(systems.len()));
all_dependencies.push(dependencies);
}
for index in (0..systems.len()).rev() {
let mut dependants = FixedBitSet::with_capacity(systems.len());
for dependant in all_dependants[index].ones() {
dependants.union_with(&all_dependants[dependant]);
dependants.insert(dependant);
}
all_dependants[index] = dependants;
}
let mut all_relations = all_dependencies
.drain(..)
.zip(all_dependants.drain(..))
.enumerate()
.map(|(index, (dependencies, dependants))| {
let mut relations = FixedBitSet::with_capacity(systems.len());
relations.union_with(&dependencies);
relations.union_with(&dependants);
relations.insert(index);
relations
})
.collect::<Vec<FixedBitSet>>();
let mut ambiguities = Vec::new();
let full_bitset: FixedBitSet = (0..systems.len()).collect();
let mut processed = FixedBitSet::with_capacity(systems.len());
for (index_a, relations) in all_relations.drain(..).enumerate() {
// TODO: prove that `.take(index_a)` would be correct here, and uncomment it if so.
for index_b in full_bitset.difference(&relations)
// .take(index_a)
{
if !processed.contains(index_b) {
let a_access = systems[index_a].component_access();
let b_access = systems[index_b].component_access();
if let (Some(a), Some(b)) = (a_access, b_access) {
let conflicts = a.get_conflicts(b);
if !conflicts.is_empty() {
ambiguities.push((index_a, index_b, conflicts));
}
} else {
ambiguities.push((index_a, index_b, Vec::new()));
}
}
}
processed.insert(index_a);
}
ambiguities
}

impl Stage for SystemStage {
fn run(&mut self, world: &mut World) {
if let Some(world_id) = self.world_id {
Expand Down