Skip to content

Commit

Permalink
Simplify design for *Labels (bevyengine#4957)
Browse files Browse the repository at this point in the history
# Objective

- Closes bevyengine#4954 
- Reduce the complexity of the `{System, App, *}Label` APIs.

## Solution

For the sake of brevity I will only refer to `SystemLabel`, but everything applies to all of the other label types as well.

- Add `SystemLabelId`, a lightweight, `copy` struct.
- Convert custom types into `SystemLabelId` using the trait `SystemLabel`.

## Changelog

- String literals implement `SystemLabel` for now, but this should be changed with bevyengine#4409 .

## Migration Guide

- Any previous use of `Box<dyn SystemLabel>` should be replaced with `SystemLabelId`.
- `AsSystemLabel` trait has been modified.
    - No more output generics.
    - Method `as_system_label` now returns `SystemLabelId`, removing an unnecessary level of indirection.
- If you *need* a label that is determined at runtime, you can use `Box::leak`. Not recommended.

## Questions for later

* Should we generate a `Debug` impl along with `#[derive(*Label)]`?
* Should we rename `as_str()`?
* Should we remove the extra derives (such as `Hash`) from builtin `*Label` types?
* Should we automatically derive types like `Clone, Copy, PartialEq, Eq`?
* More-ergonomic comparisons between `Label` and `LabelId`.
* Move `Dyn{Eq, Hash,Clone}` somewhere else.
* Some API to make interning dynamic labels easier.
* Optimize string representation
    * Empty string for unit structs -- no debug info but faster comparisons
    * Don't show enum types -- same tradeoffs as asbove.
  • Loading branch information
JoJoJet authored and james7132 committed Oct 28, 2022
1 parent 9646694 commit 35992d3
Show file tree
Hide file tree
Showing 15 changed files with 308 additions and 317 deletions.
16 changes: 13 additions & 3 deletions benches/benches/bevy_ecs/scheduling/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,29 @@ pub fn build_schedule(criterion: &mut Criterion) {

// Use multiple different kinds of label to ensure that dynamic dispatch
// doesn't somehow get optimized away.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, SystemLabel)]
#[derive(Debug, Clone, Copy)]
struct NumLabel(usize);
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, SystemLabel)]
#[derive(Debug, Clone, Copy, SystemLabel)]
struct DummyLabel;

impl SystemLabel for NumLabel {
fn as_str(&self) -> &'static str {
let s = self.0.to_string();
Box::leak(s.into_boxed_str())
}
}

let mut group = criterion.benchmark_group("build_schedule");
group.warm_up_time(std::time::Duration::from_millis(500));
group.measurement_time(std::time::Duration::from_secs(15));

// Method: generate a set of `graph_size` systems which have a One True Ordering.
// Add system to the stage with full constraints. Hopefully this should be maximimally
// difficult for bevy to figure out.
let labels: Vec<_> = (0..1000).map(NumLabel).collect();
// Also, we are performing the `as_label` operation outside of the loop since that
// requires an allocation and a leak. This is not something that would be necessary in a
// real scenario, just a contrivance for the benchmark.
let labels: Vec<_> = (0..1000).map(|i| NumLabel(i).as_label()).collect();

// Benchmark graphs of different sizes.
for graph_size in [100, 500, 1000] {
Expand Down
15 changes: 8 additions & 7 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub struct App {
pub runner: Box<dyn Fn(App)>,
/// A container of [`Stage`]s set to be run in a linear order.
pub schedule: Schedule,
sub_apps: HashMap<Box<dyn AppLabel>, SubApp>,
sub_apps: HashMap<AppLabelId, SubApp>,
}

/// Each `SubApp` has its own [`Schedule`] and [`World`], enabling a separation of concerns.
Expand Down Expand Up @@ -879,7 +879,7 @@ impl App {
sub_app_runner: impl Fn(&mut World, &mut App) + 'static,
) -> &mut Self {
self.sub_apps.insert(
Box::new(label),
label.as_label(),
SubApp {
app,
runner: Box::new(sub_app_runner),
Expand All @@ -896,15 +896,16 @@ impl App {
pub fn sub_app_mut(&mut self, label: impl AppLabel) -> &mut App {
match self.get_sub_app_mut(label) {
Ok(app) => app,
Err(label) => panic!("Sub-App with label '{:?}' does not exist", label),
Err(label) => panic!("Sub-App with label '{:?}' does not exist", label.as_str()),
}
}

/// Retrieves a `SubApp` inside this [`App`] with the given label, if it exists. Otherwise returns
/// an [`Err`] containing the given label.
pub fn get_sub_app_mut(&mut self, label: impl AppLabel) -> Result<&mut App, impl AppLabel> {
pub fn get_sub_app_mut(&mut self, label: impl AppLabel) -> Result<&mut App, AppLabelId> {
let label = label.as_label();
self.sub_apps
.get_mut((&label) as &dyn AppLabel)
.get_mut(&label)
.map(|sub_app| &mut sub_app.app)
.ok_or(label)
}
Expand All @@ -917,15 +918,15 @@ impl App {
pub fn sub_app(&self, label: impl AppLabel) -> &App {
match self.get_sub_app(label) {
Ok(app) => app,
Err(label) => panic!("Sub-App with label '{:?}' does not exist", label),
Err(label) => panic!("Sub-App with label '{:?}' does not exist", label.as_str()),
}
}

/// Retrieves a `SubApp` inside this [`App`] with the given label, if it exists. Otherwise returns
/// an [`Err`] containing the given label.
pub fn get_sub_app(&self, label: impl AppLabel) -> Result<&App, impl AppLabel> {
self.sub_apps
.get((&label) as &dyn AppLabel)
.get(&label.as_label())
.map(|sub_app| &sub_app.app)
.ok_or(label)
}
Expand Down
5 changes: 0 additions & 5 deletions crates/bevy_ecs/src/schedule/label.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,3 @@ define_label!(StageLabel);
define_label!(SystemLabel);
define_label!(AmbiguitySetLabel);
define_label!(RunCriteriaLabel);

pub(crate) type BoxedStageLabel = Box<dyn StageLabel>;
pub(crate) type BoxedSystemLabel = Box<dyn SystemLabel>;
pub(crate) type BoxedAmbiguitySetLabel = Box<dyn AmbiguitySetLabel>;
pub(crate) type BoxedRunCriteriaLabel = Box<dyn RunCriteriaLabel>;
45 changes: 24 additions & 21 deletions crates/bevy_ecs/src/schedule/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ use bevy_utils::HashMap;
/// runs indefinitely.
#[derive(Default)]
pub struct Schedule {
stages: HashMap<BoxedStageLabel, Box<dyn Stage>>,
stage_order: Vec<BoxedStageLabel>,
stages: HashMap<StageLabelId, Box<dyn Stage>>,
stage_order: Vec<StageLabelId>,
run_criteria: BoxedRunCriteria,
}

Expand Down Expand Up @@ -109,9 +109,9 @@ impl Schedule {
/// schedule.add_stage("my_stage", SystemStage::parallel());
/// ```
pub fn add_stage<S: Stage>(&mut self, label: impl StageLabel, stage: S) -> &mut Self {
let label: Box<dyn StageLabel> = Box::new(label);
self.stage_order.push(label.clone());
let prev = self.stages.insert(label.clone(), Box::new(stage));
let label = label.as_label();
self.stage_order.push(label);
let prev = self.stages.insert(label, Box::new(stage));
assert!(prev.is_none(), "Stage already exists: {:?}.", label);
self
}
Expand All @@ -133,18 +133,18 @@ impl Schedule {
label: impl StageLabel,
stage: S,
) -> &mut Self {
let label: Box<dyn StageLabel> = Box::new(label);
let target = &target as &dyn StageLabel;
let label = label.as_label();
let target = target.as_label();
let target_index = self
.stage_order
.iter()
.enumerate()
.find(|(_i, stage_label)| &***stage_label == target)
.find(|(_i, stage_label)| **stage_label == target)
.map(|(i, _)| i)
.unwrap_or_else(|| panic!("Target stage does not exist: {:?}.", target));

self.stage_order.insert(target_index + 1, label.clone());
let prev = self.stages.insert(label.clone(), Box::new(stage));
self.stage_order.insert(target_index + 1, label);
let prev = self.stages.insert(label, Box::new(stage));
assert!(prev.is_none(), "Stage already exists: {:?}.", label);
self
}
Expand All @@ -167,18 +167,18 @@ impl Schedule {
label: impl StageLabel,
stage: S,
) -> &mut Self {
let label: Box<dyn StageLabel> = Box::new(label);
let target = &target as &dyn StageLabel;
let label = label.as_label();
let target = target.as_label();
let target_index = self
.stage_order
.iter()
.enumerate()
.find(|(_i, stage_label)| &***stage_label == target)
.find(|(_i, stage_label)| **stage_label == target)
.map(|(i, _)| i)
.unwrap_or_else(|| panic!("Target stage does not exist: {:?}.", target));

self.stage_order.insert(target_index, label.clone());
let prev = self.stages.insert(label.clone(), Box::new(stage));
self.stage_order.insert(target_index, label);
let prev = self.stages.insert(label, Box::new(stage));
assert!(prev.is_none(), "Stage already exists: {:?}.", label);
self
}
Expand Down Expand Up @@ -213,7 +213,7 @@ impl Schedule {

let stage = self
.get_stage_mut::<SystemStage>(&stage_label)
.unwrap_or_else(move || stage_not_found(&stage_label));
.unwrap_or_else(move || stage_not_found(&stage_label.as_label()));
stage.add_system(system);
self
}
Expand Down Expand Up @@ -282,7 +282,10 @@ impl Schedule {
func: F,
) -> &mut Self {
let stage = self.get_stage_mut::<T>(&label).unwrap_or_else(move || {
panic!("stage '{:?}' does not exist or is the wrong type", label)
panic!(
"stage '{:?}' does not exist or is the wrong type",
label.as_label()
)
});
func(stage);
self
Expand All @@ -305,7 +308,7 @@ impl Schedule {
/// ```
pub fn get_stage<T: Stage>(&self, label: &dyn StageLabel) -> Option<&T> {
self.stages
.get(label)
.get(&label.as_label())
.and_then(|stage| stage.downcast_ref::<T>())
}

Expand All @@ -326,7 +329,7 @@ impl Schedule {
/// ```
pub fn get_stage_mut<T: Stage>(&mut self, label: &dyn StageLabel) -> Option<&mut T> {
self.stages
.get_mut(label)
.get_mut(&label.as_label())
.and_then(|stage| stage.downcast_mut::<T>())
}

Expand All @@ -341,10 +344,10 @@ impl Schedule {
}

/// Iterates over all of schedule's stages and their labels, in execution order.
pub fn iter_stages(&self) -> impl Iterator<Item = (&dyn StageLabel, &dyn Stage)> {
pub fn iter_stages(&self) -> impl Iterator<Item = (StageLabelId, &dyn Stage)> {
self.stage_order
.iter()
.map(move |label| (&**label, &*self.stages[label]))
.map(move |&label| (label, &*self.stages[&label]))
}
}

Expand Down
40 changes: 20 additions & 20 deletions crates/bevy_ecs/src/schedule/run_criteria.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::{
schedule::{BoxedRunCriteriaLabel, GraphNode, RunCriteriaLabel},
schedule::{GraphNode, RunCriteriaLabel, RunCriteriaLabelId},
system::{BoxedSystem, IntoSystem, Local},
world::World,
};
Expand Down Expand Up @@ -104,9 +104,9 @@ pub(crate) enum RunCriteriaInner {
pub(crate) struct RunCriteriaContainer {
pub(crate) should_run: ShouldRun,
pub(crate) inner: RunCriteriaInner,
pub(crate) label: Option<BoxedRunCriteriaLabel>,
pub(crate) before: Vec<BoxedRunCriteriaLabel>,
pub(crate) after: Vec<BoxedRunCriteriaLabel>,
pub(crate) label: Option<RunCriteriaLabelId>,
pub(crate) before: Vec<RunCriteriaLabelId>,
pub(crate) after: Vec<RunCriteriaLabelId>,
}

impl RunCriteriaContainer {
Expand Down Expand Up @@ -139,7 +139,7 @@ impl RunCriteriaContainer {
}

impl GraphNode for RunCriteriaContainer {
type Label = BoxedRunCriteriaLabel;
type Label = RunCriteriaLabelId;

fn name(&self) -> Cow<'static, str> {
match &self.inner {
Expand All @@ -148,26 +148,26 @@ impl GraphNode for RunCriteriaContainer {
}
}

fn labels(&self) -> &[BoxedRunCriteriaLabel] {
fn labels(&self) -> &[RunCriteriaLabelId] {
if let Some(ref label) = self.label {
std::slice::from_ref(label)
} else {
&[]
}
}

fn before(&self) -> &[BoxedRunCriteriaLabel] {
fn before(&self) -> &[RunCriteriaLabelId] {
&self.before
}

fn after(&self) -> &[BoxedRunCriteriaLabel] {
fn after(&self) -> &[RunCriteriaLabelId] {
&self.after
}
}

pub enum RunCriteriaDescriptorOrLabel {
Descriptor(RunCriteriaDescriptor),
Label(BoxedRunCriteriaLabel),
Label(RunCriteriaLabelId),
}

#[derive(Clone, Copy)]
Expand All @@ -178,10 +178,10 @@ pub(crate) enum DuplicateLabelStrategy {

pub struct RunCriteriaDescriptor {
pub(crate) system: RunCriteriaSystem,
pub(crate) label: Option<BoxedRunCriteriaLabel>,
pub(crate) label: Option<RunCriteriaLabelId>,
pub(crate) duplicate_label_strategy: DuplicateLabelStrategy,
pub(crate) before: Vec<BoxedRunCriteriaLabel>,
pub(crate) after: Vec<BoxedRunCriteriaLabel>,
pub(crate) before: Vec<RunCriteriaLabelId>,
pub(crate) after: Vec<RunCriteriaLabelId>,
}

pub(crate) enum RunCriteriaSystem {
Expand Down Expand Up @@ -222,12 +222,12 @@ where
}
}

impl<L> IntoRunCriteria<BoxedRunCriteriaLabel> for L
impl<L> IntoRunCriteria<RunCriteriaLabelId> for L
where
L: RunCriteriaLabel,
{
fn into(self) -> RunCriteriaDescriptorOrLabel {
RunCriteriaDescriptorOrLabel::Label(Box::new(self))
RunCriteriaDescriptorOrLabel::Label(self.as_label())
}
}

Expand All @@ -254,24 +254,24 @@ pub trait RunCriteriaDescriptorCoercion<Param> {

impl RunCriteriaDescriptorCoercion<()> for RunCriteriaDescriptor {
fn label(mut self, label: impl RunCriteriaLabel) -> RunCriteriaDescriptor {
self.label = Some(Box::new(label));
self.label = Some(label.as_label());
self.duplicate_label_strategy = DuplicateLabelStrategy::Panic;
self
}

fn label_discard_if_duplicate(mut self, label: impl RunCriteriaLabel) -> RunCriteriaDescriptor {
self.label = Some(Box::new(label));
self.label = Some(label.as_label());
self.duplicate_label_strategy = DuplicateLabelStrategy::Discard;
self
}

fn before(mut self, label: impl RunCriteriaLabel) -> RunCriteriaDescriptor {
self.before.push(Box::new(label));
self.before.push(label.as_label());
self
}

fn after(mut self, label: impl RunCriteriaLabel) -> RunCriteriaDescriptor {
self.after.push(Box::new(label));
self.after.push(label.as_label());
self
}
}
Expand Down Expand Up @@ -327,7 +327,7 @@ where
}

pub struct RunCriteria {
label: BoxedRunCriteriaLabel,
label: RunCriteriaLabelId,
}

impl RunCriteria {
Expand All @@ -342,7 +342,7 @@ impl RunCriteria {
label: None,
duplicate_label_strategy: DuplicateLabelStrategy::Panic,
before: vec![],
after: vec![Box::new(label)],
after: vec![label.as_label()],
}
}
}
Loading

0 comments on commit 35992d3

Please sign in to comment.