Nonmax all rows

[ElliottjPierce]

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

[github-actions]

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.

[ItsDoot]

Looks good! I like the additional cleanup with Archetype::entities_with_location

[urben1680]

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.

[ElliottjPierce]

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.

[urben1680]

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.

[urben1680]

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())) };

[ElliottjPierce]

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.

[urben1680]

I think I did not understand all logic behind this part of the codebase, but you did not change much from how it was so I focused on the safety comments.

[urben1680]

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.

[ElliottjPierce]

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

[urben1680]

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.

[ElliottjPierce]

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.

[urben1680]

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.

[ElliottjPierce]

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.

[NthTensor]

This approval is limited to the migration guide, which looks great! Thanks again for the detail.

Very pleased to see people updating/expanding existing migration guides. This is exactly the intended use. Great job.

[ElliottjPierce]

The new migration guide system is like 10x better than the old. It feels so much better now. Thanks for making it!

When I finish my entity pr marathon, I'm going to make a pr just to consolidate/polish the entity migration guides a little more.(I know we'll do that again before release, but keeping the guides as files lets us do this iteratively too.) Amazing!