Skip to content

Nonmax all rows #19132

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
May 26, 2025
Merged

Nonmax all rows #19132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b3561e2
use EntityRow in sparse_set
ElliottjPierce May 8, 2025
984b99e
Make TableRow nonmax
ElliottjPierce May 8, 2025
f2bdd9d
make archetype row non max
ElliottjPierce May 8, 2025
67a468b
consistent naming between table and archetype row
ElliottjPierce May 8, 2025
7fd04e5
formatting
ElliottjPierce May 9, 2025
52cc0bb
migration guide
ElliottjPierce May 9, 2025
42e3196
fix ci
ElliottjPierce May 9, 2025
a8bd757
now fix markdown linter
ElliottjPierce May 9, 2025
a5e226b
Fix typo
ElliottjPierce May 11, 2025
f488f85
Add todos
ElliottjPierce May 23, 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
48 changes: 40 additions & 8 deletions crates/bevy_ecs/src/archetype.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ use core::{
hash::Hash,
ops::{Index, IndexMut, RangeFrom},
};
use nonmax::NonMaxU32;

/// An opaque location within a [`Archetype`].
///
Expand All @@ -44,23 +45,30 @@ use core::{
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
// SAFETY: Must be repr(transparent) due to the safety requirements on EntityLocation
#[repr(transparent)]
pub struct ArchetypeRow(u32);
pub struct ArchetypeRow(NonMaxU32);

impl ArchetypeRow {
/// Index indicating an invalid archetype row.
/// This is meant to be used as a placeholder.
pub const INVALID: ArchetypeRow = ArchetypeRow(u32::MAX);
// TODO: Deprecate in favor of options, since `INVALID` is, technically, valid.
pub const INVALID: ArchetypeRow = ArchetypeRow(NonMaxU32::MAX);
Copy link
Contributor

@urben1680 urben1680 May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding my earlier comment, even if there are not u32::MAX entities but NonMaxU32::MAX entities, that last one would have a row equal to INVALID? If they all end up in the same archetype.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It was that way before though. This will let us trade INVALID for None though in the future!

Copy link
Contributor

@urben1680 urben1680 May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It was that way before though

I am not so happy with this reasoning when stumbling over a potential bug. 😅

If I am not mistaken this is only used in EntityLocation::INVALID and these are checked upon by looking at the ArchetypeId being invalid or not, so this seems to be no concern. Maybe this constant could be renamed to PLACEHOLDER and warn in the docs that this may be a valid row in a full archetype.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we're on the same page here. It's not good that INVALID is, in fact, valid.

In the past, it wrapped u32, so we were forced to use u32::MAX as None (Option would take 64 bits!). In the future, making it non-max will let us remove INVALID and just use Option.

Also note that this is only ever used in coordination with ArchetypeId, which has the same issue.

We're in agreement that this would be better called PLACEHOLDER or something or removed and replaced with a proper Option. But I think changing names/semantics like this is better left for a different PR. For now, I just want to tighten up the type, now that entity rows are themselves NonMax.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be more comfortable with adding a "todo: deprecate because this row can indeed be valid" comment on the constant? That could be addressed in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I've added a todo item. I suspect I can fulfill it in a future PR in the near future. I'd just like remote reservation to be merged first for convenience.


/// Creates a `ArchetypeRow`.
#[inline]
pub const fn new(index: usize) -> Self {
Self(index as u32)
pub const fn new(index: NonMaxU32) -> Self {
Self(index)
}

/// Gets the index of the row.
#[inline]
pub const fn index(self) -> usize {
self.0 as usize
self.0.get() as usize
}

/// Gets the index of the row.
#[inline]
pub const fn index_u32(self) -> u32 {
self.0.get()
}
}

Expand Down Expand Up @@ -467,6 +475,27 @@ impl Archetype {
&self.entities
}

/// Fetches the entities contained in this archetype.
#[inline]
pub fn entities_with_location(&self) -> impl Iterator<Item = (Entity, EntityLocation)> {
self.entities.iter().enumerate().map(
|(archetype_row, &ArchetypeEntity { entity, table_row })| {
(
entity,
EntityLocation {
archetype_id: self.id,
// SAFETY: The entities in the archetype must be unique and there are never more than u32::MAX entities.
archetype_row: unsafe {
ArchetypeRow::new(NonMaxU32::new_unchecked(archetype_row as u32))
},
table_id: self.table_id,
table_row,
},
)
},
)
}

/// Gets an iterator of all of the components stored in [`Table`]s.
///
/// All of the IDs are unique.
Expand Down Expand Up @@ -569,7 +598,8 @@ impl Archetype {
entity: Entity,
table_row: TableRow,
) -> EntityLocation {
let archetype_row = ArchetypeRow::new(self.entities.len());
// SAFETY: An entity can not have multiple archetype rows and there can not be more than u32::MAX entities.
let archetype_row = unsafe { ArchetypeRow::new(NonMaxU32::new_unchecked(self.len())) };
Comment on lines +601 to +602
Copy link
Contributor

@urben1680 urben1680 May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Something is off here.

"there can not be more than u32::MAX entities" semantically means "there can be up to u32::MAX entities". That would mean this can be unsound if there really end up being u32::MAX entities, not fitting into NonMaxU32.

If I understand #18704 correctly, then this is not a incorrectly worded comment: There can be u32::MAX entities indeed, thus an archetype containing them all can have a size of u32::MAX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is admittedly tricky to reason about.

The index of the Entity id, may be up to (but not equal to) u32::MAX. But the number of entities is the maximum index + 1. So there may well be u32::MAX entities. But even if there are, none of them will have u32::MAX as an index. The opposite applies too. If the maximum number of entities is u32::MAX, each entity has an index less than u32::MAX.

So the safety comment is correct everywhere. It's just a bit hard to get one's head around it unless you're familiar with the entity id scheme.

If you can think of clearer wording, I'm open to suggestions. But off by one errors are so common, I don't think we need to bend over backwards to explain it at each safety comment. I think it's fine as it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the idea is this:

  • This method is called when an entity is about to be put into the archetype
  • This implies that this new entity is not already part of the archetype, so archetype.len() cannot return u32::MAX, it only could once this call is over
  • When u32::MAX entities are reached, this method here will not called again as that implied there would be more entities than u32::MAX, thus this is safe

If I am correct then this is safe. I am just not sure if the method safety docs need to note that one should not make an Entity and TableRow out of thin air and call this when there are already u32::MAX entities.

Maybe add to the comment I marked here that at the time of the call there can not be u32::MAX entities inside this archetype. That would more reflect on the self.len() call here and not these mental steps you have to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// SAFETY: An entity can not have multiple archetype rows and there can not be more than u32::MAX entities.
let archetype_row = unsafe { ArchetypeRow::new(NonMaxU32::new_unchecked(self.len())) };
// SAFETY: An entity can not have multiple archetype rows and with this at most
// u32::MAX'th entity being new to this archetype, self.len() cannot return u32::MAX.
let archetype_row = unsafe { ArchetypeRow::new(NonMaxU32::new_unchecked(self.len())) };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the idea is this: ...

That's a good summary.

If I am correct then this is safe. I am just not sure if the method safety docs need to note that one should not make an Entity and TableRow out of thin air and call this when there are already u32::MAX entities.

If I'm understanding your concerns correctly, we're on the same page that the only logical problem here is if the caller is inventing these values outside of an entity allocator. As long as these values come from an allocator, and the relevant EntityLocation safety comments are upheld by when interacting with Entities, this is 100% safe.

The only danger is creating rows and entities "out of thin air". Nobody is doing that. Further, this is already expressly unsafe. Safety comments usually require these ids to be valid, etc.

I don't disagree that this safety issue would be nice to address, but it should be somewhat obvious that the Entity and TableRow must be valid. Making the safety comments explicitly say this would be a big effort (I think) because this assumption that Entity ids are valid, have valid location data, etc, are so fundamental that adding this to all safety comments would be a big PR.

At the end of the day, I'm not opposed to this; I just don't think that should be grouped into this PR.

self.entities.push(ArchetypeEntity { entity, table_row });

EntityLocation {
Expand Down Expand Up @@ -606,8 +636,10 @@ impl Archetype {

/// Gets the total number of entities that belong to the archetype.
#[inline]
pub fn len(&self) -> usize {
self.entities.len()
pub fn len(&self) -> u32 {
// No entity may have more than one archetype row, so there are no duplicates,
// and there may only ever be u32::MAX entities, so the length never exceeds u32's cappacity.
self.entities.len() as u32
}

/// Checks if the archetype has any entities.
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/batching.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use core::ops::Range;
/// [`EventReader::par_read`]: crate::event::EventReader::par_read
#[derive(Clone, Debug)]
pub struct BatchingStrategy {
/// The upper and lower limits for a batch of entities.
/// The upper and lower limits for a batch of items.
///
/// Setting the bounds to the same value will result in a fixed
/// batch size.
Expand Down
44 changes: 26 additions & 18 deletions crates/bevy_ecs/src/query/fetch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ unsafe impl ReadOnlyQueryData for EntityLocation {}
/// match spawn_details.spawned_by().into_option() {
/// Some(location) => println!(" by {:?}", location),
/// None => println!()
/// }
/// }
/// }
/// }
///
Expand Down Expand Up @@ -1505,7 +1505,7 @@ unsafe impl<T: Component> QueryData for &T {
// SAFETY: set_table was previously called
let table = unsafe { table.debug_checked_unwrap() };
// SAFETY: Caller ensures `table_row` is in range.
let item = unsafe { table.get(table_row.as_usize()) };
let item = unsafe { table.get(table_row.index()) };
item.deref()
},
|sparse_set| {
Expand Down Expand Up @@ -1617,11 +1617,15 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> {
) {
let column = table.get_column(component_id).debug_checked_unwrap();
let table_data = Some((
column.get_data_slice(table.entity_count()).into(),
column.get_added_ticks_slice(table.entity_count()).into(),
column.get_changed_ticks_slice(table.entity_count()).into(),
column.get_data_slice(table.entity_count() as usize).into(),
column
.get_added_ticks_slice(table.entity_count() as usize)
.into(),
column
.get_changed_ticks_slice(table.entity_count() as usize)
.into(),
column
.get_changed_by_slice(table.entity_count())
.get_changed_by_slice(table.entity_count() as usize)
.map(Into::into),
));
// SAFETY: set_table is only called when T::STORAGE_TYPE = StorageType::Table
Expand Down Expand Up @@ -1679,13 +1683,13 @@ unsafe impl<'__w, T: Component> QueryData for Ref<'__w, T> {
unsafe { table.debug_checked_unwrap() };

// SAFETY: The caller ensures `table_row` is in range.
let component = unsafe { table_components.get(table_row.as_usize()) };
let component = unsafe { table_components.get(table_row.index()) };
// SAFETY: The caller ensures `table_row` is in range.
let added = unsafe { added_ticks.get(table_row.as_usize()) };
let added = unsafe { added_ticks.get(table_row.index()) };
// SAFETY: The caller ensures `table_row` is in range.
let changed = unsafe { changed_ticks.get(table_row.as_usize()) };
let changed = unsafe { changed_ticks.get(table_row.index()) };
// SAFETY: The caller ensures `table_row` is in range.
let caller = callers.map(|callers| unsafe { callers.get(table_row.as_usize()) });
let caller = callers.map(|callers| unsafe { callers.get(table_row.index()) });

Ref {
value: component.deref(),
Expand Down Expand Up @@ -1812,11 +1816,15 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
) {
let column = table.get_column(component_id).debug_checked_unwrap();
let table_data = Some((
column.get_data_slice(table.entity_count()).into(),
column.get_added_ticks_slice(table.entity_count()).into(),
column.get_changed_ticks_slice(table.entity_count()).into(),
column.get_data_slice(table.entity_count() as usize).into(),
column
.get_added_ticks_slice(table.entity_count() as usize)
.into(),
column
.get_changed_ticks_slice(table.entity_count() as usize)
.into(),
column
.get_changed_by_slice(table.entity_count())
.get_changed_by_slice(table.entity_count() as usize)
.map(Into::into),
));
// SAFETY: set_table is only called when T::STORAGE_TYPE = StorageType::Table
Expand Down Expand Up @@ -1874,13 +1882,13 @@ unsafe impl<'__w, T: Component<Mutability = Mutable>> QueryData for &'__w mut T
unsafe { table.debug_checked_unwrap() };

// SAFETY: The caller ensures `table_row` is in range.
let component = unsafe { table_components.get(table_row.as_usize()) };
let component = unsafe { table_components.get(table_row.index()) };
// SAFETY: The caller ensures `table_row` is in range.
let added = unsafe { added_ticks.get(table_row.as_usize()) };
let added = unsafe { added_ticks.get(table_row.index()) };
// SAFETY: The caller ensures `table_row` is in range.
let changed = unsafe { changed_ticks.get(table_row.as_usize()) };
let changed = unsafe { changed_ticks.get(table_row.index()) };
// SAFETY: The caller ensures `table_row` is in range.
let caller = callers.map(|callers| unsafe { callers.get(table_row.as_usize()) });
let caller = callers.map(|callers| unsafe { callers.get(table_row.index()) });

Mut {
value: component.deref_mut(),
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/query/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ unsafe impl<T: Component> QueryFilter for Added<T> {
// SAFETY: set_table was previously called
let table = unsafe { table.debug_checked_unwrap() };
// SAFETY: The caller ensures `table_row` is in range.
let tick = unsafe { table.get(table_row.as_usize()) };
let tick = unsafe { table.get(table_row.index()) };

tick.deref().is_newer_than(fetch.last_run, fetch.this_run)
},
Expand Down Expand Up @@ -1044,7 +1044,7 @@ unsafe impl<T: Component> QueryFilter for Changed<T> {
// SAFETY: set_table was previously called
let table = unsafe { table.debug_checked_unwrap() };
// SAFETY: The caller ensures `table_row` is in range.
let tick = unsafe { table.get(table_row.as_usize()) };
let tick = unsafe { table.get(table_row.index()) };

tick.deref().is_newer_than(fetch.last_run, fetch.this_run)
},
Expand Down
Loading