Skip to content

Use SlotMaps to store systems and system sets in Schedules #19352

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Use SlotMaps to store systems and system sets in Schedules #19352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0aac3f
slotmaps
ItsDoot May 24, 2025
bfed18a
fix ci
ItsDoot May 24, 2025
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
1 change: 1 addition & 0 deletions crates/bevy_ecs/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ variadics_please = { version = "1.1", default-features = false }
tracing = { version = "0.1", default-features = false, optional = true }
log = { version = "0.4", default-features = false }
bumpalo = "3"
slotmap = { version = "1.0.7", default-features = false }

concurrent-queue = { version = "2.5.0", default-features = false }
[target.'cfg(not(all(target_has_atomic = "8", target_has_atomic = "16", target_has_atomic = "32", target_has_atomic = "64", target_has_atomic = "ptr")))'.dependencies]
Expand Down
128 changes: 76 additions & 52 deletions crates/bevy_ecs/src/schedule/auto_insert_apply_deferred.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ use alloc::{boxed::Box, collections::BTreeSet, vec::Vec};

use bevy_platform::collections::HashMap;

use crate::system::IntoSystem;
use crate::schedule::SystemSetKey;
use crate::world::World;
use crate::{schedule::SystemKey, system::IntoSystem};

use super::{
is_apply_deferred, ApplyDeferred, DiGraph, Direction, NodeId, ReportCycles, ScheduleBuildError,
Expand Down Expand Up @@ -37,28 +38,26 @@ impl AutoInsertApplyDeferredPass {
.get(&distance)
.copied()
.or_else(|| {
let node_id = self.add_auto_sync(graph);
let node_id = NodeId::System(self.add_auto_sync(graph));
self.auto_sync_node_ids.insert(distance, node_id);
Some(node_id)
})
.unwrap()
}
/// add an [`ApplyDeferred`] system with no config
fn add_auto_sync(&mut self, graph: &mut ScheduleGraph) -> NodeId {
let id = NodeId::System(graph.systems.len());

graph
fn add_auto_sync(&mut self, graph: &mut ScheduleGraph) -> SystemKey {
let key = graph
.systems
.push(SystemNode::new(Box::new(IntoSystem::into_system(
.insert(SystemNode::new(Box::new(IntoSystem::into_system(
ApplyDeferred,
))));
graph.system_conditions.push(Vec::new());
graph.system_conditions.insert(key, Vec::new());

// ignore ambiguities with auto sync points
// They aren't under user control, so no one should know or care.
graph.ambiguous_with_all.insert(id);
graph.ambiguous_with_all.insert(NodeId::System(key));

id
key
}
}

Expand All @@ -80,76 +79,90 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
let mut sync_point_graph = dependency_flattened.clone();
let topo = graph.topsort_graph(dependency_flattened, ReportCycles::Dependency)?;

fn set_has_conditions(graph: &ScheduleGraph, node: NodeId) -> bool {
!graph.set_conditions_at(node).is_empty()
fn set_has_conditions(graph: &ScheduleGraph, set: SystemSetKey) -> bool {
!graph.set_conditions_at(set).is_empty()
|| graph
.hierarchy()
.graph()
.edges_directed(node, Direction::Incoming)
.any(|(parent, _)| set_has_conditions(graph, parent))
.edges_directed(NodeId::Set(set), Direction::Incoming)
.any(|(parent, _)| {
parent
.as_set()
.is_some_and(|p| set_has_conditions(graph, p))
})
}

fn system_has_conditions(graph: &ScheduleGraph, node: NodeId) -> bool {
assert!(node.is_system());
!graph.system_conditions[node.index()].is_empty()
fn system_has_conditions(graph: &ScheduleGraph, key: SystemKey) -> bool {
!graph.system_conditions[key].is_empty()
|| graph
.hierarchy()
.graph()
.edges_directed(node, Direction::Incoming)
.any(|(parent, _)| set_has_conditions(graph, parent))
.edges_directed(NodeId::System(key), Direction::Incoming)
.any(|(parent, _)| {
parent
.as_set()
.is_some_and(|p| set_has_conditions(graph, p))
})
}

let mut system_has_conditions_cache = HashMap::<usize, bool>::default();
let mut is_valid_explicit_sync_point = |system: NodeId| {
let index = system.index();
is_apply_deferred(graph.systems[index].get().unwrap())
let mut system_has_conditions_cache = HashMap::<SystemKey, bool>::default();
let mut is_valid_explicit_sync_point = |key: SystemKey| {
is_apply_deferred(graph.systems[key].get().unwrap())
&& !*system_has_conditions_cache
.entry(index)
.or_insert_with(|| system_has_conditions(graph, system))
.entry(key)
.or_insert_with(|| system_has_conditions(graph, key))
};

// Calculate the distance for each node.
// The "distance" is the number of sync points between a node and the beginning of the graph.
// Also store if a preceding edge would have added a sync point but was ignored to add it at
// a later edge that is not ignored.
let mut distances_and_pending_sync: HashMap<usize, (u32, bool)> =
let mut distances_and_pending_sync: HashMap<SystemKey, (u32, bool)> =
HashMap::with_capacity_and_hasher(topo.len(), Default::default());

// Keep track of any explicit sync nodes for a specific distance.
let mut distance_to_explicit_sync_node: HashMap<u32, NodeId> = HashMap::default();

// Determine the distance for every node and collect the explicit sync points.
for node in &topo {
let &NodeId::System(key) = node else {
continue;
};

let (node_distance, mut node_needs_sync) = distances_and_pending_sync
.get(&node.index())
.get(&key)
.copied()
.unwrap_or_default();

if is_valid_explicit_sync_point(*node) {
if is_valid_explicit_sync_point(key) {
// The distance of this sync point does not change anymore as the iteration order
// makes sure that this node is no unvisited target of another node.
// Because of this, the sync point can be stored for this distance to be reused as
// automatically added sync points later.
distance_to_explicit_sync_node.insert(node_distance, *node);
distance_to_explicit_sync_node.insert(node_distance, NodeId::System(key));

// This node just did a sync, so the only reason to do another sync is if one was
// explicitly scheduled afterwards.
node_needs_sync = false;
} else if !node_needs_sync {
// No previous node has postponed sync points to add so check if the system itself
// has deferred params that require a sync point to apply them.
node_needs_sync = graph.systems[node.index()].get().unwrap().has_deferred();
node_needs_sync = graph.systems[key].get().unwrap().has_deferred();
}

for target in dependency_flattened.neighbors_directed(*node, Direction::Outgoing) {
let (target_distance, target_pending_sync) = distances_and_pending_sync
.entry(target.index())
.or_default();
let NodeId::System(target) = target else {
continue;
};
let (target_distance, target_pending_sync) =
distances_and_pending_sync.entry(target).or_default();

let mut edge_needs_sync = node_needs_sync;
if node_needs_sync
&& !graph.systems[target.index()].get().unwrap().is_exclusive()
&& self.no_sync_edges.contains(&(*node, target))
&& !graph.systems[target].get().unwrap().is_exclusive()
&& self
.no_sync_edges
.contains(&(*node, NodeId::System(target)))
{
// The node has deferred params to apply, but this edge is ignoring sync points.
// Mark the target as 'delaying' those commands to a future edge and the current
Expand All @@ -174,14 +187,20 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
// Find any edges which have a different number of sync points between them and make sure
// there is a sync point between them.
for node in &topo {
let &NodeId::System(key) = node else {
continue;
};
let (node_distance, _) = distances_and_pending_sync
.get(&node.index())
.get(&key)
.copied()
.unwrap_or_default();

for target in dependency_flattened.neighbors_directed(*node, Direction::Outgoing) {
let NodeId::System(target) = target else {
continue;
};
let (target_distance, _) = distances_and_pending_sync
.get(&target.index())
.get(&target)
.copied()
.unwrap_or_default();

Expand All @@ -190,7 +209,7 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
continue;
}

if is_apply_deferred(graph.systems[target.index()].get().unwrap()) {
if is_apply_deferred(graph.systems[target].get().unwrap()) {
// We don't need to insert a sync point since ApplyDeferred is a sync point
// already!
continue;
Expand All @@ -202,10 +221,10 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
.unwrap_or_else(|| self.get_sync_point(graph, target_distance));

sync_point_graph.add_edge(*node, sync_point);
sync_point_graph.add_edge(sync_point, target);
sync_point_graph.add_edge(sync_point, NodeId::System(target));

// The edge without the sync point is now redundant.
sync_point_graph.remove_edge(*node, target);
sync_point_graph.remove_edge(*node, NodeId::System(target));
}
}

Expand All @@ -215,34 +234,39 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {

fn collapse_set(
&mut self,
set: NodeId,
systems: &[NodeId],
set: SystemSetKey,
systems: &[SystemKey],
dependency_flattened: &DiGraph,
) -> impl Iterator<Item = (NodeId, NodeId)> {
if systems.is_empty() {
// collapse dependencies for empty sets
for a in dependency_flattened.neighbors_directed(set, Direction::Incoming) {
for b in dependency_flattened.neighbors_directed(set, Direction::Outgoing) {
if self.no_sync_edges.contains(&(a, set))
&& self.no_sync_edges.contains(&(set, b))
for a in dependency_flattened.neighbors_directed(NodeId::Set(set), Direction::Incoming)
{
for b in
dependency_flattened.neighbors_directed(NodeId::Set(set), Direction::Outgoing)
{
if self.no_sync_edges.contains(&(a, NodeId::Set(set)))
&& self.no_sync_edges.contains(&(NodeId::Set(set), b))
{
self.no_sync_edges.insert((a, b));
}
}
}
} else {
for a in dependency_flattened.neighbors_directed(set, Direction::Incoming) {
for a in dependency_flattened.neighbors_directed(NodeId::Set(set), Direction::Incoming)
{
for &sys in systems {
if self.no_sync_edges.contains(&(a, set)) {
self.no_sync_edges.insert((a, sys));
if self.no_sync_edges.contains(&(a, NodeId::Set(set))) {
self.no_sync_edges.insert((a, NodeId::System(sys)));
}
}
}

for b in dependency_flattened.neighbors_directed(set, Direction::Outgoing) {
for b in dependency_flattened.neighbors_directed(NodeId::Set(set), Direction::Outgoing)
{
for &sys in systems {
if self.no_sync_edges.contains(&(set, b)) {
self.no_sync_edges.insert((sys, b));
if self.no_sync_edges.contains(&(NodeId::Set(set), b)) {
self.no_sync_edges.insert((NodeId::System(sys), b));
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/schedule/executor/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::{
error::{BevyError, ErrorContext, Result},
prelude::{IntoSystemSet, SystemSet},
query::{Access, FilteredAccessSet},
schedule::{BoxedCondition, InternedSystemSet, NodeId, SystemTypeSet},
schedule::{BoxedCondition, InternedSystemSet, SystemKey, SystemSetKey, SystemTypeSet},
system::{ScheduleSystem, System, SystemIn, SystemParamValidationError},
world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World},
};
Expand Down Expand Up @@ -73,7 +73,7 @@ pub enum ExecutorKind {
#[derive(Default)]
pub struct SystemSchedule {
/// List of system node ids.
pub(super) system_ids: Vec<NodeId>,
pub(super) system_ids: Vec<SystemKey>,
/// Indexed by system node id.
pub(super) systems: Vec<ScheduleSystem>,
/// Indexed by system node id.
Expand All @@ -96,7 +96,7 @@ pub struct SystemSchedule {
/// List of sets containing the system that have conditions
pub(super) sets_with_conditions_of_systems: Vec<FixedBitSet>,
/// List of system set node ids.
pub(super) set_ids: Vec<NodeId>,
pub(super) set_ids: Vec<SystemSetKey>,
/// Indexed by system set node id.
pub(super) set_conditions: Vec<Vec<BoxedCondition>>,
/// Indexed by system set node id.
Expand Down
Loading
Loading