Skip to content

Conversation

chescock
Copy link
Contributor

Objective

Improve the performance of some dynamic queries with FilteredEntity(Ref|Mut) by allowing dense iteration in more cases, and remove a call to the sort-of deprecated Access::component_reads_and_writes() method.

QueryBuilder currently requires sparse iteration if any sparse set components may be read. We do need sparse iteration if sparse set components are used in the filters, but FilteredEntityRef can still perform dense iteration when reading optional components or when reading all components.

Note that the optional case is different from Option, which performs sparse iteration when the inner query is sparse so that it can cache whether the inner query matches for an entire archetype.

Solution

Change FilteredEntity(Ref|Mut) to have IS_DENSE = true. It used to require sparse iteration in order to filter the Access for each archetype, but #15207 changed it to copy the entire access.

Change QueryBuilder::is_dense() to check D::IS_DENSE && F::IS_DENSE instead of looking at the component reads and writes.
QueryBuilder::is_dense() still checks the filters, so builder.data::<&Sparse>() will still cause sparse iteration, but builder.data::<Option<&Sparse>>() no longer will.

I believe this is sound, even in the presence of query transmutes. The only WorldQuery implementations that rely on a sparse query being sparse for soundness are &, &mut, Ref, and Mut, but they can only be transmuted to if the component is in the required set. If a dynamic query has the component in the required set, then it appears in the filters and the query will use sparse iteration.

Note that Option and Has will misbehave and report None and false for all entities if they do a dense query while wrapping a sparse component, but they won't cause UB. And it's already possible to hit that case by transmuting from Query<EntityMut> to Query<Option<&Sparse>>.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Uncontroversial This work is generally agreed upon D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 15, 2024
@alice-i-cecile
Copy link
Member

Note that Option and Has will misbehave and report None and false for all entities if they do a dense query while wrapping a sparse component, but they won't cause UB. And it's already possible to hit that case by transmuting from Query to Query<Option<&Sparse>>.

Can you open an issue for this? :)

@Victoronz
Copy link
Contributor

And it's already possible to hit that case by transmuting from Query<EntityMut> to Query<Option<&Sparse>>.

This is possible? This is not documented under Query::transmute_lens, and I've never heard of this kind of transmute before, do you know what PR introduced Query<EntityMut> -> Query<Option<T>> transmutes?

@chescock
Copy link
Contributor Author

This is possible? This is not documented under Query::transmute_lens, and I've never heard of this kind of transmute before, do you know what PR introduced Query<EntityMut> -> Query<Option<T>> transmutes?

It does work! EntityMut has write access to all components, but doesn't require any of them. So it can't transmute to &T, which requires T, but it can transmute to Option<&T>, which reads it but does not require it.

I confess I only looked at the implementation and not the documentation, so I didn't realize it wasn't documented :). It looks like the subset check hasn't changed much since transmutes were introduced, so I think it's always worked.

Looking at the documentation now, that list doesn't seem especially exhaustive. You can also add Has<T> for any T, or go from AnyOf<(T, U)> to Option<T>. And there can always be third-party WorldQuery implementations.

Copy link
Contributor

@cBournhonesque cBournhonesque left a comment

Choose a reason for hiding this comment

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

I wonder if there is a way to write a test for it?

  1. we could have a small test showing that FilteredEntityRef with Option<&Sparse> access still is dense
  2. but how can we show that it doesn't miss any entity during iteration?

…dentityref

# Conflicts:
#	crates/bevy_ecs/src/query/builder.rs
#	crates/bevy_ecs/src/query/state.rs
…dentityref

# Conflicts:
#	crates/bevy_ecs/src/query/state.rs
chescock added 2 commits March 4, 2025 12:44
…dentityref

# Conflicts:
#	crates/bevy_ecs/src/query/builder.rs
…dentityref

# Conflicts:
#	crates/bevy_ecs/src/query/state.rs
@alice-i-cecile
Copy link
Member

@villor and @Victoronz could you two take a look at this?

…dentityref

# Conflicts:
#	crates/bevy_ecs/src/query/builder.rs
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone May 7, 2025
@atlv24 atlv24 modified the milestones: 0.17, 0.18 Jul 8, 2025
@james7132 james7132 self-requested a review August 18, 2025 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times D-Complex Quite challenging from either a design or technical perspective. Ask for help! D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Uncontroversial This work is generally agreed upon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants