[Merged by Bors] - Simplify design for `*Label`s #4957

JoJoJet · 2022-06-07T08:21:07Z

Objective

Closes Simplify design for *Label types #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 Remove the ability to use strings as labels #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.

crates/bevy_ecs/src/schedule/state.rs

infmagic2047 · 2022-06-07T10:03:48Z

I have some concerns over the design here:

The use of &'static str looks worrying to me. I imagine runtime labels will be useful for things like scripting or editor support, and having to leak memory is bad. If the IntoLabel traits are derived in almost all cases, can we use a simple integer in the second field of label here (and generate it automatically with the macro)?
Somewhat related to the point above. I find it important to be able to customize Debug impls of labels. With this change, the Debug string of labels are determined by the as_str() function, which can be a surprising place for users.
To be honest I am not convinced that we need such a change. Label comparisons mostly happens when the system schedule changes, which should be very infrequent. Do you have benchmarks showing the performance difference?
I suggest keeping the old trait name *Label, and use another name (like *LabelId) for the struct, to reduce disruption for users.

alice-i-cecile · 2022-06-07T15:17:41Z

I imagine runtime labels will be useful for things like scripting or editor support, and having to leak memory is bad.

Hmm. I suspect we will want this for editor support, but I'm not actually convinced that there's a higher memory cost to leaking the memory versus just allocating a static string at compile time.

Somewhat related to the point above. I find it important to be able to customize Debug impls of labels. With this change, the Debug string of labels are determined by the as_str() function, which can be a surprising place for users.

Interesting, that's a bit surprising to me. Can you explain a bit more @infmagic2047?

To be honest I am not convinced that we need such a change. Label comparisons mostly happens when the system schedule changes, which should be very infrequent. Do you have benchmarks showing the performance difference?

I would also like to see benchmarks. This will primarily show up in complex schedule intialization. I think that adding such a benchmark is good to do regardless; I'd split this out into a seperate PR.

It also might show up in compile times, so I'd check that.

I suggest keeping the old trait name *Label, and use another name (like *LabelId) for the struct, to reduce disruption for users.

100% agree. The existing names are clearer, and there's no need to increase churn.

alice-i-cecile · 2022-06-07T15:19:38Z

crates/bevy_app/src/lib.rs


 /// The names of the default [`App`] stages.
 ///
 /// The relative [`Stages`](bevy_ecs::schedule::Stage) are added by [`App::add_default_stages`].
-#[derive(Debug, Hash, PartialEq, Eq, Clone, StageLabel)]
+#[derive(Debug, Hash, PartialEq, Eq, Clone, IntoStageLabel)]


As part of these changes, we should get rid of all of the extra derives on these types.

My first impression is that we should have a blanket Debug impl, remove Hash, and keep PartialEq, Eq and Clone.

I didn't want to break any user code unnecessarily, and I don't see how the extra derives are hurting anyone. What if we only remove them from private definitions?

alice-i-cecile

Definitely revert the SystemLabel -> IntoSystemLabel change; it's making this PR very tricky to review due to all of the noise. Initial impressions are positive though; I don't think there's any serious loss of functionality.

infmagic2047 · 2022-06-07T16:26:32Z

I imagine runtime labels will be useful for things like scripting or editor support, and having to leak memory is bad.

Hmm. I suspect we will want this for editor support, but I'm not actually convinced that there's a higher memory cost to leaking the memory versus just allocating a static string at compile time.

Now that I think more about it, leaking should be fine in most cases. Plugins which need runtime labels can maintain a cache of &'static str, so they won't leak the same string many times.

Somewhat related to the point above. I find it important to be able to customize Debug impls of labels. With this change, the Debug string of labels are determined by the as_str() function, which can be a surprising place for users.

Interesting, that's a bit surprising to me. Can you explain a bit more @infmagic2047?

Well, in my case it's mostly a desire to have distinguishable StageLabel names in diagnostics and similar places. In my code there are stages GameStage::Update and UiStage::Update along with the builtin CoreStage::Update, and they all show up as Update with the default Debug impl.

That said, my use case can be fulfilled by modifying the derive macro to add type name as prefix (so .as_str() returns something like MyEnum::Variant), and I can't think of any other reasonable use cases, so it should be fine.

In fact, there is another reason for printing MyEnum::Variant in diagnostics: it is easier to locate the label in code.

JoJoJet · 2022-06-07T16:51:27Z

The use of &'static str looks worrying to me. I imagine runtime labels will be useful for things like scripting or editor support, and having to leak memory is bad. If the IntoLabel traits are derived in almost all cases, can we use a simple integer in the second field of label here (and generate it automatically with the macro)?

This was actually the original design I had for this before publishing the issue. It's elegant, but the problem is that then the label becomes completely opaque, and you can't even print it at all. You can store an integer and the string, but that's pointless since you can just use the string+TypeID for equality.

We can always do string interning for editor support, but I think Alice already convinced you.

Somewhat related to the point above. I find it important to be able to customize Debug impls of labels. With this change, the Debug string of labels are determined by the as_str() function, which can be a surprising place for users.

In practice I don't think there's much use for arbitrary Debug impls, in most cases you just want constant text. I can rename the as_str() fn to make it more clear though.

I suggest keeping the old trait name *Label, and use another name (like *LabelId) for the struct, to reduce disruption for users.

Makes sense. Done.

I would also like to see benchmarks. This will primarily show up in complex schedule intialization. I think that adding such a benchmark is good to do regardless; I'd split this out into a seperate PR.
It also might show up in compile times, so I'd check that.

I agree that benchmarks are valuable but I don't really know how to benchmark bevy. Is someone willing to help me out on discord?

Well, in my case it's mostly a desire to have distinguishable StageLabel names in diagnostics and similar places. In my code there are stages GameStage::Update and UiStage::Update along with the builtin CoreStage::Update, and they all show up as Update with the default Debug impl.

Good idea, on it.

JoJoJet · 2022-06-07T17:17:12Z

Also, I think the Dyn{Eq, Hash, Clone} types need a new home. Is that within the scope of this PR?

alice-i-cecile · 2022-06-07T17:17:53Z

Also, I think the Dyn{Eq, Hash, Clone} types need a new home. Is that within the scope of this PR?

Seems reasonable, but make a new PR :)

alice-i-cecile · 2022-06-07T17:18:27Z

I agree that benchmarks are valuable but I don't really know how to benchmark bevy. Is someone willing to help me out on discord?

100%. Come say hi in #engine-dev and people will be able to help you out :)

crates/bevy_ecs/src/schedule/stage.rs

alice-i-cecile

Looking good! To do:

Address small nits.
Collapse the define_label! macro back down to a single argument.
Add and run some quick schedule construction benchmarks (another PR is probably ideal).

crates/bevy_ecs/src/schedule/state.rs

crates/bevy_macro_utils/src/lib.rs

crates/bevy_utils/src/label.rs

alice-i-cecile · 2022-06-07T19:19:03Z

As commented on Discord; I meant another PR.

alice-i-cecile

Approved pending not-terrible benchmark results :) I think this is a clearer and simpler way to get the same benefits.

JoJoJet · 2022-06-07T20:45:15Z

Benchmarks: #4961

JoJoJet · 2022-06-08T03:15:38Z

The benchmarks are very promising (located here)

Old labels

# of Systems	No Deps	W/ Deps
100	747.62 us	3.4763 ms
500	1.2992 ms	88.073 ms
1000	1.9776 ms	420.98 ms

New Labels

# of Systems	No Deps	W/ Deps
100	741.35 us	2.4197 ms
500	1.2930 ms	61.667 ms
1000	1.9627 ms	299.96 ms

% Change

# of Systems	No Deps	W/ Deps
100	-0.5121%	-30.201%
500	-0.5987%	-29.982%
1000	-0.9030%	-28.746%

alice-i-cecile · 2022-06-08T03:29:12Z

Excellent! Those are the sort of numbers I like to see. This is a one-time cost at app-startup (until #279 lands), so it's not as critical as some of the other perf optimizations, but it's still really nice to shave down app initialization time when we can.

crates/bevy_macro_utils/src/lib.rs

Rewrite {System,Stage,*}Label to use a simpler design. *Label is now a lightweight, copy-able struct. You derive `Into*Label` on your custom labels.

Update lib.rs Update lib.rs

Rename `*Label` -> `*LabelId` Rename `Into*Label` -> `*Label`

Removed the private types `StateCallback` and `StateRunCriteriaLabel`. They were added to every single state-system, but they don't appear to be "read" anywhere.

crates/bevy_ecs/src/lib.rs

TheRawMeatball

The usage of strings is slightly concerning to me, but given the derive seems to prevent any major footguns I'm okay with merging this and revisiting it when dynamic plugins are more of a concern.

Ratysz

I'm with @TheRawMeatball.

Though, when we first came up with this I don't remember us thinking much about dynamic plugins either.

alice-i-cecile · 2022-07-14T18:02:55Z

bors r+

# Objective - Closes #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 #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.

bors · 2022-07-14T18:14:11Z

Build failed:

build (stable, macos-latest)

alice-i-cecile · 2022-07-14T18:22:44Z

bors retry

# Objective - Closes #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 #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.

bors · 2022-07-14T18:47:41Z

Pull request successfully merged into main.

Build succeeded:

# 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.

alice-i-cecile 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 Jun 7, 2022

JoJoJet commented Jun 7, 2022

View reviewed changes

crates/bevy_ecs/src/schedule/state.rs Show resolved Hide resolved

alice-i-cecile reviewed Jun 7, 2022

View reviewed changes

alice-i-cecile requested changes Jun 7, 2022

View reviewed changes

alice-i-cecile reviewed Jun 7, 2022

View reviewed changes

crates/bevy_ecs/src/schedule/stage.rs Show resolved Hide resolved

alice-i-cecile requested changes Jun 7, 2022

View reviewed changes

alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Jun 7, 2022

alice-i-cecile approved these changes Jun 7, 2022

View reviewed changes

alice-i-cecile mentioned this pull request Jun 8, 2022

[Merged by Bors] - Add benchmarks for schedule dependency resolution #4961

Closed

alice-i-cecile removed the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Jun 8, 2022

cart reviewed Jun 8, 2022

View reviewed changes

crates/bevy_macro_utils/src/lib.rs Show resolved Hide resolved

JoJoJet force-pushed the light-labels branch from c10f1b3 to ae9a836 Compare June 28, 2022 22:30

JoJoJet added 4 commits July 13, 2022 20:08

use a simpler design for *Label

a9fa654

Rewrite {System,Stage,*}Label to use a simpler design. *Label is now a lightweight, copy-able struct. You derive `Into*Label` on your custom labels.

Update the workspace to use the new Label API

fecd697

Update lib.rs Update lib.rs

revert *Label rename

bab032c

Rename `*Label` -> `*LabelId` Rename `Into*Label` -> `*Label`

Remove unused state types

8c04a04

Removed the private types `StateCallback` and `StateRunCriteriaLabel`. They were added to every single state-system, but they don't appear to be "read" anywhere.

fix a rebase error

b13222e

JoJoJet force-pushed the light-labels branch from 2e5d4e2 to b13222e Compare July 14, 2022 02:26

alice-i-cecile reviewed Jul 14, 2022

View reviewed changes

crates/bevy_ecs/src/lib.rs Outdated Show resolved Hide resolved

Remove *LabelId types from prelude

7d94ca2

alice-i-cecile requested a review from TheRawMeatball July 14, 2022 04:07

TheRawMeatball approved these changes Jul 14, 2022

View reviewed changes

Ratysz approved these changes Jul 14, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 14, 2022

bors bot changed the title ~~Simplify design for *Labels~~ [Merged by Bors] - Simplify design for *Labels Jul 14, 2022

bors bot closed this Jul 14, 2022

JoJoJet deleted the light-labels branch July 14, 2022 18:49

This was referenced Jul 18, 2022

SystemLabel proc-macro no longer allows for generic enums #5362

Closed

[Merged by Bors] - Add attribute to ignore fields of derived labels #5366

Closed

This was referenced Jul 21, 2022

Implement Label traits for their boxed variants #2505

Closed

schedule v3 ("stageless") #4391

Closed

maniwani mentioned this pull request Aug 3, 2022

Revisit labels again #5569

Closed

JoJoJet mentioned this pull request Aug 7, 2022

Optimize label comparisons and remove restrictions on types that can be labels #5377

Closed

JoJoJet mentioned this pull request Aug 16, 2022

Remove restrictions on Label types #5715

Closed

maniwani mentioned this pull request Nov 24, 2022

[Merged by Bors] - Add bevy_ecs::schedule_v3 module #6587

Closed

7 tasks

maniwani mentioned this pull request Feb 20, 2023

Use string interning to avoid boxing labels #7760

Closed

JoJoJet mentioned this pull request Aug 5, 2023

Replace all labels with interned labels #7762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Simplify design for `*Label`s #4957

[Merged by Bors] - Simplify design for `*Label`s #4957

JoJoJet commented Jun 7, 2022 •

edited

Loading

infmagic2047 commented Jun 7, 2022

alice-i-cecile commented Jun 7, 2022

alice-i-cecile Jun 7, 2022

JoJoJet Jun 7, 2022

alice-i-cecile left a comment

infmagic2047 commented Jun 7, 2022

JoJoJet commented Jun 7, 2022 •

edited

Loading

JoJoJet commented Jun 7, 2022

alice-i-cecile commented Jun 7, 2022

alice-i-cecile commented Jun 7, 2022

alice-i-cecile left a comment

alice-i-cecile commented Jun 7, 2022

alice-i-cecile left a comment

JoJoJet commented Jun 7, 2022 •

edited

Loading

JoJoJet commented Jun 8, 2022

alice-i-cecile commented Jun 8, 2022

TheRawMeatball left a comment

Ratysz left a comment

alice-i-cecile commented Jul 14, 2022

bors bot commented Jul 14, 2022

alice-i-cecile commented Jul 14, 2022

bors bot commented Jul 14, 2022

[Merged by Bors] - Simplify design for *Labels #4957

[Merged by Bors] - Simplify design for *Labels #4957

Conversation

JoJoJet commented Jun 7, 2022 • edited Loading

Objective

Solution

Changelog

Migration Guide

Questions for later

infmagic2047 commented Jun 7, 2022

alice-i-cecile commented Jun 7, 2022

alice-i-cecile Jun 7, 2022

Choose a reason for hiding this comment

JoJoJet Jun 7, 2022

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

infmagic2047 commented Jun 7, 2022

JoJoJet commented Jun 7, 2022 • edited Loading

JoJoJet commented Jun 7, 2022

alice-i-cecile commented Jun 7, 2022

alice-i-cecile commented Jun 7, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Jun 7, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

JoJoJet commented Jun 7, 2022 • edited Loading

JoJoJet commented Jun 8, 2022

Old labels

New Labels

% Change

alice-i-cecile commented Jun 8, 2022

TheRawMeatball left a comment

Choose a reason for hiding this comment

Ratysz left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Jul 14, 2022

bors bot commented Jul 14, 2022

alice-i-cecile commented Jul 14, 2022

bors bot commented Jul 14, 2022

[Merged by Bors] - Simplify design for `*Label`s #4957

[Merged by Bors] - Simplify design for `*Label`s #4957

JoJoJet commented Jun 7, 2022 •

edited

Loading

JoJoJet commented Jun 7, 2022 •

edited

Loading

JoJoJet commented Jun 7, 2022 •

edited

Loading