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.

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

Remove restrictions on Label types #5715

Closed
wants to merge 21 commits into from
Closed

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Aug 16, 2022

Redo of #5377

Background

Originally, label traits were essentially like Any, but with extra capabilities.

  • This was unnecessarily slow, as it required boxing every label even though the vast majority are simply unit structs or C-style enums.
  • Ignited months-long unresolved debates as to whether Box<dyn *Label> should implement the trait.
  • Required a long list of derives for every implementer.
  • O(?) comparisons due to dynamic dispatch.

In #4957, labels were simplified: they are represented as just a TypeId and a string literal.

  • This sped up label comparisons and dependency resolution considerably, as allocations and dynamic dispatch are no longer necessary.
  • Makes the use of labels more ergonomic, as their type-erased form is Copy. Additionally, none of the extra derives are required.
  • It introduced major restrictions on what types can be labels: labels can only have names, not data.
  • In most cases, this change was completely invisible to users.

Objective

  • Remove the restrictions: allow labels to store any data that they want.
  • Make complex labels faster than before [Merged by Bors] - Simplify design for *Labels #4957.
  • Don't pessimize simple label types -- they should be just as fast as before this PR.
  • Make comparisons O(1).
  • Optimize the stack size of type-erased labels.
  • Keep the user-facing API as simple as possible.

Solution

  • Allow arbitrary debug formatting by writing to an &mut Formatter.
  • For comparisons, use a u64.
    • Types that are 8 bytes or smaller can be encoded and stored inline.
    • For larger types, intern them and store their index. This also avoids duplicate allocations for identical labels.
    • Both of these cases are supported in the derive macro.
  • Keep the stack size low by storing type-related data behind a pointer to an associated const.

Examples

Basic labels get converted to an integer and stored on the stack.

#[derive(SystemLabel)]
enum MyLabel {
    One,
    Two,
}

//
// Derive expands to...

impl IntoSystemLabel for MyLabel {
    fn data(&self) -> u64 {
        match self {
            Self::One => 0,
            Self::Two => 1,
        }
    }
    fn fmt(data: u64, f: &mut fmt::Formatter) -> fmt::Result {
        match data {
            0 => write!(f, "MyLabel::One"),
            1 => write!(f, "MyLabel::Two"),
            _ => Err(fmt::Error),
        }
    }
}

Complex labels get interned, with their index stored on the stack.

#[derive(Debug, Clone, PartialEq, Eq, Hash, SystemLabel)]
#[system_label(intern)]
pub struct MyLabel<T>(Vec<T>);

//
// Derive expands to...

use bevy_utils::{AnyInterner, InternGuard};

// Global label interner for `MyLabel<T>`, for any `T`.
static MAP: AnyInterner = AnyInterner::new();

impl<T: Debug + Clone + Hash + Eq + Send + Sync + 'static> IntoSystemLabel for MyLabel<T> {
    fn data(&self) -> u64 {
        MAP.intern(self)
    }
    fn fmt(idx: u64, f: &mut Formatter) -> fmt::Result {
        let val = MAP.get::<Self>(idx).ok_or(fmt::Error)?;
        write!(f, "{val:?}")
    }
}

Benchmarks

Relative to before this PR.

  • Using stack-allocated labels.
# of Systems No deps % Change W/ deps % Change
100 235.51 us -0.9142% 1.8465 ms -9.2033%
500 782.39 us +0.1074% 61.109 ms -15.269%
1000 1.4279 ms +1.0771% 310.41 ms -13.131%
  • Heap-allocating everything.
# of Systems No deps % Change W/ deps % Change
100 233.20 us +1.0003% 2.0064 ms +1.5735%
500 789.80 us -0.9972% 64.846 ms -6.1881%
1000 1.4699 ms -2.0041% 318.41 ms -5.2037%

Notes

  • This PR adds parking_lot and indexmap as dependencies for bevy_utils.
    • indexmap only has one dependency (hashbrown), which is already a dependency of bevy_utils.

Future Work

  • We can clean up unused memory by emptying out the global interners after dependency resolution is complete. This would break debug formatting, but comparisons would still work perfectly.
  • Downcasting can easily be implemented, but the API decisions are left for a future PR.

Changelog

  • Made the internal representation of labels more flexible.
  • Added support for complex label types by interning them.

Migration Guide

The Label family of traits (SystemLabel, StageLabel, etc) is no longer object safe.

  • Any use of Box<dyn *Label> should be replaced with *LabelId - this family of types serves the same purpose but is Copy and does not usually require allocations.
  • Any use of &dyn *Label should be replaced with impl *Label if possible, or *LabelId if you cannot use generics.

For manual implementers of this family of traits, you should now implement the corresponding Into*Label trait.

enum GameLabel {
    Input,
    Physics,
}

// Before:

impl SystemLabel for GameLabel {
    fn as_str(&self) -> &'static str {
        match self {
            Self::Input => "GameLabel::Input",
            Self::Physics => "GameLabel::Physics",
        }
    }
}

// After:

impl IntoSystemLabel for GameLabel {
    fn data(&self) -> u64 {
        match self {
            Self::Input => 0,
            Self::Physics => 1,
        }
    }
    fn fmt(data: u64, f: &mut Formatter) -> fmt::Result {
        match data {
            0 => write!(f, "GameLabel::Input"),
            1 => write!(f, "GameLabel::Physics"),
            _ => Err(fmt::Error),
        }
    }
}

@Weibye Weibye added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 18, 2022
@alice-i-cecile
Copy link
Member

This is an exceptional writeup, and I agree with the motivations here. A bit annoying to increase the complexity of this niche area further, but I think the expressiveness and performance will be worth it.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work. Exceptional docs, error messages and general technical proficiency here.

There's a few nits and open questions for me, but I'm generally on board here.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Sep 20, 2022
@james7132 james7132 self-requested a review October 20, 2022 07:27
@hymm hymm self-requested a review December 23, 2022 06:45
@alice-i-cecile
Copy link
Member

@JoJoJet is this still relevant?

@JoJoJet
Copy link
Member Author

JoJoJet commented Apr 19, 2023

Nah, I have no intention to update this.

@JoJoJet JoJoJet deleted the label-intern branch September 20, 2023 05:19
github-merge-queue bot pushed a commit that referenced this pull request Oct 25, 2023
# 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]>
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

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


This also fixes bevyengine#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
bevyengine#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]>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

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


This also fixes bevyengine#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
bevyengine#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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants