Skip to content

Commit

Permalink
Replace all labels with interned labels (#7762)
Browse files Browse the repository at this point in the history 
# Objective

First of all, this PR took heavy inspiration from #7760 and #5715. It
intends to also fix #5569, but with a slightly different approach.


This also fixes #9335 by reexporting `DynEq`.

## Solution

The advantage of this API is that we can intern a value without
allocating for zero-sized-types and for enum variants that have no
fields. This PR does this automatically in the `SystemSet` and
`ScheduleLabel` derive macros for unit structs and fieldless enum
variants. So this should cover many internal and external use cases of
`SystemSet` and `ScheduleLabel`. In these optimal use cases, no memory
will be allocated.

- The interning returns a `Interned<dyn SystemSet>`, which is just a
wrapper around a `&'static dyn SystemSet`.
- `Hash` and `Eq` are implemented in terms of the pointer value of the
reference, similar to my first approach of anonymous system sets in
#7676.
- Therefore, `Interned<T>` does not implement `Borrow<T>`, only `Deref`.
- The debug output of `Interned<T>` is the same as the interned value.

Edit: 
- `AppLabel` is now also interned and the old
`derive_label`/`define_label` macros were replaced with the new
interning implementation.
- Anonymous set ids are reused for different `Schedule`s, reducing the
amount of leaked memory.

### Pros
- `InternedSystemSet` and `InternedScheduleLabel` behave very similar to
the current `BoxedSystemSet` and `BoxedScheduleLabel`, but can be copied
without an allocation.
- Many use cases don't allocate at all.
- Very fast lookups and comparisons when using `InternedSystemSet` and
`InternedScheduleLabel`.
- The `intern` module might be usable in other areas.
- `Interned{ScheduleLabel, SystemSet, AppLabel}` does implement
`{ScheduleLabel, SystemSet, AppLabel}`, increasing ergonomics.

### Cons
- Implementors of `SystemSet` and `ScheduleLabel` still need to
implement `Hash` and `Eq` (and `Clone`) for it to work.

## Changelog

### Added

- Added `intern` module to `bevy_utils`.
- Added reexports of `DynEq` to `bevy_ecs` and `bevy_app`.

### Changed

- Replaced `BoxedSystemSet` and `BoxedScheduleLabel` with
`InternedSystemSet` and `InternedScheduleLabel`.
- Replaced `impl AsRef<dyn ScheduleLabel>` with `impl ScheduleLabel`.
- Replaced `AppLabelId` with `InternedAppLabel`.
- Changed `AppLabel` to use `Debug` for error messages.
- Changed `AppLabel` to use interning.
- Changed `define_label`/`derive_label` to use interning. 
- Replaced `define_boxed_label`/`derive_boxed_label` with
`define_label`/`derive_label`.
- Changed anonymous set ids to be only unique inside a schedule, not
globally.
- Made interned label types implement their label trait. 

### Removed

- Removed `define_boxed_label` and `derive_boxed_label`. 

## Migration guide

- Replace `BoxedScheduleLabel` and `Box<dyn ScheduleLabel>` with
`InternedScheduleLabel` or `Interned<dyn ScheduleLabel>`.
- Replace `BoxedSystemSet` and `Box<dyn SystemSet>` with
`InternedSystemSet` or `Interned<dyn SystemSet>`.
- Replace `AppLabelId` with `InternedAppLabel` or `Interned<dyn
AppLabel>`.
- Types manually implementing `ScheduleLabel`, `AppLabel` or `SystemSet`
need to implement:
  - `dyn_hash` directly instead of implementing `DynHash`
  - `as_dyn_eq`
- Pass labels to `World::try_schedule_scope`, `World::schedule_scope`,
`World::try_run_schedule`. `World::run_schedule`, `Schedules::remove`,
`Schedules::remove_entry`, `Schedules::contains`, `Schedules::get` and
`Schedules::get_mut` by value instead of by reference.

---------

Co-authored-by: Joseph <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
  • Loading branch information
@geieredgar @JoJoJet @cart
3 people authored Oct 25, 2023
1 parent 317903f commit a830530
Show file tree
Hide file tree
Showing 22 changed files with 919 additions and 459 deletions.
162 changes: 137 additions & 25 deletions crates/bevy_app/src/app.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ use bevy_ecs::{
prelude::*,
schedule::{
apply_state_transition, common_conditions::run_once as run_once_condition,
run_enter_schedule, BoxedScheduleLabel, IntoSystemConfigs, IntoSystemSetConfigs,
run_enter_schedule, InternedScheduleLabel, IntoSystemConfigs, IntoSystemSetConfigs,
ScheduleBuildSettings, ScheduleLabel,
},
};
use bevy_utils::{tracing::debug, HashMap, HashSet};
use bevy_utils::{intern::Interned, tracing::debug, HashMap, HashSet};
use std::{
fmt::Debug,
panic::{catch_unwind, resume_unwind, AssertUnwindSafe},
Expand All @@ -20,10 +20,14 @@ use bevy_utils::tracing::info_span;
bevy_utils::define_label!(
/// A strongly-typed class of labels used to identify an [`App`].
AppLabel,
/// A strongly-typed identifier for an [`AppLabel`].
AppLabelId,
APP_LABEL_INTERNER
);

pub use bevy_utils::label::DynEq;

/// A shorthand for `Interned<dyn AppLabel>`.
pub type InternedAppLabel = Interned<dyn AppLabel>;

pub(crate) enum AppError {
DuplicatePlugin { plugin_name: String },
}
Expand Down Expand Up @@ -70,8 +74,8 @@ pub struct App {
/// The schedule that runs the main loop of schedule execution.
///
/// This is initially set to [`Main`].
pub main_schedule_label: BoxedScheduleLabel,
sub_apps: HashMap<AppLabelId, SubApp>,
pub main_schedule_label: InternedScheduleLabel,
sub_apps: HashMap<InternedAppLabel, SubApp>,
plugin_registry: Vec<Box<dyn Plugin>>,
plugin_name_added: HashSet<String>,
/// A private counter to prevent incorrect calls to `App::run()` from `Plugin::build()`
Expand Down Expand Up @@ -157,7 +161,7 @@ impl SubApp {

/// Runs the [`SubApp`]'s default schedule.
pub fn run(&mut self) {
self.app.world.run_schedule(&*self.app.main_schedule_label);
self.app.world.run_schedule(self.app.main_schedule_label);
self.app.world.clear_trackers();
}

Expand Down Expand Up @@ -233,7 +237,7 @@ impl App {
sub_apps: HashMap::default(),
plugin_registry: Vec::default(),
plugin_name_added: Default::default(),
main_schedule_label: Box::new(Main),
main_schedule_label: Main.intern(),
building_plugin_depth: 0,
plugins_state: PluginsState::Adding,
}
Expand All @@ -256,7 +260,7 @@ impl App {
{
#[cfg(feature = "trace")]
let _bevy_main_update_span = info_span!("main app").entered();
self.world.run_schedule(&*self.main_schedule_label);
self.world.run_schedule(self.main_schedule_label);
}
for (_label, sub_app) in &mut self.sub_apps {
#[cfg(feature = "trace")]
Expand Down Expand Up @@ -409,9 +413,10 @@ impl App {
schedule: impl ScheduleLabel,
systems: impl IntoSystemConfigs<M>,
) -> &mut Self {
let schedule = schedule.intern();
let mut schedules = self.world.resource_mut::<Schedules>();

if let Some(schedule) = schedules.get_mut(&schedule) {
if let Some(schedule) = schedules.get_mut(schedule) {
schedule.add_systems(systems);
} else {
let mut new_schedule = Schedule::new(schedule);
Expand Down Expand Up @@ -440,8 +445,9 @@ impl App {
schedule: impl ScheduleLabel,
sets: impl IntoSystemSetConfigs,
) -> &mut Self {
let schedule = schedule.intern();
let mut schedules = self.world.resource_mut::<Schedules>();
if let Some(schedule) = schedules.get_mut(&schedule) {
if let Some(schedule) = schedules.get_mut(schedule) {
schedule.configure_sets(sets);
} else {
let mut new_schedule = Schedule::new(schedule);
Expand Down Expand Up @@ -784,16 +790,15 @@ 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.as_str()),
Err(label) => panic!("Sub-App with label '{:?}' does not exist", label),
}
}

/// 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, AppLabelId> {
let label = label.as_label();
pub fn get_sub_app_mut(&mut self, label: impl AppLabel) -> Result<&mut App, impl AppLabel> {
self.sub_apps
.get_mut(&label)
.get_mut(&label.intern())
.map(|sub_app| &mut sub_app.app)
.ok_or(label)
}
Expand All @@ -806,25 +811,25 @@ 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.as_str()),
Err(label) => panic!("Sub-App with label '{:?}' does not exist", label),
}
}

/// Inserts an existing sub app into the app
pub fn insert_sub_app(&mut self, label: impl AppLabel, sub_app: SubApp) {
self.sub_apps.insert(label.as_label(), sub_app);
self.sub_apps.insert(label.intern(), sub_app);
}

/// Removes a sub app from the app. Returns [`None`] if the label doesn't exist.
pub fn remove_sub_app(&mut self, label: impl AppLabel) -> Option<SubApp> {
self.sub_apps.remove(&label.as_label())
self.sub_apps.remove(&label.intern())
}

/// 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_label())
.get(&label.intern())
.map(|sub_app| &sub_app.app)
.ok_or(label)
}
Expand All @@ -845,8 +850,9 @@ impl App {
///
/// See [`App::add_schedule`] to pass in a pre-constructed schedule.
pub fn init_schedule(&mut self, label: impl ScheduleLabel) -> &mut Self {
let label = label.intern();
let mut schedules = self.world.resource_mut::<Schedules>();
if !schedules.contains(&label) {
if !schedules.contains(label) {
schedules.insert(Schedule::new(label));
}
self
Expand All @@ -855,15 +861,15 @@ impl App {
/// Gets read-only access to the [`Schedule`] with the provided `label` if it exists.
pub fn get_schedule(&self, label: impl ScheduleLabel) -> Option<&Schedule> {
let schedules = self.world.get_resource::<Schedules>()?;
schedules.get(&label)
schedules.get(label)
}

/// Gets read-write access to a [`Schedule`] with the provided `label` if it exists.
pub fn get_schedule_mut(&mut self, label: impl ScheduleLabel) -> Option<&mut Schedule> {
let schedules = self.world.get_resource_mut::<Schedules>()?;
// We need to call .into_inner here to satisfy the borrow checker:
// it can reason about reborrows using ordinary references but not the `Mut` smart pointer.
schedules.into_inner().get_mut(&label)
schedules.into_inner().get_mut(label)
}

/// Applies the function to the [`Schedule`] associated with `label`.
Expand All @@ -874,13 +880,14 @@ impl App {
label: impl ScheduleLabel,
f: impl FnOnce(&mut Schedule),
) -> &mut Self {
let label = label.intern();
let mut schedules = self.world.resource_mut::<Schedules>();

if schedules.get(&label).is_none() {
schedules.insert(Schedule::new(label.dyn_clone()));
if schedules.get(label).is_none() {
schedules.insert(Schedule::new(label));
}

let schedule = schedules.get_mut(&label).unwrap();
let schedule = schedules.get_mut(label).unwrap();
// Call the function f, passing in the schedule retrieved
f(schedule);

Expand Down Expand Up @@ -1008,6 +1015,8 @@ pub struct AppExit;

#[cfg(test)]
mod tests {
use std::marker::PhantomData;

use bevy_ecs::{
schedule::{OnEnter, States},
system::Commands,
Expand Down Expand Up @@ -1104,4 +1113,107 @@ mod tests {
app.world.run_schedule(OnEnter(AppState::MainMenu));
assert_eq!(app.world.entities().len(), 2);
}

#[test]
fn test_derive_app_label() {
use super::AppLabel;
use crate::{self as bevy_app};

#[derive(AppLabel, Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]
struct UnitLabel;

#[derive(AppLabel, Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]
struct TupleLabel(u32, u32);

#[derive(AppLabel, Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]
struct StructLabel {
a: u32,
b: u32,
}

#[derive(AppLabel, Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]
struct EmptyTupleLabel();

#[derive(AppLabel, Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]
struct EmptyStructLabel {}

#[derive(AppLabel, Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]
enum EnumLabel {
#[default]
Unit,
Tuple(u32, u32),
Struct {
a: u32,
b: u32,
},
}

#[derive(AppLabel, Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]
struct GenericLabel<T>(PhantomData<T>);

assert_eq!(UnitLabel.intern(), UnitLabel.intern());
assert_eq!(EnumLabel::Unit.intern(), EnumLabel::Unit.intern());
assert_ne!(UnitLabel.intern(), EnumLabel::Unit.intern());
assert_ne!(UnitLabel.intern(), TupleLabel(0, 0).intern());
assert_ne!(EnumLabel::Unit.intern(), EnumLabel::Tuple(0, 0).intern());

assert_eq!(TupleLabel(0, 0).intern(), TupleLabel(0, 0).intern());
assert_eq!(
EnumLabel::Tuple(0, 0).intern(),
EnumLabel::Tuple(0, 0).intern()
);
assert_ne!(TupleLabel(0, 0).intern(), TupleLabel(0, 1).intern());
assert_ne!(
EnumLabel::Tuple(0, 0).intern(),
EnumLabel::Tuple(0, 1).intern()
);
assert_ne!(TupleLabel(0, 0).intern(), EnumLabel::Tuple(0, 0).intern());
assert_ne!(
TupleLabel(0, 0).intern(),
StructLabel { a: 0, b: 0 }.intern()
);
assert_ne!(
EnumLabel::Tuple(0, 0).intern(),
EnumLabel::Struct { a: 0, b: 0 }.intern()
);

assert_eq!(
StructLabel { a: 0, b: 0 }.intern(),
StructLabel { a: 0, b: 0 }.intern()
);
assert_eq!(
EnumLabel::Struct { a: 0, b: 0 }.intern(),
EnumLabel::Struct { a: 0, b: 0 }.intern()
);
assert_ne!(
StructLabel { a: 0, b: 0 }.intern(),
StructLabel { a: 0, b: 1 }.intern()
);
assert_ne!(
EnumLabel::Struct { a: 0, b: 0 }.intern(),
EnumLabel::Struct { a: 0, b: 1 }.intern()
);
assert_ne!(
StructLabel { a: 0, b: 0 }.intern(),
EnumLabel::Struct { a: 0, b: 0 }.intern()
);
assert_ne!(
StructLabel { a: 0, b: 0 }.intern(),
EnumLabel::Struct { a: 0, b: 0 }.intern()
);
assert_ne!(StructLabel { a: 0, b: 0 }.intern(), UnitLabel.intern(),);
assert_ne!(
EnumLabel::Struct { a: 0, b: 0 }.intern(),
EnumLabel::Unit.intern()
);

assert_eq!(
GenericLabel::<u32>(PhantomData).intern(),
GenericLabel::<u32>(PhantomData).intern()
);
assert_ne!(
GenericLabel::<u32>(PhantomData).intern(),
GenericLabel::<u64>(PhantomData).intern()
);
}
}
26 changes: 13 additions & 13 deletions crates/bevy_app/src/main_schedule.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{App, Plugin};
use bevy_ecs::{
schedule::{ExecutorKind, Schedule, ScheduleLabel},
schedule::{ExecutorKind, InternedScheduleLabel, Schedule, ScheduleLabel},
system::{Local, Resource},
world::{Mut, World},
};
Expand Down Expand Up @@ -105,21 +105,21 @@ pub struct Last;
#[derive(Resource, Debug)]
pub struct MainScheduleOrder {
/// The labels to run for the [`Main`] schedule (in the order they will be run).
pub labels: Vec<Box<dyn ScheduleLabel>>,
pub labels: Vec<InternedScheduleLabel>,
}

impl Default for MainScheduleOrder {
fn default() -> Self {
Self {
labels: vec![
Box::new(First),
Box::new(PreUpdate),
Box::new(StateTransition),
Box::new(RunFixedUpdateLoop),
Box::new(Update),
Box::new(SpawnScene),
Box::new(PostUpdate),
Box::new(Last),
First.intern(),
PreUpdate.intern(),
StateTransition.intern(),
RunFixedUpdateLoop.intern(),
Update.intern(),
SpawnScene.intern(),
PostUpdate.intern(),
Last.intern(),
],
}
}
Expand All @@ -133,7 +133,7 @@ impl MainScheduleOrder {
.iter()
.position(|current| (**current).eq(&after))
.unwrap_or_else(|| panic!("Expected {after:?} to exist"));
self.labels.insert(index + 1, Box::new(schedule));
self.labels.insert(index + 1, schedule.intern());
}
}

Expand All @@ -148,8 +148,8 @@ impl Main {
}

world.resource_scope(|world, order: Mut<MainScheduleOrder>| {
for label in &order.labels {
let _ = world.try_run_schedule(&**label);
for &label in &order.labels {
let _ = world.try_run_schedule(label);
}
});
}
Expand Down
9 changes: 5 additions & 4 deletions crates/bevy_derive/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,12 +201,13 @@ pub fn derive_enum_variant_meta(input: TokenStream) -> TokenStream {

/// Generates an impl of the `AppLabel` trait.
///
/// This works only for unit structs, or enums with only unit variants.
/// You may force a struct or variant to behave as if it were fieldless with `#[app_label(ignore_fields)]`.
#[proc_macro_derive(AppLabel, attributes(app_label))]
/// This does not work for unions.
#[proc_macro_derive(AppLabel)]
pub fn derive_app_label(input: TokenStream) -> TokenStream {
let input = syn::parse_macro_input!(input as syn::DeriveInput);
let mut trait_path = BevyManifest::default().get_path("bevy_app");
let mut dyn_eq_path = trait_path.clone();
trait_path.segments.push(format_ident!("AppLabel").into());
derive_label(input, &trait_path, "app_label")
dyn_eq_path.segments.push(format_ident!("DynEq").into());
derive_label(input, "AppLabel", &trait_path, &dyn_eq_path)
}
Loading

0 comments on commit a830530

Please sign in to comment.