Skip to content

General entity set cleanup #21498

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

General entity set cleanup #21498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
513da58
privatize entity set type fields
Victoronz Oct 10, 2025
4e08725
entity set constructor consistency
Victoronz Oct 10, 2025
bb95355
inline entity set logic
Victoronz Oct 10, 2025
dceec17
stop internally skipping entity set type construction safety
Victoronz Oct 10, 2025
68546a9
turn various entity set functions const
Victoronz Oct 10, 2025
1f1d205
miscellaneous simplifications
Victoronz Oct 10, 2025
a1530b0
doc cleanup
Victoronz Oct 10, 2025
3c0c67a
update bevy_ecs msrv to 1.89
Victoronz Oct 10, 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
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ keywords = ["game", "engine", "gamedev", "graphics", "bevy"]
license = "MIT OR Apache-2.0"
repository = "https://github.com/bevyengine/bevy"
documentation = "https://docs.rs/bevy"
rust-version = "1.88.0"
rust-version = "1.89.0"

[workspace]
resolver = "2"
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ repository = "https://github.com/bevyengine/bevy"
license = "MIT OR Apache-2.0"
keywords = ["ecs", "game", "bevy"]
categories = ["game-engines", "data-structures"]
rust-version = "1.86.0"
rust-version = "1.89.0"

[features]
default = ["std", "bevy_reflect", "async_executor", "backtrace"]
Expand Down
20 changes: 9 additions & 11 deletions crates/bevy_ecs/src/component/clone.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,20 +110,18 @@ pub fn component_clone_via_reflect(source: &SourceComponent, ctx: &mut Component
// Try to clone using ReflectFromReflect
if let Some(reflect_from_reflect) =
registry.get_type_data::<bevy_reflect::ReflectFromReflect>(type_id)
{
if let Some(mut component) =
&& let Some(mut component) =
reflect_from_reflect.from_reflect(source_component_reflect.as_partial_reflect())
{
if let Some(reflect_component) =
registry.get_type_data::<crate::reflect::ReflectComponent>(type_id)
{
if let Some(reflect_component) =
registry.get_type_data::<crate::reflect::ReflectComponent>(type_id)
{
reflect_component.map_entities(&mut *component, ctx.entity_mapper());
}
drop(registry);

ctx.write_target_component_reflect(component);
return;
reflect_component.map_entities(&mut *component, ctx.entity_mapper());
}
drop(registry);

ctx.write_target_component_reflect(component);
return;
}
// Else, try to clone using ReflectDefault
if let Some(reflect_default) =
Expand Down
10 changes: 5 additions & 5 deletions crates/bevy_ecs/src/entity/clone_entities.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1139,11 +1139,11 @@ impl OptOut {
#[inline]
fn filter_deny(&mut self, id: ComponentId, world: &World) {
self.deny.insert(id);
if self.attach_required_by_components {
if let Some(required_by) = world.components().get_required_by(id) {
self.deny.extend(required_by.iter());
};
}
if self.attach_required_by_components
&& let Some(required_by) = world.components().get_required_by(id)
{
self.deny.extend(required_by.iter());
};
}
}

Expand Down
64 changes: 54 additions & 10 deletions crates/bevy_ecs/src/entity/entity_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use core::{
fmt::{Debug, Formatter},
hash::{BuildHasher, Hash},
iter::{self, FusedIterator},
option, result,
option, ptr, result,
};

use super::{Entity, UniqueEntityEquivalentSlice};
Expand Down Expand Up @@ -58,7 +58,7 @@ pub trait ContainsEntity {
/// To obtain hash values forming the same total order as [`Entity`], any [`Hasher`] used must be
/// deterministic and concerning [`Entity`], collisionless.
/// Standard library hash collections handle collisions with an [`Eq`] fallback, but do not account for
/// determinism when [`BuildHasher`] is unspecified,.
/// determinism when [`BuildHasher`] is unspecified.
///
/// [`Hash`]: core::hash::Hash
/// [`Hasher`]: core::hash::Hasher
Expand All @@ -68,6 +68,7 @@ pub trait ContainsEntity {
pub unsafe trait EntityEquivalent: ContainsEntity + Eq {}

impl ContainsEntity for Entity {
#[inline]
fn entity(&self) -> Entity {
*self
}
Expand All @@ -78,6 +79,7 @@ impl ContainsEntity for Entity {
unsafe impl EntityEquivalent for Entity {}

impl<T: ContainsEntity> ContainsEntity for &T {
#[inline]
fn entity(&self) -> Entity {
(**self).entity()
}
Expand All @@ -90,6 +92,7 @@ impl<T: ContainsEntity> ContainsEntity for &T {
unsafe impl<T: EntityEquivalent> EntityEquivalent for &T {}

impl<T: ContainsEntity> ContainsEntity for &mut T {
#[inline]
fn entity(&self) -> Entity {
(**self).entity()
}
Expand All @@ -102,6 +105,7 @@ impl<T: ContainsEntity> ContainsEntity for &mut T {
unsafe impl<T: EntityEquivalent> EntityEquivalent for &mut T {}

impl<T: ContainsEntity> ContainsEntity for Box<T> {
#[inline]
fn entity(&self) -> Entity {
(**self).entity()
}
Expand All @@ -114,6 +118,7 @@ impl<T: ContainsEntity> ContainsEntity for Box<T> {
unsafe impl<T: EntityEquivalent> EntityEquivalent for Box<T> {}

impl<T: ContainsEntity> ContainsEntity for Rc<T> {
#[inline]
fn entity(&self) -> Entity {
(**self).entity()
}
Expand All @@ -126,6 +131,7 @@ impl<T: ContainsEntity> ContainsEntity for Rc<T> {
unsafe impl<T: EntityEquivalent> EntityEquivalent for Rc<T> {}

impl<T: ContainsEntity> ContainsEntity for Arc<T> {
#[inline]
fn entity(&self) -> Entity {
(**self).entity()
}
Expand All @@ -143,12 +149,12 @@ unsafe impl<T: EntityEquivalent> EntityEquivalent for Arc<T> {}
/// As a consequence, [`into_iter()`] on `EntitySet` will always produce another `EntitySet`.
///
/// Implementing this trait allows for unique query iteration over a list of entities.
/// See [`iter_many_unique`] and [`iter_many_unique_mut`]
/// See [`iter_many_unique`] and [`iter_many_unique_mut`].
///
/// Note that there is no guarantee of the [`IntoIterator`] impl being deterministic,
/// it might return different iterators when called multiple times.
/// Neither is there a guarantee that the comparison trait impls of `EntitySet` match that
/// of the respective [`EntitySetIterator`] (or of a [`Vec`] collected from its elements)
/// of the respective [`EntitySetIterator`] (or of a [`Vec`] collected from its elements).
///
/// [`Self::IntoIter`]: IntoIterator::IntoIter
/// [`into_iter()`]: IntoIterator::into_iter
Expand Down Expand Up @@ -342,6 +348,7 @@ pub trait FromEntitySetIterator<A: EntityEquivalent>: FromIterator<A> {
impl<T: EntityEquivalent + Hash, S: BuildHasher + Default> FromEntitySetIterator<T>
for HashSet<T, S>
{
#[inline]
fn from_entity_set_iter<I: EntitySet<Item = T>>(set_iter: I) -> Self {
let iter = set_iter.into_iter();
let set = HashSet::<T, S>::with_capacity_and_hasher(iter.size_hint().0, S::default());
Expand All @@ -358,14 +365,17 @@ impl<T: EntityEquivalent + Hash, S: BuildHasher + Default> FromEntitySetIterator
/// An iterator that yields unique entities.
///
/// This wrapper can provide an [`EntitySetIterator`] implementation when an instance of `I` is known to uphold uniqueness.
#[repr(transparent)]
pub struct UniqueEntityIter<I: Iterator<Item: EntityEquivalent>> {
iter: I,
}

impl<I: EntitySetIterator> UniqueEntityIter<I> {
/// Constructs a `UniqueEntityIter` from an [`EntitySetIterator`].
pub fn from_entity_set_iterator<S>(iter: I) -> Self {
Self { iter }
#[inline]
pub const fn from_entity_set_iter(iter: I) -> Self {
// SAFETY: iter implements `EntitySetIterator`.
unsafe { Self::from_iter_unchecked(iter) }
}
}

Expand All @@ -375,17 +385,42 @@ impl<I: Iterator<Item: EntityEquivalent>> UniqueEntityIter<I> {
/// # Safety
/// `iter` must only yield unique elements.
/// As in, the resulting iterator must adhere to the safety contract of [`EntitySetIterator`].
pub unsafe fn from_iterator_unchecked(iter: I) -> Self {
#[inline]
pub const unsafe fn from_iter_unchecked(iter: I) -> Self {
Self { iter }
}

/// Constructs a [`UniqueEntityIter`] from an iterator unsafely.
///
/// # Safety
/// `iter` must only yield unique elements.
/// As in, the resulting iterator must adhere to the safety contract of [`EntitySetIterator`].
#[inline]
pub const unsafe fn from_iter_ref_unchecked(iter: &I) -> &Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this makes sense for completeness, but there's not much you can do with an &impl Iterator.

Same thing for UniqueEntityEquivalentVec::from_vec_ref_unchecked. Are there ever any cases where you need that instead of UniqueEntityEquivalentSlice::from_slice_unchecked? I guess you can call capacity()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For iterators, remember that is a UniqueEntityIter construction method, its main purpose is to be able to mark any iterator as an EntitySetIterator. This is not just for iteration itself! Sometimes, there are &SomeIterator can return underlying views into a collection, like f.e. as_slice()/AsRef<[T]>.

For Vecs, it has to do with safety around the uniqueness invariant:
If you know that you have borrowed the full collection, you have stronger guarantees about its subsections.
F.e. a slice can always have adjacent elements you have no awareness of/access to. If you have a Vec, this is never the case.

Right now, mutable UniqueEntitySlice logic is not yet implemented, so we do not yet have safety comments that talk about this subtlety.
Interestingly enough, the need to reference collections while maintaining a "no superslice" guarantee is not one I've really heard of before, which seems to be corroborated by some triggered lints surrounding &Box<[T]> and the like.

// SAFETY: UniqueEntityIter is a transparent wrapper around I.
unsafe { &*ptr::from_ref(iter).cast() }
}

/// Constructs a [`UniqueEntityIter`] from an iterator unsafely.
///
/// # Safety
/// `iter` must only yield unique elements.
/// As in, the resulting iterator must adhere to the safety contract of [`EntitySetIterator`].
#[inline]
pub const unsafe fn from_iter_mut_unchecked(iter: &mut I) -> &mut Self {
// SAFETY: UniqueEntityIter is a transparent wrapper around I.
unsafe { &mut *ptr::from_mut(iter).cast() }
}

/// Returns the inner `I`.
#[inline]
pub fn into_inner(self) -> I {
self.iter
}

/// Returns a reference to the inner `I`.
pub fn as_inner(&self) -> &I {
#[inline]
pub const fn as_inner(&self) -> &I {
&self.iter
}

Expand All @@ -395,18 +430,21 @@ impl<I: Iterator<Item: EntityEquivalent>> UniqueEntityIter<I> {
///
/// `self` must always contain an iterator that yields unique elements,
/// even while this reference is live.
pub unsafe fn as_mut_inner(&mut self) -> &mut I {
#[inline]
pub const unsafe fn as_mut_inner(&mut self) -> &mut I {
&mut self.iter
}
}

impl<I: Iterator<Item: EntityEquivalent>> Iterator for UniqueEntityIter<I> {
type Item = I::Item;

#[inline]
Copy link
Contributor

@Shatur Shatur Oct 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't make sense to inline non-public functions or functions with generics. I'd add inline only to things that are expected to be used often, not to every small function.
Please, see the standard library developers guide and this article.

Copy link
Contributor Author

@Victoronz Victoronz Oct 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, seems like std strongly diverges from this advice. Given that the vast majority of entity set code simply delegates somewhere else, I thought taking over those inlining decisions + being more liberal with them because these functions are small would be feasible, but that doesn't seem to be the case.
I now also realize what the inline-more annotations in hashbrown mean.

Judging by the discussions around inline, I wouldn't say that it doesn't make sense to inline non-public functions or functions with generics, but that it makes sense less often.

Given this, I think I'll cull most #[inline]s, aside from hotter functions, and those that I know can miss optimization opportunities, mostly collects and extends.
Do you think that, ignoring the standard library, I should retain the inlining decisions of the most popular crates that either define/wrap data structures like this code area does?
Even with this new knowledge, some inlining in those crates feel somewhat arbitrary or like plain oversights.

Copy link
Contributor

Choose a reason for hiding this comment

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

The linked article explains it quite nicely. Trivial private functions can already be inlined because the compiler sees their bodies during crate compilation. We even got a conservative cross-crate inlining recently. Generics are monomorphized during compilation, so their bodies are also visible during crate compilation, which means the compiler can inline them as well. inline just hints to the compiler that this function might be used often and suggests inlining even without LTO (though the compiler can still ignore this).

It would be annoying to manually add inline to every small function. So unless profiled or used really often, I'd avoid adding inline at all.

It's like unrolling loops - we don't do this anymore, unless profiling shows that it's necessary for humans to step in.

fn next(&mut self) -> Option<Self::Item> {
self.iter.next()
}

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
self.iter.size_hint()
}
Expand All @@ -415,6 +453,7 @@ impl<I: Iterator<Item: EntityEquivalent>> Iterator for UniqueEntityIter<I> {
impl<I: ExactSizeIterator<Item: EntityEquivalent>> ExactSizeIterator for UniqueEntityIter<I> {}

impl<I: DoubleEndedIterator<Item: EntityEquivalent>> DoubleEndedIterator for UniqueEntityIter<I> {
#[inline]
fn next_back(&mut self) -> Option<Self::Item> {
self.iter.next_back()
}
Expand All @@ -426,6 +465,7 @@ impl<I: FusedIterator<Item: EntityEquivalent>> FusedIterator for UniqueEntityIte
unsafe impl<I: Iterator<Item: EntityEquivalent>> EntitySetIterator for UniqueEntityIter<I> {}

impl<T, I: Iterator<Item: EntityEquivalent> + AsRef<[T]>> AsRef<[T]> for UniqueEntityIter<I> {
#[inline]
fn as_ref(&self) -> &[T] {
self.iter.as_ref()
}
Expand All @@ -434,6 +474,7 @@ impl<T, I: Iterator<Item: EntityEquivalent> + AsRef<[T]>> AsRef<[T]> for UniqueE
impl<T: EntityEquivalent, I: Iterator<Item: EntityEquivalent> + AsRef<[T]>>
AsRef<UniqueEntityEquivalentSlice<T>> for UniqueEntityIter<I>
{
#[inline]
fn as_ref(&self) -> &UniqueEntityEquivalentSlice<T> {
// SAFETY: All elements in the original slice are unique.
unsafe { UniqueEntityEquivalentSlice::from_slice_unchecked(self.iter.as_ref()) }
Expand All @@ -443,6 +484,7 @@ impl<T: EntityEquivalent, I: Iterator<Item: EntityEquivalent> + AsRef<[T]>>
impl<T: EntityEquivalent, I: Iterator<Item: EntityEquivalent> + AsMut<[T]>>
AsMut<UniqueEntityEquivalentSlice<T>> for UniqueEntityIter<I>
{
#[inline]
fn as_mut(&mut self) -> &mut UniqueEntityEquivalentSlice<T> {
// SAFETY: All elements in the original slice are unique.
unsafe { UniqueEntityEquivalentSlice::from_slice_unchecked_mut(self.iter.as_mut()) }
Expand All @@ -451,6 +493,7 @@ impl<T: EntityEquivalent, I: Iterator<Item: EntityEquivalent> + AsMut<[T]>>

// Default does not guarantee uniqueness, meaning `I` needs to be EntitySetIterator.
impl<I: EntitySetIterator + Default> Default for UniqueEntityIter<I> {
#[inline]
fn default() -> Self {
Self {
iter: Default::default(),
Expand All @@ -460,6 +503,7 @@ impl<I: EntitySetIterator + Default> Default for UniqueEntityIter<I> {

// Clone does not guarantee to maintain uniqueness, meaning `I` needs to be EntitySetIterator.
impl<I: EntitySetIterator + Clone> Clone for UniqueEntityIter<I> {
#[inline]
fn clone(&self) -> Self {
Self {
iter: self.iter.clone(),
Expand Down Expand Up @@ -506,7 +550,7 @@ mod tests {

// SAFETY: SpawnBatchIter is `EntitySetIterator`,
let mut unique_entity_iter =
unsafe { UniqueEntityIter::from_iterator_unchecked(spawn_batch.iter()) };
unsafe { UniqueEntityIter::from_iter_unchecked(spawn_batch.iter()) };

let entity_set = unique_entity_iter
.by_ref()
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/entity/hash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ pub struct EntityHash;
impl BuildHasher for EntityHash {
type Hasher = EntityHasher;

#[inline]
fn build_hasher(&self) -> Self::Hasher {
Self::Hasher::default()
}
Expand Down
Loading