Remove restrictions on Label types#5715
Closed
joseph-gio wants to merge 21 commits intobevyengine:mainfrom
Closed
Conversation
ac7cfba to
11a578c
Compare
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. |
alice-i-cecile
requested changes
Sep 3, 2022
Member
alice-i-cecile
left a comment
There was a problem hiding this comment.
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
approved these changes
Sep 19, 2022
Member
|
@JoJoJet is this still relevant? |
Member
Author
|
Nah, I have no intention to update this. |
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 <21144246+JoJoJet@users.noreply.github.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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 <21144246+JoJoJet@users.noreply.github.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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 <21144246+JoJoJet@users.noreply.github.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Redo of #5377
Background
Originally, label traits were essentially like
Any, but with extra capabilities.Box<dyn *Label>should implement the trait.In #4957, labels were simplified: they are represented as just a
TypeIdand a string literal.Copy. Additionally, none of the extra derives are required.Objective
*Labels #4957.Solution
&mut Formatter.u64.Examples
Basic labels get converted to an integer and stored on the stack.
Complex labels get interned, with their index stored on the stack.
Benchmarks
Relative to before this PR.
Notes
parking_lotandindexmapas dependencies forbevy_utils.indexmaponly has one dependency (hashbrown), which is already a dependency ofbevy_utils.Future Work
Changelog
Migration Guide
The
Labelfamily of traits (SystemLabel,StageLabel, etc) is no longer object safe.Box<dyn *Label>should be replaced with*LabelId- this family of types serves the same purpose but isCopyand does not usually require allocations.&dyn *Labelshould be replaced withimpl *Labelif possible, or*LabelIdif you cannot use generics.For manual implementers of this family of traits, you should now implement the corresponding
Into*Labeltrait.