Construct and deconstruct entities to improve entity allocation #19451
Conversation
it's not exact, but it should be good enough.
ElliottjPierce
added
C-Feature
| /// This can error if the [`EntityGeneration`] of this id has passed or if the [`EntityRow`] is not constructed. | ||
| /// See the module docs for a full explanation of these ids, entity life cycles, and the meaning of this result. | ||
| #[inline] | ||
| pub fn get_constructed( |
There was a problem hiding this comment.
A lot of the churn above seems to be replacing calls to get with get_constructed. Should get_constructed be the default?
Or, should this be the only method (and renamed to get)? If I'm reading these branches correctly, they return the same data in a different format. That is, I think get could be
pub fn get(&self, entity: Entity) -> Result<EntityIdLocation, EntityDoesNotExistError> {
match self.get_constructed(entity) {
Ok(location) => Ok(Some(location)),
Err(ConstructedEntityDoesNotExistError::WasNotConstructed { .. }) => Ok(None),
Err(ConstructedEntityDoesNotExistError::DidNotExist(err)) => Err(err),
}
}and I think that match could then be inlined cleanly into most callers, especially ones that do things like if let Ok(None) = self.world.entities.get(self.entity) {.
There was a problem hiding this comment.
Good thoughts. For now I just did the match. I'd like to keep both for utility purposes, but I'm open to ideas and renames.
crates/bevy_ecs/src/entity/mod.rs
Outdated
| // SAFETY: Caller guarantees that `index` a valid entity index | ||
| let meta = unsafe { self.meta.get_unchecked_mut(index as usize) }; | ||
| meta.spawned_or_despawned = SpawnedOrDespawned { by, at }; | ||
| pub(crate) unsafe fn declare( |
There was a problem hiding this comment.
The name declare is a little confusing without context. And then it's implemented in terms of update, which seems to be another word for the same thing.
I think this this is setting the location, and it seems to be the equivalent of the old set method. Could this be set or set_location, and fn update be set_unchecked or set_location_unchecked?
There was a problem hiding this comment.
I don't have a preference here. In my mind, declare means "this is location is true; make it happen" and update means "this is a revision, just change the value". But that doesn't mean there aren't better names. There's a lot of naming questions here in general. I'm open to suggestions, but I also don't want to make this pr blocked on name discussions. I'd rather merge with a bad less-good name and update it later.
There was a problem hiding this comment.
I don't have a preference here. In my mind,
declaremeans "this is location is true; make it happen" andupdatemeans "this is a revision, just change the value". But that doesn't mean there aren't better names. There's a lot of naming questions here in general. I'm open to suggestions, but I also don't want to make this pr blocked on name discussions. I'd rather merge with abadless-good name and update it later.
Yeah, I don't want to block on naming questions! I was just confused while starting to read the PR why entities.set(entity.index(), Some(location)); changed to let was_at = entities.declare(entity.row(), Some(location));. It would have been easier to read if the name hadn't changed, but that doesn't really matter now.
| pub unsafe fn flush( | ||
| /// - `row` must have been constructed at least once, ensuring its row is valid. | ||
| #[inline] | ||
| pub(crate) unsafe fn mark_construct_or_destruct( |
There was a problem hiding this comment.
You changed the name of this method from mark_spawn_despawn to mark_construct_or_destruct, but it's still updating a field called spawned_or_despawned. I'd vote for reverting the method name change to reduce churn, but if we really want to change the method name then shouldn't we also change the field name?
There was a problem hiding this comment.
These are good questions. I mentioned this early in the PR discussion (now hidden it was so long ago).
Basically, we need to find a balance between intuitive and easy to migrate (spawn/despawn) and precisely correct (construct/destruct). Using both in different places is one way I tried to strike the balance. Higher-level stuff is named spawn/despawn and lower level stuff is named construct/destruct. (In general.) But other ideas for how to handle this are welcome.
There was a problem hiding this comment.
These are good questions. I mentioned this early in the PR discussion (now hidden it was so long ago).
Oh, I see, sorry!
Basically, we need to find a balance between intuitive and easy to migrate (spawn/despawn) and precisely correct (construct/destruct). Using both in different places is one way I tried to strike the balance. Higher-level stuff is named spawn/despawn and lower level stuff is named construct/destruct. (In general.) But other ideas for how to handle this are welcome.
Yeah, that seems like a good approach! It just seems odd for a field and the method that sets it to be on opposite sides of the high-level/low-level boundary :). Not a blocker, though!
There was a problem hiding this comment.
It just seems odd for a field and the method that sets it to be on opposite sides of the high-level/low-level boundary.
100% agree. I just don't want to rename everything in one pr.
| let index = row.index() as usize; | ||
| if self.meta.len() <= index { | ||
| // TODO: hint unlikely once stable. | ||
| expand(&mut self.meta, index + 1); |
There was a problem hiding this comment.
Why not just call meta.resize(len, EntityMeta::FRESH); directly? I would expect Vec::resize to already implement optimizations for making actual resizing be cold, since it's called during things like push.
There was a problem hiding this comment.
Two reasons: First, this lets us also init the whole capacity, which reduces these calls, and second, turns out resize is not marked as cold. At least, I didn't see it. IDK why not.
There was a problem hiding this comment.
I guess I'm asking whether you benchmarked this part or are just theorizing. If there's a measured performance benefit to the complexity here then we should absolutely do it! But otherwise I'd prefer the code be smaller and simpler.
I think the #[cold] in Vec::reserve is buried inside RawVec: https://github.com/rust-lang/rust/blob/048b8790916ee2b1baf4dea62c2c8367aedb0d0c/library/alloc/src/raw_vec/mod.rs#L546
There was a problem hiding this comment.
Note that resize can also shrink the Vec. Not sure if this can actually happen here though.
There was a problem hiding this comment.
I think the
#[cold]inVec::reserveis buried insideRawVec: https://github.com/rust-lang/rust/blob/048b8790916ee2b1baf4dea62c2c8367aedb0d0c/library/alloc/src/raw_vec/mod.rs#L546
Oh interesting.
I guess I'm asking whether you benchmarked this part or are just theorizing. If there's a measured performance benefit to the complexity here then we should absolutely do it! But otherwise I'd prefer the code be smaller and simpler.
I am just theorizing performance, but I think the theory is right. Prior to this PR, Entities::set was a get_unchecked. Now update is get_unchecked, and declare needs bounds checking. I think skipping those checks for things like inserting components (which only needs update) is worth it. And initing the whole capacity with the default value instead of doing that incrementally means this is called 32 times max instead of 2 ^ 32 times. So I think it's worth it. And I don't think I can get precise enough benches to be meaningful here anyway; I'm not sure.
There was a problem hiding this comment.
Note that
resizecan also shrink theVec. Not sure if this can actually happen here though.
Yeah, and extend_with is private. I'm not aware of an alternative.
| { | ||
| let archetype = &mut world.archetypes[location.archetype_id]; | ||
| let archetype = &mut self.world.archetypes[location.archetype_id]; |
There was a problem hiding this comment.
Removing the let world = self.world; added a lot of churn to this method. It's a good change, but I'd prefer it be done as a separate PR to make this one smaller.
There was a problem hiding this comment.
The old one method took self, so it could be easily moved out. The new method takes &mut self (because it could be constructed again later), so reverting this would be challenging with lifetimes I think.
There was a problem hiding this comment.
The old one method took
self, so it could be easily moved out. The new method takes&mut self(because it could be constructed again later), so reverting this would be challenging with lifetimes I think.
Oh, I didn't see that! I think it still works if you reborrow with let world = &mut self.world;, though.
There was a problem hiding this comment.
I think it still works if you reborrow with
let world = &mut self.world;, though.
Lol. Forgot I could do that. That would be double indirection though (&mut &mut World). The compiler would probably unroll that, but I'm not sure. If you think it would be a purely aesthetic change, I can revert it. It does cause a lot of noise in the diff for that function.
There was a problem hiding this comment.
That would be double indirection though (
&mut &mut World).
Oh, right, it would need to be let world = &mut *self.world; to reborrow.
If you think it would be a purely aesthetic change, I can revert it. It does cause a lot of noise in the diff for that function.
I would personally revert it to reduce the diff, but I don't actually feel strongly. I only keep responding because you keep pointing out the flaws in my plan and so I am compelled to fix them :).
There was a problem hiding this comment.
I would personally revert it to reduce the diff, but I don't actually feel strongly. I only keep responding because you keep pointing out the flaws in my plan and so I am compelled to fix them :).
Lol. I'll just keep it as is then. I definitely should have tried to reduce the diff, but after 7 reviews, I think it's too little too late to revet now.
| Ok(fetched) => fetched, | ||
| Err(error) => panic_no_entity(self, error.entity), | ||
| Ok(res) => res, | ||
| Err(err) => panic!("{err}"), |
There was a problem hiding this comment.
Can this method now be shortened to self.get_entity(entities).unwrap()?
There was a problem hiding this comment.
That would tag along the "calling unwrap on a None value" message. Which it didn't before, but in principal, yeah.
| /// Returns the [`EntityCommands`] for the requested [`Entity`] if it exists in the world *now*. | ||
| /// Note that for entities that have not been constructed, like ones from [`spawn`](Self::spawn), this will error. | ||
| /// If that is not desired, try [`get_entity`](Self::get_entity). | ||
| /// This should be used over [`get_entity`](Self::get_entity) when you expect the entity to already exist constructed in the world. |
There was a problem hiding this comment.
Does this mean get_constructed_entity should be the one that users should usually use? If that's the case, then it might be good to give that behavior the short name of get_entity and rename get_entity to something longer. (Do we have a good term for a constructed-or-allocated entity?)
There was a problem hiding this comment.
You've mentioned this too with Entities::get vs get_constructed. They are both useful for different things. I would use get_constructed_entity when doing commands for an entity in a query. But for almost everything else, I would use get_entity, so it includes those that are not yet constructed.
I don't have a strong preference for where we settle with the naming. But from my (loose) experience updating internal usages of this, they are roughly equally useful. For example, consider an entity being piped through an event: Even tough those will almost always be constructed, we don't want bevy_systems to break down when they aren't.
These command methods are meant to catch "definitely an issue" cases, and even if get_constructed_entity fails, it is very possible that by the time the command would have been applied, the entity would be constructed. I mostly just added it for parity.
There was a problem hiding this comment.
I would use
get_constructed_entitywhen doing commands for an entity in a query. But for almost everything else, I would useget_entity, so it includes those that are not yet constructed.I don't have a strong preference for where we settle with the naming. But from my (loose) experience updating internal usages of this, they are roughly equally useful. For example, consider an entity being piped through an event: Even tough those will almost always be constructed, we don't want bevy_systems to break down when they aren't.
What sort of user code needs to interact with entities that are allocated but not constructed? I think doing commands for an entity in a query is the common case. Apart from commands.spawn(), of course, but that already gives you an EntityCommands.
Or, wait, didn't we rework these now that we have error handling in commands? ... Ah, I guess commands.entity() is the common case. It doesn't do any eager checks, since the entity might get despawned before the commands run. That makes get_entity feel even more niche. If I care enough to check eagerly, then it's probably not because I expect an allocated-but-not-constructed entity! So I'd vote for getting rid of the get_entity behavior and renaming get_constructed_entity to get_entity.
There was a problem hiding this comment.
Yeah, I don't think commands.get_entity is a very useful API in general.
There was a problem hiding this comment.
I'm broadly on board with these ideas. What do we want to do here? A few options
- Keep it as is and sort out renaming latter. (Most options)
- Remove
get_entityand renameget_constructed_entitytoget_entity. (Simplest) - Do option 2 but keep
get_entityunder a new name:get_maybe_constructed_entity? (Good, but starts naming battles with parity, etc.) - Do another pass later to move this error handling into
Commands::entityitself sinceget_entityisn't that helpful. (It just catches stuff slightly earlier.) (Probably best long-term IMO)
There was a problem hiding this comment.
I would prefer 4. This PR is already too complex.
| /// The next id to hand out is tracked by `free_len`. | ||
| free: Vec<Entity>, | ||
| /// This is continually subtracted from. | ||
| /// If it wraps to a very large number, it will be outside the bounds of `free`, |
There was a problem hiding this comment.
This is really subtle. The old code used signed numbers here so that it could use a sign check before casting, which made things a little more clear. Why switch to unsigned?
Do we need to worry about overflow if the free list is very large? The old code used 64-bit counters when available to prevent that.
There was a problem hiding this comment.
There's really three reasons for this:
- We know that the free list doesn't contain duplicates of an entity row, so the total length can now fit in 32 bits.
- I didn't like how the whole allocator cursor situation changed based on pointer width.
- I would guess not needing to cast signed to unsigned with the check is maybe part of the perf improvements.
If you would prefer, I could switch this to do the same trick but with an AtomicUsize to make reason 1 a moot point. Open to suggestions.
There was a problem hiding this comment.
If you would prefer, I could switch this to do the same trick but with an
AtomicUsizeto make reason 1 a moot point. Open to suggestions.
That sounds reasonable to me! I didn't like the cfgs there, either. usize gets us protection on common 64-bit platforms, and it's going to be hard to actually overflow on 32-bit platforms without running out of memory.
But it might be worth looking at the history of why that was done originally to make sure we aren't missing something!
- I would guess not needing to cast signed to unsigned with the check is maybe part of the perf improvements.
This would really surprise me. Signed and unsigned values usually have the same representation, and the high-level types just affect what instruction the arithmetic operations get compiled to. But I'm often surprised by the results of profilers :).
There was a problem hiding this comment.
That sounds reasonable to me! I didn't like the
cfgs there, either.usizegets us protection on common 64-bit platforms, and it's going to be hard to actually overflow on 32-bit platforms without running out of memory.
I'll do that then!
But it might be worth looking at the history of why that was done originally to make sure we aren't missing something!
I've looked at this for a while, and I don't think there's anything spooky going on here. Could be wrong though.
This would really surprise me. Signed and unsigned values usually have the same representation, and the high-level types just affect what instruction the arithmetic operations get compiled to. But I'm often surprised by the results of profilers :).
Honestly, me too. I'm not sure if the x > 0 is any slower than the x < len. But I do just generally prefer unsigned. IDK why.
There was a problem hiding this comment.
But it might be worth looking at the history of why that was done originally to make sure we aren't missing something!
Found some context from three years ago: #4452 (comment)
Is there a reason to not use isize/AtomicIsize everywhere?
We want the smallest signed integer big enough to contain
u32::MAX.That is
i64. It just so happens that some platforms don't support 64 bit atomics, so for those rare cases, it's better to fall back on isize, which (since we use at least 4 bytes of overhead per entity) is definitely big enough to contain as many entities as can be spawned (u32::MAX, but capped atusize::MAX/4).The reason not to use isize everywhere is that it's wasteful on 128 bit platforms. Admittedly, I don't know of any 128 bit platforms, but I'd rather keep it as the condition explained in my first paragraph.
And I think the consensus is that we don't guarantee it works. We just don't intentionally stand in the way of it not working (without good reason, anyway). In rustc parlance, these are tier 3 targets, i.e. it might be supported, and you can bring PRs to fix the support, but we don't try and ensure it is.
Co-Authored-By: Chris Russell <[email protected]>
alice-i-cecile
changed the title
|
Does this fix #19012? I probably cannot test before the weekend. |
| let source_archetype = source_entity.archetype(); | ||
| let source_archetype = source_entity | ||
| .archetype() | ||
| .expect("Source entity must exist constructed"); |
There was a problem hiding this comment.
Are these expects actually more useful than an unwrap with a comment above?
,expect() wastes binary space with strings, often gives worse error messages, and don't get automatically updated as the error type changes.
There was a problem hiding this comment.
I ended up collapsing them together. Now, instead of "expect the entity exists" and "expect it is constructed" it is just one "expect the entity exists and is constructed". I think that strikes a good balance.
| pub(crate) unsafe fn declare( | ||
| &mut self, | ||
| row: EntityRow, | ||
| location: EntityIdLocation, |
There was a problem hiding this comment.
Changing this to new_location would help me understand how this function works a lot faster :)
There was a problem hiding this comment.
This is one of those times when I go "why didn't I think of that?". Now it's new_location and update_location.
There was a problem hiding this comment.
Judging that marking one line in github shows the three previous lines here, I think the suggestion was renaming the parameter, not the method. But it is still better now. 😁
There was a problem hiding this comment.
lol. I didn't notice that. 😂 But yeah, I think this is much better than before.
There was a problem hiding this comment.
Dramatically clearer than your initial attempts, and I'm sold on the functionality and core model. I'm also extremely pleased that we now have a number of ECS contributors who understand what's going on here. Thanks y'all!
I've left a few nits around docs and unwraps, but fundamentally nothing blocking. I'd also like to try and incorporate that lovely diagram into the entity module docs, but that's fine in follow-up.
The most important follow-up work for this is cleaning up commands.get_entity, but that can and should be done separately.
There was a problem hiding this comment.
What an Odyssee this is.
I understand this PR and while I approve it, I do have two notes:
- I think these names could use some bikeshedding, especially
EntityRowis confusing to me. I also think the docs could be more clear in places. - Secondly, I want to take a closer look at the seperation of responsibillities between EntityAllocator and Entities. Do they need to be seperate structs? Can the former be nested in the latter?
I'm worried that the average user will find this mighty confusing, but I say this with the caveat that I've been staring af this code for a couple hours and the average user might never have to, so keep that in mind.
| //! This column doesn't represents a component and is specific to the [`EntityRow`], not the [`Entity`]. | ||
| //! For example, one thing Bevy stores in this metadata is the current [`EntityGeneration`] of the row. | ||
| //! It also stores more information like the [`Tick`] a row was last constructed or destructed, and the [`EntityIdLocation`] itself. | ||
| //! For more information about what's stored here, see [`Entities`], Bevy's implementation of this special column. |
There was a problem hiding this comment.
I think this documentation is a good start, but I think a more direct route can be taken, along the lines of
"Entity spawning is done in two stages: First we create an entity id (alloc step), and then we construct it (add components and other metadata). The reason for this is that we need to be able to assign entity ids concurrently, while for construction we need exclusive (non-concurrent) access to the world. This leads to an entity having three states: It doesn't exist (unallocated), a id has been assigned but is not known to the rest of the world (null), and an entity that has been fully created (constructed)."
This, to me, seems like a much simpler mental model (even if it's partially incorrect). I think this documentation is fine for now, but it might be worth looking over again in a future PR.
There was a problem hiding this comment.
I like the clarity here. Yes, we need another docs pass, and book pass, etc. But for now, I adapted this into the storage docs. It was too good not to!
Co-authored-by: Alice Cecile <[email protected]>
Co-Authored-By: Trashtalk217 <[email protected]>
…ps://github.com/ElliottjPierce/bevy into Remove-entity-reserving/pending/flushing-system
@urben1680: Yes and no. No, it doesn't fix that. Technically, a freed entity does still exist. That is correct. The only requirement for an entity to exist is that the generation is up to date. This is intended behavior. For example, it could allow checking if the entity exists, and then constructing it. (In the particular case of a freed entity, this would cause errors when that entity is allocated, but Yes, it does add a |
I know, right?
Agreed. The more I think about it, the more "row" -> "index" renames make sense to me. And definitely more docs are needed in future PRs.
Yes, they can be together. In fact, originally, they were. But ultimately, they do very different things. There is no overlap of responsibility between them, and, more importantly, for remote reservation, the allocator needs to operate completely independently of As a side note, I see
I hear what you're saying here, but I don't think this will be an issue. Before, we had, |
Hm with two separate methods that distinction becomes more obvious to me which makes me lean more to the "yes" part of your answer. |
Objective
This is the next step for #19430 and is also convinient for #18670.
For context, the way entities work on main is as a "allocate and use" system. Entity ids are allocated, and given a location. The location can then be changed, etc. Entities that are free have an invalid location. To allocate an entity, one must also set its location. This introduced the need for pending entities, where an entity would be reserved, pending, and at some point flushed. Pending and free entities have an invalid location, and others are assumed to have a valid one.
This paradigm has a number of downsides: First, the entities metadata table is inseparable from the allocator, which makes remote reservation challenging. Second, the
Worldmust be flushed, even to do simple things, like allocate a temporary entity id. Third, users have little control over entity ids, only interacting with conceptual entities. This made things likeEntities::alloc_atclunky and slow, leading to its removal, despite some users still having valid need of it.So the goal of this PR is to:
Entitiesfrom entity allocation to make room for other allocators and resolvealloc_atissues.reserveandflushpatterns toallocandconstructpatterns.It is possible to break this up into multiple prs, as I originally intended, but doing so would require lots of temporary scaffolding that would both hurt performance and make things harder to review.
Solution
This solution builds on #19433, which changed the representation of invalid entity locations from a constant to
None.There's quite a few steps to this, each somewhat controversial:
Entities with no location
This pr introduces the idea of entity rows both with and without locations. This corresponds to entities that are constructed (the row has a location) and not constructed (the row has no location). When a row is free or pending, it is not constructed. When a row is outside the range of the meta list, it still exists; it's just not constructed.
This extends to conceptual entities; conceptual entities may now be in one of 3 states: empty (constructed; no components), normal (constructed; 1 or more components), or null (not constructed). This extends to entity pointers (
EntityWorldMut, etc): These now can point to "null"/not constructed entities. Depending on the privilege of the pointer, these can also construct or destruct the entity.This also changes how
Entityids relate to conceptual entities. AnEntitynow exists if its generation matches that of its row. AnEntitythat has the right generation for its row will claim to exist, even if it is not constructed. This means, for example, anEntitymanually constructed with a large index and generation of 0 will exist if it has not been allocated yet.Entitiesis separate from the allocatorThis pr separates entity allocation from
Entities.Entitiesis now only focused on tracking entity metadata, etc. The newEntitiesAllocatoronWorldmanages all allocations. This forcesEntitiesto not rely on allocator state to determine if entities exist, etc, which is convinient for remote reservation and needed for custom allocators. It also paves the way for allocators not housed within theWorld, makes some unsafe code easier since the allocator and metadata live under different pointers, etc.This separation requires thinking about interactions with
Entitiesin a new way. Previously, theEntitiesset the rules for what entities are valid and what entities are not. Now, it has no way of knowing. Instead, interaction withEntitiesare more like declaring some information for it to track than changing some information it was already tracking. To reflect this,sethas been split up intodeclareandupdate.Constructing and destructing
As mentioned, entities that have no location (not constructed) can be constructed at any time. This takes on exactly the same meaning as the previous
spawn_non_existent. It creates/declares a location instead of updating an old one. As an example, this makes spawning an entity now literately just allocate a new id and construct it immediately.Conversely, entities that are constructed may be destructed. This removes all components and despawns related entities, just like
despawn. The only difference is that destructing does not free the entity id for reuse. Between constructing and destructing, all needs foralloc_atare resolved. If you want to keep the id for custom reuse, just destruct instead of despawn! Despawn, now just destructs the entity and frees it.Destructing a not constructed entity will do nothing. Constructing an already constructed entity will panic. This is to guard against users constructing a manually formed
Entitythat the allocator could later hand out. However, public construction methods have proper error handling for this. Despawning a not constructed entity just frees its id.No more flushing
All places that once needed to reserve and flush entity ids now allocate and construct them instead. This improves performance and simplifies things.
Flow chart
(Thanks @ItsDoot)
Testing
Showcase
Here's an example of constructing and destructing
Future Work
Entitydoesn't always correspond to a conceptual entity.EntityWorldMut. There is (and was) a lot of assuming the entity is constructed there (was assuming it was not despawned).Performance
Benchmarks
This roughly doubles command spawning speed! Despawning also sees a 20-30% improvement. Dummy commands improve by 10-50% (due to not needing an entity flush). Other benchmarks seem to be noise and are negligible. It looks to me like a massive performance win!