Use NonMaxUsize for non-component SparseSets #12083
Merged
Conversation
This file contains 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
james7132
added
A-ECS
|
A quick run through of the micro-benchmarks seems to show at least no consistent regression in performance: |
james7132
force-pushed
the
nonmax-sparse-set
branch
from
56a7685
to
9799f87
Compare
|
Looks good, but I can't provide any feedback about the generated instructions |
MiniaczQ
approved these changes
Mar 2, 2024
alice-i-cecile
approved these changes
Mar 2, 2024
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
I think this is a good set of trade-offs, and the added crate dep seems sensible. |
jdm
approved these changes
Mar 3, 2024
This was referenced Mar 4, 2024
spectria-limina
pushed a commit
to spectria-limina/bevy
that referenced
this pull request
Mar 9, 2024
# Objective Adoption of bevyengine#2104 and bevyengine#11843. The `Option<usize>` wastes 3-7 bytes of memory per potential entry, and represents a scaling memory overhead as the ID space grows. The goal of this PR is to reduce memory usage without significantly impacting common use cases. Co-Authored By: @NathanSWard Co-Authored By: @tygyh ## Solution Replace `usize` in `SparseSet`'s sparse array with `nonmax::NonMaxUsize`. NonMaxUsize wraps a NonZeroUsize, and applies a bitwise NOT to the value when accessing it. This allows the compiler to niche the value and eliminate the extra padding used for the `Option` inside the sparse array, while moving the niche value from 0 to usize::MAX instead. Checking the [diff in x86 generated assembly](james7132/bevy_asm_tests@6e4da65), this change actually results in fewer instructions generated. One potential downside is that it seems to have moved a load before a branch, which means we may be incurring a cache miss even if the element is not there. Note: unlike bevyengine#2104 and bevyengine#11843, this PR only targets the metadata stores for the ECS and not the component storage itself. Due to bevyengine#9907 targeting `Entity::generation` instead of `Entity::index`, `ComponentSparseSet` storing only up to `u32::MAX` elements would become a correctness issue. This will come with a cost when inserting items into the SparseSet, as now there is a potential for a panic. These cost are really only incurred when constructing a new Table, Archetype, or Resource that has never been seen before by the World. All operations that are fairly cold and not on any particular hotpath, even for command application. --- ## Changelog Changed: `SparseSet` now can only store up to `usize::MAX - 1` elements instead of `usize::MAX`. Changed: `SparseSet` now uses 33-50% less memory overhead per stored item.
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.
Objective
Adoption of #2104 and #11843. The
Option<usize>wastes 3-7 bytes of memory per potential entry, and represents a scaling memory overhead as the ID space grows.The goal of this PR is to reduce memory usage without significantly impacting common use cases.
Co-Authored By: @NathanSWard
Co-Authored By: @tygyh
Solution
Replace
usizeinSparseSet's sparse array withnonmax::NonMaxUsize. NonMaxUsize wraps a NonZeroUsize, and applies a bitwise NOT to the value when accessing it. This allows the compiler to niche the value and eliminate the extra padding used for theOptioninside the sparse array, while moving the niche value from 0 to usize::MAX instead.Checking the diff in x86 generated assembly, this change actually results in fewer instructions generated. One potential downside is that it seems to have moved a load before a branch, which means we may be incurring a cache miss even if the element is not there.
Note: unlike #2104 and #11843, this PR only targets the metadata stores for the ECS and not the component storage itself. Due to #9907 targeting
Entity::generationinstead ofEntity::index,ComponentSparseSetstoring only up tou32::MAXelements would become a correctness issue.This will come with a cost when inserting items into the SparseSet, as now there is a potential for a panic. These cost are really only incurred when constructing a new Table, Archetype, or Resource that has never been seen before by the World. All operations that are fairly cold and not on any particular hotpath, even for command application.
Changelog
Changed:
SparseSetnow can only store up tousize::MAX - 1elements instead ofusize::MAX.Changed:
SparseSetnow uses 33-50% less memory overhead per stored item.