Replace all labels with interned labels #7762

geieredgar · 2023-02-20T13:41:42Z

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 run_if for SystemConfigs via anonymous system sets #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 Schedules, 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.

alice-i-cecile · 2023-02-20T15:29:30Z

Neat! I've added Controversial to all three of these PRs, to make sure the ECS SMEs consciously choose between them.

crates/bevy_ecs/src/schedule/set.rs

JoJoJet

I've scanned over this quickly, and it definitely seems like a promising approach. Very similar to #7760 but potentially preferable.

Just for clarity, what are the differences between SystemSet::dyn_intern, Intern::intern, and IntoInterned::into_interned? Is it possible to simplify or combine any of these methods some way?

geieredgar · 2023-07-22T13:13:55Z

@JoJoJet I've replaced SystemSet::dyn_intern, Intern::intern and IntoInterned::into_interned with the standard Into<Interned<T>>::into.

The functions that took impl AsRef<dyn ScheduleLabel> as parameter now take impl Into<InternedScheduleLabel>, which is implemented by the derive-macro for ScheduleLabel for T and &T, so it almost works the same as before.

I've also introduced a new trait and method StaticRef::static_ref which can be used to provide a consistent static reference to a value equal to self. This is used to further optimize the interning process, since interning a value for which a static reference can be provided doesn't need to lock the internal hash set anymore.

JoJoJet

Can you also switch AppLabel to use interned labels? It still uses pseudo-stringly typed labels, and I'd rather not leave that relic around.

crates/bevy_utils/src/intern.rs

crates/bevy_app/src/app.rs

crates/bevy_ecs/src/schedule/set.rs

crates/bevy_utils/src/intern.rs

geieredgar · 2023-07-23T00:14:44Z

@JoJoJet Regarding AppLabel, is it okay if I do the switch in a follow-up PR?

JoJoJet · 2023-07-23T02:03:34Z

I guess that's fine, but is there a reason not to do it now? As far as I know, you'd just have to change a define_label! to define_interned_label!, and update some callsites.

geieredgar · 2023-07-23T10:34:22Z

OK, after giving it a second thought, I've decided to do the AppLabel switch inside this PR.

I also changed AnonymousSet to not use globally unique ids, and use ids only unique inside the given Schedule. This means that we are not increasing the leaked memory indefinitely when the user creates Schedules repeatedly. Instead the amount of memory leaked by AnonymousSets depends now on the Schedule that contained the most anonymous sets.

JoJoJet · 2023-07-23T12:08:08Z

Thank you for implementing my suggestions. I'll try to review this more closely today.

JoJoJet · 2023-07-28T01:14:26Z

I finally got around to this. I have a couple more ideas for how the API can be refined. Rather than going back-and-forth, I opened a PR to your branch: geieredgar#1.

As always, feel free to voice any concerns you have with my suggestions.

JoJoJet

I think this PR is in a good state now! I just have a minor documentation suggestion.

crates/bevy_utils/src/intern.rs

github-actions · 2023-07-28T15:22:16Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

JoJoJet · 2023-07-28T16:03:14Z

Interned does and probably should not implement SystemSet, I think for the same reasons that Box doesn't, but I haven't looked up all the discussions about that. Generally, Interned(x) and x should not be treated as the same thing when it comes to hash values and comparisons.

This can be removed from the cons list, and added to the pros.

Diddykonga · 2023-10-01T09:32:15Z

Adding these here:
https://www.reddit.com/r/rust/comments/zfi61k/comment/izc2guh/?utm_source=share&utm_medium=web2x&context=3
https://www.reddit.com/r/rust/comments/zfi61k/comment/izceo6f/?utm_source=share&utm_medium=web2x&context=3

geieredgar · 2023-10-01T09:48:35Z

@nicopap After finding rust-lang/rust#106447 I think you are correct that this approach was unsound, because two equal labels could yield two different pointers. Therefore I removed the static_ref optimization completely, so interning will always go through the internal HashMap, guaranteeing that equal labels always return equal pointers.

I also believe that two different labels cannot yield the same fat pointer when leaking, because then a.dyn_eq(b) would return true. And currently ptr::eq does compare both the address and the metadata, so it should work. However, as shown in rust-lang/rust#106447 this behavior could change in the future.

geieredgar · 2023-10-01T12:17:55Z

I've replaced the use of std::ptr::eq with a custom equality comparison, which first checks for equal type ids and then for equal data pointers. Therefore, we don't rely on the vtable pointers anymore.

alice-i-cecile · 2023-10-18T19:06:59Z

@geieredgar this is in the milestone for 0.12, slated for this Saturday. I'm going to do a full review now, but I'm hopeful we can get this in and ship it. Can you resolve merge conflicts?

alice-i-cecile

Really cool stuff! The changes in schedule.rs and intern.rs are the only non-trivial changes: focus your review efforts there.

The breaking changes are acceptable and well-documented, the motivation is sound, I basically understand how this works, and this is incredibly well-tested. I'm comfortable approving this!

geieredgar · 2023-10-18T20:12:02Z

Conflicts are resolved.

cart

I'm happy with this approach!

cart · 2023-10-25T20:38:52Z

crates/bevy_utils/src/intern.rs

+ }
+
+ fn ref_eq(&self, other: &Self) -> bool {
+ self.len() == other.len() && self.as_ptr() == other.as_ptr()


As an optimization, seems worth it to flip the order here to short-circuit on the ptr comparison? I think two interned strs are more likely to have different pointers than different lengths.

Just pushed as I doubt this will be controversial (and in the interest of moving forward).

I'll leave this open in case anyone wants to discuss.

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

geieredgar mentioned this pull request Feb 20, 2023

Use string interning to avoid boxing labels #7760

Closed

JoJoJet self-requested a review February 20, 2023 14:50

alice-i-cecile requested a review from maniwani February 20, 2023 15:28

DanielJin21 mentioned this pull request Mar 4, 2023

Remove impl ScheduleLabel for Box<dyn ScheduleLabel> #7556

Closed

hymm mentioned this pull request Apr 17, 2023

Simplify world schedule methods #8403

Merged

JoJoJet reviewed Jul 20, 2023

View reviewed changes

crates/bevy_ecs/src/schedule/set.rs Outdated Show resolved Hide resolved

JoJoJet reviewed Jul 20, 2023

View reviewed changes

geieredgar force-pushed the interned-labels branch from 9d7efbf to 3b3b4e3 Compare July 22, 2023 13:12

geieredgar force-pushed the interned-labels branch from 3b3b4e3 to 35d535a Compare July 22, 2023 13:24

JoJoJet reviewed Jul 22, 2023

View reviewed changes

crates/bevy_utils/src/intern.rs Outdated Show resolved Hide resolved

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved

JoJoJet reviewed Jul 22, 2023

View reviewed changes

crates/bevy_ecs/src/schedule/set.rs Outdated Show resolved Hide resolved

JoJoJet reviewed Jul 22, 2023

View reviewed changes

crates/bevy_utils/src/intern.rs Outdated Show resolved Hide resolved

geieredgar changed the title ~~Replace boxed labels with interned labels~~ Replace all labels with interned labels Jul 23, 2023

geieredgar force-pushed the interned-labels branch from b9fb74f to b91bee9 Compare July 28, 2023 11:42

JoJoJet approved these changes Jul 28, 2023

View reviewed changes

crates/bevy_utils/src/intern.rs Outdated Show resolved Hide resolved

JoJoJet added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 28, 2023

JoJoJet requested a review from alice-i-cecile July 28, 2023 16:32

nicopap self-requested a review October 1, 2023 08:42

alice-i-cecile removed 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 Oct 1, 2023

geieredgar added 2 commits October 1, 2023 10:53

Merge branch 'main' into interned-labels

a05e6f1

Fix errors after merging

83c2b8b

alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Oct 1, 2023

geieredgar added 2 commits October 1, 2023 11:19

Remove static_ref optimization

563e392

Add unit distinction test

6a3760f

geieredgar added 4 commits October 1, 2023 12:30

Remove proc-macro2 dependency (leftover from static_ref optimization)

2af81e9

Add and use ref_eq instead of relying on std::ptr::eq

846734a

Fix missing ;

8d10df7

Fix test

4b1a9fd

geieredgar added 2 commits October 1, 2023 14:21

Fix doctest

813dd42

Fix doc link

83ee170

alice-i-cecile requested a review from JoJoJet October 1, 2023 13:16

alice-i-cecile approved these changes Oct 18, 2023

View reviewed changes

Merge branch 'main' into interned-labels

2d75e20

cart approved these changes Oct 25, 2023

View reviewed changes

Flip comparison order

e2acc8b

cart enabled auto-merge October 25, 2023 21:22

cart added this pull request to the merge queue Oct 25, 2023

Merged via the queue into bevyengine:main with commit a830530 Oct 25, 2023
21 checks passed

Vortex mentioned this pull request Nov 5, 2023

Bevy 0.12 support zkat/big-brain#86

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace all labels with interned labels #7762

Replace all labels with interned labels #7762

geieredgar commented Feb 20, 2023 •

edited

Loading

alice-i-cecile commented Feb 20, 2023

JoJoJet left a comment •

edited

Loading

geieredgar commented Jul 22, 2023 •

edited

Loading

JoJoJet left a comment

geieredgar commented Jul 23, 2023

JoJoJet commented Jul 23, 2023

geieredgar commented Jul 23, 2023 •

edited

Loading

JoJoJet commented Jul 23, 2023

JoJoJet commented Jul 28, 2023

JoJoJet left a comment

github-actions bot commented Jul 28, 2023

JoJoJet commented Jul 28, 2023

Diddykonga commented Oct 1, 2023

geieredgar commented Oct 1, 2023 •

edited

Loading

geieredgar commented Oct 1, 2023

alice-i-cecile commented Oct 18, 2023 •

edited

Loading

alice-i-cecile left a comment

geieredgar commented Oct 18, 2023

cart left a comment

cart Oct 25, 2023

cart Oct 25, 2023

cart Oct 25, 2023

Replace all labels with interned labels #7762

Replace all labels with interned labels #7762

Conversation

geieredgar commented Feb 20, 2023 • edited Loading

Objective

Solution

Pros

Cons

Changelog

Added

Changed

Removed

Migration guide

alice-i-cecile commented Feb 20, 2023

JoJoJet left a comment • edited Loading

Choose a reason for hiding this comment

geieredgar commented Jul 22, 2023 • edited Loading

JoJoJet left a comment

Choose a reason for hiding this comment

geieredgar commented Jul 23, 2023

JoJoJet commented Jul 23, 2023

geieredgar commented Jul 23, 2023 • edited Loading

JoJoJet commented Jul 23, 2023

JoJoJet commented Jul 28, 2023

JoJoJet left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 28, 2023

JoJoJet commented Jul 28, 2023

Diddykonga commented Oct 1, 2023

geieredgar commented Oct 1, 2023 • edited Loading

geieredgar commented Oct 1, 2023

alice-i-cecile commented Oct 18, 2023 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

geieredgar commented Oct 18, 2023

cart left a comment

Choose a reason for hiding this comment

cart Oct 25, 2023

Choose a reason for hiding this comment

cart Oct 25, 2023

Choose a reason for hiding this comment

cart Oct 25, 2023

Choose a reason for hiding this comment

geieredgar commented Feb 20, 2023 •

edited

Loading

JoJoJet left a comment •

edited

Loading

geieredgar commented Jul 22, 2023 •

edited

Loading

geieredgar commented Jul 23, 2023 •

edited

Loading

geieredgar commented Oct 1, 2023 •

edited

Loading

alice-i-cecile commented Oct 18, 2023 •

edited

Loading