[Merged by Bors] - Extract Resources into their own dedicated storage#4809
Closed
james7132 wants to merge 40 commits intobevyengine:mainfrom
Closed
[Merged by Bors] - Extract Resources into their own dedicated storage#4809james7132 wants to merge 40 commits intobevyengine:mainfrom
james7132 wants to merge 40 commits intobevyengine:mainfrom
Conversation
james7132
added
A-ECS
alice-i-cecile
requested changes
May 30, 2022
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
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.
This was referenced Jun 4, 2022
4 tasks
alice-i-cecile
approved these changes
Jun 7, 2022
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
@bevyengine/ecs-team This one is easy; reviews?
Member
|
@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. |
james7132
force-pushed
the
no-unique-components
branch
from
d8758f8 to
4918e10
Compare
alice-i-cecile
approved these changes
Jun 11, 2022
james7132
added
the
M-Migration-Guide
Member
|
Ping @BoxyUwU for attempt 5 to remember to review this 😂 |
BoxyUwU
suggested changes
Jun 11, 2022
Member
BoxyUwU
left a comment
BoxyUwU
left a comment
There was a problem hiding this comment.
5th times the charm I suppose
Co-authored-by: Boxy <supbscripter@gmail.com>
Co-authored-by: Boxy <supbscripter@gmail.com>
Co-authored-by: Boxy <supbscripter@gmail.com>
Co-authored-by: Boxy <supbscripter@gmail.com>
Co-authored-by: Boxy <supbscripter@gmail.com>
alice-i-cecile
added
the
X-Controversial
james7132
added
C-Bug
Member
|
I think we can do those in follow-up PRs. It's useful, but not essential. |
BoxyUwU
approved these changes
Oct 18, 2022
Member
BoxyUwU
left a comment
BoxyUwU
left a comment
There was a problem hiding this comment.
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.
Member
|
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+ |
bors bot
pushed a commit
that referenced
this pull request
Oct 24, 2022
# 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`.
Contributor
bors
bot
changed the title
james7132
added a commit
to james7132/bevy
that referenced
this pull request
Oct 28, 2022
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`.
bors bot
pushed a commit
that referenced
this pull request
Nov 15, 2022
…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.
bors bot
pushed a commit
that referenced
this pull request
Nov 15, 2022
…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.
bors bot
pushed a commit
that referenced
this pull request
Nov 15, 2022
# 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 <mcanders1@gmail.com>
taiyoungjang
pushed a commit
to taiyoungjang/bevy
that referenced
this pull request
Dec 15, 2022
…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.
taiyoungjang
pushed a commit
to taiyoungjang/bevy
that referenced
this pull request
Dec 15, 2022
# 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 <mcanders1@gmail.com>
Pietrek14
pushed a commit
to Pietrek14/bevy
that referenced
this pull request
Dec 17, 2022
# 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`.
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
…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.
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
# 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 <mcanders1@gmail.com>
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# 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`.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
…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.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# 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 <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
5 participants
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.
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
Resourcesparallel toTablesandSparseSetsand extract the functionality used byArchetypein it.unique_componentsfromArchetypepub(crate)onArchetype::components.ArchetypeId::RESOURCEArchetypes::resourceandArchetypes::resource_mutChangelog
Added:
Resourcestype to store resources.Added:
Storages::resourceRemoved:
ArchetypeId::RESOURCERemoved:
Archetypes::resourceandArchetypes::resourcesRemoved:
Archetype::unique_componentsandArchetypes::unique_components_mutMigration Guide
Resources have been moved to
ResourcesunderStoragesinWorld. All code dependent onArchetype::unique_components(_mut)should access it viaworld.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.