Skip to content
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

Remote entity reservation v2 #18266

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
0278c77
sketched balancing
ElliottjPierce Mar 11, 2025
da541a0
decided on a data patern
ElliottjPierce Mar 11, 2025
279a5ae
reservations
ElliottjPierce Mar 11, 2025
8ca72cd
finished data needed
ElliottjPierce Mar 11, 2025
95bf141
flushing
ElliottjPierce Mar 11, 2025
95d86e1
figured out allocation
ElliottjPierce Mar 11, 2025
7ee4814
figured out free
ElliottjPierce Mar 11, 2025
415d87f
clearing and reservation
ElliottjPierce Mar 11, 2025
eb6859d
fixed more methods
ElliottjPierce Mar 11, 2025
5f587c4
sorted out lengths
ElliottjPierce Mar 11, 2025
c77f21a
roughly fixed alloc_at
ElliottjPierce Mar 11, 2025
160591c
cleaned up remaining missing pieces
ElliottjPierce Mar 11, 2025
0981b77
improved alloc_at quality
ElliottjPierce Mar 11, 2025
309aaae
rename
ElliottjPierce Mar 11, 2025
3f783f1
created remote interface
ElliottjPierce Mar 11, 2025
02d823e
correctness and docs pass
ElliottjPierce Mar 11, 2025
294142a
doc changes
ElliottjPierce Mar 11, 2025
35c5c81
preserve generation when marking dont flush
ElliottjPierce Mar 11, 2025
dc36429
fixed one test case ish
ElliottjPierce Mar 11, 2025
281f5b9
fixed typo
ElliottjPierce Mar 11, 2025
28ae3e2
fixed more tests
ElliottjPierce Mar 11, 2025
e145c3d
removed flushed assert
ElliottjPierce Mar 11, 2025
7fd1774
Safety comment rational
ElliottjPierce Mar 11, 2025
34065dc
fixed many tests
ElliottjPierce Mar 12, 2025
b57fefb
fixed ordering problems in tests
ElliottjPierce Mar 12, 2025
a8defe3
Fixed wrong tests
ElliottjPierce Mar 12, 2025
75ff44a
roll reserve_generations into freeing
ElliottjPierce Mar 12, 2025
b2b9705
bench in increments of allocation batch size
ElliottjPierce Mar 12, 2025
3db2fb8
improve reservation performance
ElliottjPierce Mar 12, 2025
d62f2c2
fix a test
ElliottjPierce Mar 12, 2025
622abb3
fix doc
ElliottjPierce Mar 12, 2025
145c3b6
Merge branch 'main' into remote-entity-reservation-v2
ElliottjPierce Mar 12, 2025
6c249b7
fixed dynamic scene tests
ElliottjPierce Mar 12, 2025
fede71c
fixed hardcoded serialized entities
ElliottjPierce Mar 12, 2025
d890692
redid the serializing test
ElliottjPierce Mar 12, 2025
6c3eb46
Merge branch 'remote-entity-reservation-v2' of https://github.com/Ell…
ElliottjPierce Mar 12, 2025
aa830e2
Tiny docs correction
ElliottjPierce Mar 12, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/bevy_ecs/src/archetype.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ impl ArchetypeRow {
/// Index indicating an invalid archetype row.
/// This is meant to be used as a placeholder.
pub const INVALID: ArchetypeRow = ArchetypeRow(u32::MAX);
/// This is the same as [`Self::INVALID`], but it signals to [`Entities`](crate::entity::Entities)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see this expanded to explain why it exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, when we reserve Entitys from EntityReservations for Entities::owned, we don't want to flush them like normal. They haven't been requested yet, so the flush doesn't make sense, and when they are requested by Entities::alloc, the caller will be the one to Entities::set the EntityLocation.

In principal, we could just have a HashSet of Entitys that have been in Entities::owned since the last flush, but that lookup takes time and memory.

Instead, by keeping a special EntityLocation::INVALID_BUT_DONT_FLUSH, we can determine whether or not to flush based only on if meta.location == EntityLocation::INVALID or not.

Hopefully that makes sense. This is complicated.

/// that this entity should not be flushed.
pub(crate) const INVALID_BUT_DONT_FLUSH: ArchetypeRow = ArchetypeRow(u32::MAX - 1);

/// Creates a `ArchetypeRow`.
#[inline]
Expand Down
15 changes: 6 additions & 9 deletions crates/bevy_ecs/src/entity/map_entities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ impl<T: VisitEntitiesMut> MapEntities for T {
/// fn get_mapped(&mut self, entity: Entity) -> Entity {
/// self.map.get(&entity).copied().unwrap_or(entity)
/// }
///
///
/// fn set_mapped(&mut self, source: Entity, target: Entity) {
/// self.map.insert(source, target);
/// }
Expand Down Expand Up @@ -275,8 +275,8 @@ mod tests {
);

mapper.finish(&mut world);
// Next allocated entity should be a further generation on the same index
let entity = world.spawn_empty().id();
// Future allocated entity should be a further generation on the same index
let entity = world.entities().resolve_from_id(dead_ref.index()).unwrap();
assert_eq!(entity.index(), dead_ref.index());
assert!(entity.generation() > dead_ref.generation());
}
Expand All @@ -290,8 +290,8 @@ mod tests {
mapper.get_mapped(Entity::from_raw(0))
});

// Next allocated entity should be a further generation on the same index
let entity = world.spawn_empty().id();
// Future allocated entity should be a further generation on the same index
let entity = world.entities().resolve_from_id(dead_ref.index()).unwrap();
assert_eq!(entity.index(), dead_ref.index());
assert!(entity.generation() > dead_ref.generation());
}
Expand All @@ -301,15 +301,12 @@ mod tests {
let mut world = World::new();
// "Dirty" the `Entities`, requiring a flush afterward.
world.entities.reserve_entity();
assert!(world.entities.needs_flush());
assert!(world.entities.worth_flushing());

// Create and exercise a SceneEntityMapper - should not panic because it flushes
// `Entities` first.
SceneEntityMapper::world_scope(&mut Default::default(), &mut world, |_, m| {
m.get_mapped(Entity::PLACEHOLDER);
});

// The SceneEntityMapper should leave `Entities` in a flushed state.
assert!(!world.entities.needs_flush());
}
}
Loading
Loading