[Merged by Bors] - Extract Resources into their own dedicated storage #4809

james7132 · 2022-05-20T06:12:05Z

Objective

At least partially addresses #6282.

Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be pub(crate) which isn't ideal.

This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a World.

Solution

Add Resources parallel to Tables and SparseSets and extract the functionality used by Archetype in it.
Remove unique_components from Archetype
Remove the pub(crate) on Archetype::components.
Remove ArchetypeId::RESOURCE
Remove Archetypes::resource and Archetypes::resource_mut

Changelog

Added: Resources type to store resources.
Added: Storages::resource
Removed: ArchetypeId::RESOURCE
Removed: Archetypes::resource and Archetypes::resources
Removed: Archetype::unique_components and Archetypes::unique_components_mut

Migration Guide

Resources have been moved to Resources under Storages in World. All code dependent on Archetype::unique_components(_mut) should access it via world.storages().resources() instead.

All APIs accessing the raw data of individual resources (mutable and read-only) have been removed as these APIs allowed for unsound unsafe code. All usages of these APIs should be changed to use World::{get, insert, remove}_resource.

alice-i-cecile

Very happy with the direction here; this is much cleaner.

A couple of small changes needed.

Finish the migration guide.
Add doc strings to Resources.

Optionally:

Refactor the common snippet noted above into a function.
Add doc strings to the methods on Resources.

crates/bevy_ecs/src/world/world_cell.rs

crates/bevy_ecs/src/storage/resource.rs

alice-i-cecile

@bevyengine/ecs-team This one is easy; reviews?

alice-i-cecile · 2022-06-07T04:27:59Z

@james7132 can you fix this up when you get the chance? I think the fixes should be straightforward, and I'd like to unblock #4955.

alice-i-cecile · 2022-06-11T16:16:30Z

Ping @BoxyUwU for attempt 5 to remember to review this 😂

BoxyUwU

5th times the charm I suppose

crates/bevy_ecs/src/storage/resource.rs

crates/bevy_ecs/src/world/mod.rs

Co-authored-by: Boxy <[email protected]>

alice-i-cecile · 2022-10-18T02:15:48Z

I think we can do those in follow-up PRs. It's useful, but not essential.

BoxyUwU

Honestly this is still kinda sketchy around Send/Sync but I am feeling like we ought to just merge this and move on because trying to get bevy to be sound with World: Send + Sync seems (to me) like a losing battle. Since this PR doesnt actually make anything worse we should just merge this and make World: !Send + !Sync hold later.

alice-i-cecile · 2022-10-24T13:26:43Z

This is ready: I agree with Boxy's assessment and think this makes reasoning about the internals of resources much nicer. Small perf improvements are a nice bonus.

bors r+

# Objective At least partially addresses #6282. Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be `pub(crate)` which isn't ideal. This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a `World`. ## Solution - Add `Resources` parallel to `Tables` and `SparseSets` and extract the functionality used by `Archetype` in it. - Remove `unique_components` from `Archetype` - Remove the `pub(crate)` on `Archetype::components`. - Remove `ArchetypeId::RESOURCE` - Remove `Archetypes::resource` and `Archetypes::resource_mut` --- ## Changelog Added: `Resources` type to store resources. Added: `Storages::resource` Removed: `ArchetypeId::RESOURCE` Removed: `Archetypes::resource` and `Archetypes::resources` Removed: `Archetype::unique_components` and `Archetypes::unique_components_mut` ## Migration Guide Resources have been moved to `Resources` under `Storages` in `World`. All code dependent on `Archetype::unique_components(_mut)` should access it via `world.storages().resources()` instead. All APIs accessing the raw data of individual resources (mutable *and* read-only) have been removed as these APIs allowed for unsound unsafe code. All usages of these APIs should be changed to use `World::{get, insert, remove}_resource`.

bors · 2022-10-24T14:11:32Z

Pull request successfully merged into main.

Build succeeded:

At least partially addresses bevyengine#6282. Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be `pub(crate)` which isn't ideal. This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a `World`. - Add `Resources` parallel to `Tables` and `SparseSets` and extract the functionality used by `Archetype` in it. - Remove `unique_components` from `Archetype` - Remove the `pub(crate)` on `Archetype::components`. - Remove `ArchetypeId::RESOURCE` - Remove `Archetypes::resource` and `Archetypes::resource_mut` --- Added: `Resources` type to store resources. Added: `Storages::resource` Removed: `ArchetypeId::RESOURCE` Removed: `Archetypes::resource` and `Archetypes::resources` Removed: `Archetype::unique_components` and `Archetypes::unique_components_mut` Resources have been moved to `Resources` under `Storages` in `World`. All code dependent on `Archetype::unique_components(_mut)` should access it via `world.storages().resources()` instead. All APIs accessing the raw data of individual resources (mutable *and* read-only) have been removed as these APIs allowed for unsound unsafe code. All usages of these APIs should be changed to use `World::{get, insert, remove}_resource`.

…4927) # Objective Archetype is a deceptively large type in memory. It stores metadata about which components are in which storage in multiple locations, which is only used when creating new Archetypes while moving entities. ## Solution Remove the redundant `Box<[ComponentId]>`s and iterate over the sparse set of component metadata instead. Reduces Archetype's size by 4 usizes (32 bytes on 64-bit systems), as well as the additional allocations for holding these slices. It'd seem like there's a downside that the origin archetype has it's component metadata iterated over twice when creating a new archetype, but this change also removes the extra `Vec<ArchetypeComponentId>` allocations when creating a new archetype which may amortize out to a net gain here. This change likely negatively impacts creating new archetypes with a large number of components, but that's a cost mitigated by the fact that these archetypal relationships are cached in Edges and is incurred only once for each edge created. ## Additional Context There are several other in-flight PRs that shrink Archetype: - #4800 merges the entities and rows Vecs together (shaves off 24 bytes per archetype) - #4809 removes unique_components and moves it to it's own dedicated storage (shaves off 72 bytes per archetype) --- ## Changelog Changed: `Archetype::table_components` and `Archetype::sparse_set_components` return iterators instead of slices. `Archetype::new` requires iterators instead of parallel slices/vecs. ## Migration Guide Do I still need to do this? I really hope people were not relying on the public facing APIs changed here.

# Objective Make core types in ECS smaller. The column sparse set in Tables is never updated after creation. ## Solution Create `ImmutableSparseSet` which removes the capacity fields in the backing vec's and the APIs for inserting or removing elements. Drops the size of the sparse set by 3 usizes (24 bytes on 64-bit systems) ## Followup ~~After #4809, Archetype's component SparseSet should be replaced with it.~~ This has been done. --- ## Changelog Removed: `Table::component_capacity` ## Migration Guide `Table::component_capacity()` has been removed as Tables do not support adding/removing columns after construction. Co-authored-by: Carter Anderson <[email protected]>

…evyengine#4927) # Objective Archetype is a deceptively large type in memory. It stores metadata about which components are in which storage in multiple locations, which is only used when creating new Archetypes while moving entities. ## Solution Remove the redundant `Box<[ComponentId]>`s and iterate over the sparse set of component metadata instead. Reduces Archetype's size by 4 usizes (32 bytes on 64-bit systems), as well as the additional allocations for holding these slices. It'd seem like there's a downside that the origin archetype has it's component metadata iterated over twice when creating a new archetype, but this change also removes the extra `Vec<ArchetypeComponentId>` allocations when creating a new archetype which may amortize out to a net gain here. This change likely negatively impacts creating new archetypes with a large number of components, but that's a cost mitigated by the fact that these archetypal relationships are cached in Edges and is incurred only once for each edge created. ## Additional Context There are several other in-flight PRs that shrink Archetype: - bevyengine#4800 merges the entities and rows Vecs together (shaves off 24 bytes per archetype) - bevyengine#4809 removes unique_components and moves it to it's own dedicated storage (shaves off 72 bytes per archetype) --- ## Changelog Changed: `Archetype::table_components` and `Archetype::sparse_set_components` return iterators instead of slices. `Archetype::new` requires iterators instead of parallel slices/vecs. ## Migration Guide Do I still need to do this? I really hope people were not relying on the public facing APIs changed here.

# Objective Make core types in ECS smaller. The column sparse set in Tables is never updated after creation. ## Solution Create `ImmutableSparseSet` which removes the capacity fields in the backing vec's and the APIs for inserting or removing elements. Drops the size of the sparse set by 3 usizes (24 bytes on 64-bit systems) ## Followup ~~After bevyengine#4809, Archetype's component SparseSet should be replaced with it.~~ This has been done. --- ## Changelog Removed: `Table::component_capacity` ## Migration Guide `Table::component_capacity()` has been removed as Tables do not support adding/removing columns after construction. Co-authored-by: Carter Anderson <[email protected]>

# Objective At least partially addresses bevyengine#6282. Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be `pub(crate)` which isn't ideal. This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a `World`. ## Solution - Add `Resources` parallel to `Tables` and `SparseSets` and extract the functionality used by `Archetype` in it. - Remove `unique_components` from `Archetype` - Remove the `pub(crate)` on `Archetype::components`. - Remove `ArchetypeId::RESOURCE` - Remove `Archetypes::resource` and `Archetypes::resource_mut` --- ## Changelog Added: `Resources` type to store resources. Added: `Storages::resource` Removed: `ArchetypeId::RESOURCE` Removed: `Archetypes::resource` and `Archetypes::resources` Removed: `Archetype::unique_components` and `Archetypes::unique_components_mut` ## Migration Guide Resources have been moved to `Resources` under `Storages` in `World`. All code dependent on `Archetype::unique_components(_mut)` should access it via `world.storages().resources()` instead. All APIs accessing the raw data of individual resources (mutable *and* read-only) have been removed as these APIs allowed for unsound unsafe code. All usages of these APIs should be changed to use `World::{get, insert, remove}_resource`.

…evyengine#4927) # Objective Archetype is a deceptively large type in memory. It stores metadata about which components are in which storage in multiple locations, which is only used when creating new Archetypes while moving entities. ## Solution Remove the redundant `Box<[ComponentId]>`s and iterate over the sparse set of component metadata instead. Reduces Archetype's size by 4 usizes (32 bytes on 64-bit systems), as well as the additional allocations for holding these slices. It'd seem like there's a downside that the origin archetype has it's component metadata iterated over twice when creating a new archetype, but this change also removes the extra `Vec<ArchetypeComponentId>` allocations when creating a new archetype which may amortize out to a net gain here. This change likely negatively impacts creating new archetypes with a large number of components, but that's a cost mitigated by the fact that these archetypal relationships are cached in Edges and is incurred only once for each edge created. ## Additional Context There are several other in-flight PRs that shrink Archetype: - bevyengine#4800 merges the entities and rows Vecs together (shaves off 24 bytes per archetype) - bevyengine#4809 removes unique_components and moves it to it's own dedicated storage (shaves off 72 bytes per archetype) --- ## Changelog Changed: `Archetype::table_components` and `Archetype::sparse_set_components` return iterators instead of slices. `Archetype::new` requires iterators instead of parallel slices/vecs. ## Migration Guide Do I still need to do this? I really hope people were not relying on the public facing APIs changed here.

# Objective Make core types in ECS smaller. The column sparse set in Tables is never updated after creation. ## Solution Create `ImmutableSparseSet` which removes the capacity fields in the backing vec's and the APIs for inserting or removing elements. Drops the size of the sparse set by 3 usizes (24 bytes on 64-bit systems) ## Followup ~~After bevyengine#4809, Archetype's component SparseSet should be replaced with it.~~ This has been done. --- ## Changelog Removed: `Table::component_capacity` ## Migration Guide `Table::component_capacity()` has been removed as Tables do not support adding/removing columns after construction. Co-authored-by: Carter Anderson <[email protected]>

# Objective At least partially addresses bevyengine#6282. Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be `pub(crate)` which isn't ideal. This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a `World`. ## Solution - Add `Resources` parallel to `Tables` and `SparseSets` and extract the functionality used by `Archetype` in it. - Remove `unique_components` from `Archetype` - Remove the `pub(crate)` on `Archetype::components`. - Remove `ArchetypeId::RESOURCE` - Remove `Archetypes::resource` and `Archetypes::resource_mut` --- ## Changelog Added: `Resources` type to store resources. Added: `Storages::resource` Removed: `ArchetypeId::RESOURCE` Removed: `Archetypes::resource` and `Archetypes::resources` Removed: `Archetype::unique_components` and `Archetypes::unique_components_mut` ## Migration Guide Resources have been moved to `Resources` under `Storages` in `World`. All code dependent on `Archetype::unique_components(_mut)` should access it via `world.storages().resources()` instead. All APIs accessing the raw data of individual resources (mutable *and* read-only) have been removed as these APIs allowed for unsound unsafe code. All usages of these APIs should be changed to use `World::{get, insert, remove}_resource`.

…evyengine#4927) # Objective Archetype is a deceptively large type in memory. It stores metadata about which components are in which storage in multiple locations, which is only used when creating new Archetypes while moving entities. ## Solution Remove the redundant `Box<[ComponentId]>`s and iterate over the sparse set of component metadata instead. Reduces Archetype's size by 4 usizes (32 bytes on 64-bit systems), as well as the additional allocations for holding these slices. It'd seem like there's a downside that the origin archetype has it's component metadata iterated over twice when creating a new archetype, but this change also removes the extra `Vec<ArchetypeComponentId>` allocations when creating a new archetype which may amortize out to a net gain here. This change likely negatively impacts creating new archetypes with a large number of components, but that's a cost mitigated by the fact that these archetypal relationships are cached in Edges and is incurred only once for each edge created. ## Additional Context There are several other in-flight PRs that shrink Archetype: - bevyengine#4800 merges the entities and rows Vecs together (shaves off 24 bytes per archetype) - bevyengine#4809 removes unique_components and moves it to it's own dedicated storage (shaves off 72 bytes per archetype) --- ## Changelog Changed: `Archetype::table_components` and `Archetype::sparse_set_components` return iterators instead of slices. `Archetype::new` requires iterators instead of parallel slices/vecs. ## Migration Guide Do I still need to do this? I really hope people were not relying on the public facing APIs changed here.

# Objective Make core types in ECS smaller. The column sparse set in Tables is never updated after creation. ## Solution Create `ImmutableSparseSet` which removes the capacity fields in the backing vec's and the APIs for inserting or removing elements. Drops the size of the sparse set by 3 usizes (24 bytes on 64-bit systems) ## Followup ~~After bevyengine#4809, Archetype's component SparseSet should be replaced with it.~~ This has been done. --- ## Changelog Removed: `Table::component_capacity` ## Migration Guide `Table::component_capacity()` has been removed as Tables do not support adding/removing columns after construction. Co-authored-by: Carter Anderson <[email protected]>

james7132 added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels May 20, 2022

alice-i-cecile requested a review from BoxyUwU May 30, 2022 17:39

alice-i-cecile requested changes May 30, 2022

View reviewed changes

crates/bevy_ecs/src/world/world_cell.rs Show resolved Hide resolved

crates/bevy_ecs/src/storage/resource.rs Outdated Show resolved Hide resolved

This was referenced Jun 4, 2022

[Merged by Bors] - Remove redundant table and sparse set component IDs from Archetype #4927

Closed

[Merged by Bors] - Immutable sparse sets for metadata storage #4928

Closed

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

Implement methods to list all components and all resources (Rebase of PR #3012) #4955

Closed

4 tasks

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

View reviewed changes

james7132 requested a review from alice-i-cecile June 7, 2022 09:10

james7132 added 9 commits June 7, 2022 02:10

Preliminary changes

7cd3e3f

Fix compilation

76ede2f

Provide internal helper function

b7b16be

More complete Resources impl

5464d47

Fix build

32c5456

Cleanup

bb9a49f

Fix CI

b719c1b

Fix CI

99f14d7

Shrink Resources

4918e10

james7132 force-pushed the no-unique-components branch from d8758f8 to 4918e10 Compare June 7, 2022 09:11

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

View reviewed changes

james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 11, 2022

BoxyUwU suggested changes Jun 11, 2022

View reviewed changes

james7132 and others added 5 commits June 11, 2022 15:54

Less jank way of fetching ticks.

2b24bac

Co-authored-by: Boxy <[email protected]>

Better safety comment.

ab675d1

Co-authored-by: Boxy <[email protected]>

Better safety comment.

bd25ccc

Co-authored-by: Boxy <[email protected]>

Correct safety comment pointing to the right parameter.

70578fe

Co-authored-by: Boxy <[email protected]>

Much simpler remove impl.

27ce1a3

Co-authored-by: Boxy <[email protected]>

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 12, 2022

Remove public access to data methods

efd5113

james7132 added C-Bug An unexpected or incorrect behavior P-Unsound A bug that results in undefined compiler behavior labels Oct 18, 2022

james7132 added 4 commits October 17, 2022 19:25

Update safety comments on mutative resource APIs

535bc9f

Fix CI

7932fa8

Update safety docs on get_ticks

c2476ed

Formatting

31cd2b4

james7132 requested a review from BoxyUwU October 18, 2022 03:16

BoxyUwU approved these changes Oct 18, 2022

View reviewed changes

bors bot changed the title ~~Extract Resources into their own dedicated storage~~ [Merged by Bors] - Extract Resources into their own dedicated storage Oct 24, 2022

bors bot closed this Oct 24, 2022

james7132 mentioned this pull request Oct 25, 2022

unique_components allows read access to !Sync resources from any thread #6282

Closed

gschup mentioned this pull request Nov 15, 2022

Upgrade for Bevy 0.9 gschup/bevy_ggrs#32

Closed

james7132 deleted the no-unique-components branch March 14, 2023 04:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Extract Resources into their own dedicated storage #4809

[Merged by Bors] - Extract Resources into their own dedicated storage #4809

james7132 commented May 20, 2022 •

edited

Loading

alice-i-cecile left a comment

alice-i-cecile left a comment

alice-i-cecile commented Jun 7, 2022

alice-i-cecile commented Jun 11, 2022

BoxyUwU left a comment

alice-i-cecile commented Oct 18, 2022

BoxyUwU left a comment

alice-i-cecile commented Oct 24, 2022

bors bot commented Oct 24, 2022

[Merged by Bors] - Extract Resources into their own dedicated storage #4809

[Merged by Bors] - Extract Resources into their own dedicated storage #4809

Conversation

james7132 commented May 20, 2022 • edited Loading

Objective

Solution

Changelog

Migration Guide

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Jun 7, 2022

alice-i-cecile commented Jun 11, 2022

BoxyUwU left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Oct 18, 2022

BoxyUwU left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Oct 24, 2022

bors bot commented Oct 24, 2022

james7132 commented May 20, 2022 •

edited

Loading