-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Make entity::index
non max
#18704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make entity::index
non max
#18704
Conversation
glam = "0.29" | ||
rand = "0.8" | ||
rand_chacha = "0.3" | ||
nonmax = { version = "0.5", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there value in using NonMax over NonZero (I can see the value of banning u32::MAX
from being a valid Entity
but that doesn't require that the missing value be there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do u32
, we just need to document that u32::MAX
is invalid. We can go that route, but that means there won't be a niche in things like TableRow
or anything. (Or at least, it would be more work to convert types, etc.)
If we do u32::NonZero
, we loose both index 0
and u32::MAX
. We still need careful documentation, and we need to make sure we put a dummy value at index 0 anywhere we are using it as an index.
If we do u32::NonZero
but map it to a non max somehow, then we're effectively just reinventing NonMax
.
So out of those options I went with NonMax
here. But that's not to say we can't do otherwise. If you have strong opinions otherwise, feel free to make your case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only called it out since the PR focused solely on wanting a niche but didn't go into why this value was chosen.
IMHO I would lean towards banning 0 as a dummy (many similar setups just use 0 as explicitly none) and either fixing the places that can't handle Max or banning it. I only say that because I think the code here to use NonMax efficiently isn't worth the trade off of preserving 0 that isn't really used.
However I don't think this set of changes is bad and so wouldn't be against this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. We just need to put a niche in the EntityRow
, marking one value as invalid. Then the total number of ids possible is within u32::MAX. That's the goal.
So we could make 0 the niche, create dummy values for every relevant list, communicate that to users, and map 0..u32::MAX onto 1..=u32::MAX in the allocator. That would work. Pro: no op to access the index. Cons: need dummy values + extra allocation + extra math in entity allocator + invalidates some serialized data.
This pr makes the max the niche, keeping everything but the bit layout exactly the same. Pro: very simple. Cons: binary ! to access + invalidates a lot of serialized data.
It's just a trade off. If the consensus is different, I'm 100% fine with that. But at the end of the day, we just need to pick one.
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
|
Co-authored-by: atlv <email@atlasdostal.com>
# Objective This is a followup to #18704 . There's lots more followup work, but this is the minimum to unblock #18670, etc. This direction has been given the green light by Alice [here](#18704 (comment)). ## Solution I could have split this over multiple PRs, but I figured skipping straight here would be easiest for everyone and would unblock things the quickest. This removes the now no longer needed `identifier` module and makes `Entity::generation` go from `NonZeroU32` to `struct EntityGeneration(u32)`. ## Testing CI --------- Co-authored-by: Mark Nokalt <marknokalt@live.com>
# Objective There are two problems this aims to solve. First, `Entity::index` is currently a `u32`. That means there are `u32::MAX + 1` possible entities. Not only is that awkward, but it also make `Entity` allocation more difficult. I discovered this while working on remote entity reservation, but even on main, `Entities` doesn't handle the `u32::MAX + 1` entity very well. It can not be batch reserved because that iterator uses exclusive ranges, which has a maximum upper bound of `u32::MAX - 1`. In other words, having `u32::MAX` as a valid index can be thought of as a bug right now. We either need to make that invalid (this PR), which makes Entity allocation cleaner and makes remote reservation easier (because the length only needs to be u32 instead of u64, which, in atomics is a big deal), or we need to take another pass at `Entities` to make it handle the `u32::MAX` index properly. Second, `TableRow`, `ArchetypeRow` and `EntityIndex` (a type alias for u32) all have `u32` as the underlying type. That means using these as the index type in a `SparseSet` uses 64 bits for the sparse list because it stores `Option<IndexType>`. By using `NonMaxU32` here, we cut the memory of that list in half. To my knowledge, `EntityIndex` is the only thing that would really benefit from this niche. `TableRow` and `ArchetypeRow` I think are not stored in an `Option` in bulk. But if they ever are, this would help. Additionally this ensures `TableRow::INVALID` and `ArchetypeRow::INVALID` never conflict with an actual row, which in a nice bonus. As a related note, if we do components as entities where `ComponentId` becomes `Entity`, the the `SparseSet<ComponentId>` will see a similar memory improvement too. ## Solution Create a new type `EntityRow` that wraps `NonMaxU32`, similar to `TableRow` and `ArchetypeRow`. Change `Entity::index` to this type. ## Downsides `NonMax` is implemented as a `NonZero` with a binary inversion. That means accessing and storing the value takes one more instruction. I don't think that's a big deal, but it's worth mentioning. As a consequence, `to_bits` uses `transmute` to skip the inversion which keeps it a nop. But that also means that ordering has now flipped. In other words, higher indices are considered less than lower indices. I don't think that's a problem, but it's also worth mentioning. ## Alternatives We could keep the index as a u32 type and just document that `u32::MAX` is invalid, modifying `Entities` to ensure it never gets handed out. (But that's not enforced by the type system.) We could still take advantage of the niche here in `ComponentSparseSet`. We'd just need some unsafe manual conversions, which is probably fine, but opens up the possibility for correctness problems later. We could change `Entities` to fully support the `u32::MAX` index. (But that makes `Entities` more complex and potentially slightly slower.) ## Testing - CI - A few tests were changed because they depend on different ordering and `to_bits` values. ## Future Work - It might be worth removing the niche on `Entity::generation` since there is now a different niche. - We could move `Entity::generation` into it's own type too for clarity. - We should change `ComponentSparseSet` to take advantage of the new niche. (This PR doesn't change that yet.) - Consider removing or updating `Identifier`. This is only used for `Entity`, so it might be worth combining since `Entity` is now more unique. --------- Co-authored-by: atlv <email@atlasdostal.com> Co-authored-by: Zachary Harrold <zac@harrold.com.au>
…9121) # Objective This is a followup to bevyengine#18704 . There's lots more followup work, but this is the minimum to unblock bevyengine#18670, etc. This direction has been given the green light by Alice [here](bevyengine#18704 (comment)). ## Solution I could have split this over multiple PRs, but I figured skipping straight here would be easiest for everyone and would unblock things the quickest. This removes the now no longer needed `identifier` module and makes `Entity::generation` go from `NonZeroU32` to `struct EntityGeneration(u32)`. ## Testing CI --------- Co-authored-by: Mark Nokalt <marknokalt@live.com>
# Objective Since #18704 is done, we can track the length of unique entity row collections with only a `u32` and identify an index within that collection with only a `NonMaxU32`. This leaves an opportunity for performance improvements. ## Solution - Use `EntityRow` in sparse sets. - Change table, entity, and query lengths to be `u32` instead of `usize`. - Keep `batching` module `usize` based since that is reused for events, which may exceed `u32::MAX`. - Change according `Range<usize>` to `Range<u32>`. This is more efficient and helps justify safety. - Change `ArchetypeRow` and `TableRow` to wrap `NonMaxU32` instead of `u32`. Justifying `NonMaxU32::new_unchecked` everywhere is predicated on this safety comment in `Entities::set`: "`location` must be valid for the entity at `index` or immediately made valid afterwards before handing control to unknown code." This ensures no entity is in two table rows for example. That fact is used to argue uniqueness of the entity rows in each table, archetype, sparse set, query, etc. So if there's no duplicates, and a maximum total entities of `u32::MAX` none of the corresponding row ids / indexes can exceed `NonMaxU32`. ## Testing CI --------- Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
For details see bevyengine/bevy#19121 bevyengine/bevy#18704 The niche is now in the index, which makes the compression logic even simpler. The index now represented by NonMaxU32, which internally represented as NonZeroU32 with all bits reversed, so we have to xor the bits. I opened a PR to make it more ergonomic and avoid us relying on the internal layout: bevyengine/bevy#21246
# Objective Entity serialization is necessary for networking. Entities can exist inside components and events. After deserialization, we simply map remote entities to local entities. To serialize entities efficiently, we split them into index and generation, which benefits from varint serialization. #19121 and #18704 changed the entity layout, and I like the new layout a lot. We can now use the extra bit from the index to store whether the generation is zero or not, avoiding the need to serialize the generation entirely. However, constructing new entities requires relying on the internal layout, which is not very ergonomic. For example, here is how an entity with index = 1 and generation = 1 can be created: ```rust let expected_entity = Entity::from_bits((1 ^ u32::MAX) as u64 | (1 << 32)); ``` ## Solution - Make `Entity::from_raw_and_generation` public. While at it, I also removed outdated comment. - Add `EntityRow::from_raw_u32` to make the initialization nicer. ## Testing - It's a trivial change, but I re-used `EntityRow::from_raw_u32` in unit tests to simplify them. ## Notes I'd probably rename `Entity::from_raw_and_generation` into `Entity::from_row_and_generation` or `Entity::from_index_and_generation`.
# Objective Entity serialization is necessary for networking. Entities can exist inside components and events. After deserialization, we simply map remote entities to local entities. To serialize entities efficiently, we split them into index and generation, which benefits from varint serialization. #19121 and #18704 changed the entity layout, and I like the new layout a lot. We can now use the extra bit from the index to store whether the generation is zero or not, avoiding the need to serialize the generation entirely. However, constructing new entities requires relying on the internal layout, which is not very ergonomic. For example, here is how an entity with index = 1 and generation = 1 can be created: ```rust let expected_entity = Entity::from_bits((1 ^ u32::MAX) as u64 | (1 << 32)); ``` ## Solution - Make `Entity::from_raw_and_generation` public. While at it, I also removed outdated comment. - Add `EntityRow::from_raw_u32` to make the initialization nicer. ## Testing - It's a trivial change, but I re-used `EntityRow::from_raw_u32` in unit tests to simplify them. ## Notes I'd probably rename `Entity::from_raw_and_generation` into `Entity::from_row_and_generation` or `Entity::from_index_and_generation`.
# Objective Entity serialization is necessary for networking. Entities can exist inside components and events. After deserialization, we simply map remote entities to local entities. To serialize entities efficiently, we split them into index and generation, which benefits from varint serialization. #19121 and #18704 changed the entity layout, and I like the new layout a lot. We can now use the extra bit from the index to store whether the generation is zero or not, avoiding the need to serialize the generation entirely. However, constructing new entities requires relying on the internal layout, which is not very ergonomic. For example, here is how an entity with index = 1 and generation = 1 can be created: ```rust let expected_entity = Entity::from_bits((1 ^ u32::MAX) as u64 | (1 << 32)); ``` ## Solution - Make `Entity::from_raw_and_generation` public. While at it, I also removed outdated comment. - Add `EntityRow::from_raw_u32` to make the initialization nicer. ## Testing - It's a trivial change, but I re-used `EntityRow::from_raw_u32` in unit tests to simplify them. ## Notes I'd probably rename `Entity::from_raw_and_generation` into `Entity::from_row_and_generation` or `Entity::from_index_and_generation`.
For details see bevyengine/bevy#19121 bevyengine/bevy#18704 The niche is now in the index, which makes the compression logic even simpler. The index now represented by NonMaxU32, which internally represented as NonZeroU32 with all bits reversed, so we have to xor the bits. I opened a PR to make it more ergonomic and avoid us relying on the internal layout: bevyengine/bevy#21246
For details see bevyengine/bevy#19121 bevyengine/bevy#18704 The niche is now in the index, which makes the compression logic even simpler.
For details see bevyengine/bevy#19121 bevyengine/bevy#18704 The niche is now in the index, which makes the compression logic even simpler.
For details see bevyengine/bevy#19121 bevyengine/bevy#18704 The niche is now in the index, which makes the compression logic even simpler.
* Bump Bevy version * Migrate to the new Entity layout For details see bevyengine/bevy#19121 bevyengine/bevy#18704 The niche is now in the index, which makes the compression logic even simpler. * Migrate to the new `SystemParam` changes For details see bevyengine/bevy#16885 bevyengine/bevy#19143 * Remove `*_trigger_targets` * Simplify fns logic We no longer need custom sed/de to additionally serialize targets. This allows us to express things a bit nicer using conversion traits. * Rename all "event" into "message". * Rename all "trigger" into "event". * Rename "resend locally" into just "send locally" Fits better. * Split channel methods --------- Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
Objective
There are two problems this aims to solve.
First,
Entity::index
is currently au32
. That means there areu32::MAX + 1
possible entities. Not only is that awkward, but it also makeEntity
allocation more difficult. I discovered this while working on remote entity reservation, but even on main,Entities
doesn't handle theu32::MAX + 1
entity very well. It can not be batch reserved because that iterator uses exclusive ranges, which has a maximum upper bound ofu32::MAX - 1
. In other words, havingu32::MAX
as a valid index can be thought of as a bug right now. We either need to make that invalid (this PR), which makes Entity allocation cleaner and makes remote reservation easier (because the length only needs to be u32 instead of u64, which, in atomics is a big deal), or we need to take another pass atEntities
to make it handle theu32::MAX
index properly.Second,
TableRow
,ArchetypeRow
andEntityIndex
(a type alias for u32) all haveu32
as the underlying type. That means using these as the index type in aSparseSet
uses 64 bits for the sparse list because it storesOption<IndexType>
. By usingNonMaxU32
here, we cut the memory of that list in half. To my knowledge,EntityIndex
is the only thing that would really benefit from this niche.TableRow
andArchetypeRow
I think are not stored in anOption
in bulk. But if they ever are, this would help. Additionally this ensuresTableRow::INVALID
andArchetypeRow::INVALID
never conflict with an actual row, which in a nice bonus.As a related note, if we do components as entities where
ComponentId
becomesEntity
, the theSparseSet<ComponentId>
will see a similar memory improvement too.Solution
Create a new type
EntityRow
that wrapsNonMaxU32
, similar toTableRow
andArchetypeRow
.Change
Entity::index
to this type.Downsides
NonMax
is implemented as aNonZero
with a binary inversion. That means accessing and storing the value takes one more instruction. I don't think that's a big deal, but it's worth mentioning.As a consequence,
to_bits
usestransmute
to skip the inversion which keeps it a nop. But that also means that ordering has now flipped. In other words, higher indices are considered less than lower indices. I don't think that's a problem, but it's also worth mentioning.Alternatives
We could keep the index as a u32 type and just document that
u32::MAX
is invalid, modifyingEntities
to ensure it never gets handed out. (But that's not enforced by the type system.) We could still take advantage of the niche here inComponentSparseSet
. We'd just need some unsafe manual conversions, which is probably fine, but opens up the possibility for correctness problems later.We could change
Entities
to fully support theu32::MAX
index. (But that makesEntities
more complex and potentially slightly slower.)Testing
to_bits
values.Future Work
Entity::generation
since there is now a different niche.Entity::generation
into it's own type too for clarity.ComponentSparseSet
to take advantage of the new niche. (This PR doesn't change that yet.)Identifier
. This is only used forEntity
, so it might be worth combining sinceEntity
is now more unique.