Accelerate Query::get #12925

re0312 · 2024-04-11T09:04:18Z

Objective

query::get is commonly used to retrieve an item from a specific entity. However, its performance may not be optimal when dealing with a group of entities, particularly for wide queries. In fact, it may even be slower than EntityHashmap.

Solution

In real-world scenarios, it's highly probable that the next entity belongs to the same archetype as the previous one , Therefore, caching the fetch operation can lead to faster retrieval.

Changelog

Added QueryEntityGetter
Added Query::getter(_mut)
Added QueryState::getter(_mut)

Performance

Best case (all entity in the same archetype), 2x-4x faster with QueryEntityGetter::get , 4x-10x faster with QueryEntityGetter::get_unchecked .
Worst case (every entity with different archetypes and using safety get ), it has 10%~20% regresion.

alice-i-cecile · 2024-04-11T10:24:12Z

crates/bevy_ecs/src/system/query.rs

+    }
+
+    /// Return a [`QueryEntityGetter`] to get the query item for the [`Entity`].
+    /// this could be more efficient than [`Query::get`].


This should call out why / when you might expect this to be faster.

alice-i-cecile · 2024-04-11T10:25:02Z

crates/bevy_ecs/src/query/getter.rs

+
+use super::{DebugCheckedUnwrap, QueryData, QueryEntityError, QueryFilter, QueryState};
+
+/// A getter used for fast fetch its Component with Specific [`Entity`]


This doc string needs an editing pass.

james7132

Thanks for taking a swing at this! Haven't had the time for a full review yet, but a bit of a bikeshed on the name QueryEntityGetter -> PointQuery.

This should be done in a followup PR, but PointQuery implementing SystemParam would also be useful, though it may not be intuitive to use since immutable fetches require a mutable struct.

re0312 · 2024-04-12T11:42:11Z

Thanks for taking a swing at this! Haven't had the time for a full review yet, but a bit of a bikeshed on the name QueryEntityGetter -> PointQuery.

This should be done in a followup PR, but PointQuery implementing SystemParam would also be useful, though it may not be intuitive to use since immutable fetches require a mutable struct.

I did a benchmark on many_foxes with this PR, and I didn't observe any performance differences in propagate_transform. Only the use of the unsafe version seemed to yield around a 10% improvement.

TBH, the greatest value of this PR lies in providing a more versatile method for retrieving items from many entities than iter_many. However, I'm not certain if this approach is worthy.

crates/bevy_ecs/src/query/getter.rs

crates/bevy_ecs/src/query/state.rs

james7132 · 2024-04-14T02:14:12Z

Might want to test this against the visibility propagation system since that runs single-threaded and thus can needing fetch reinitialization as often as the transform propagate system.

re0312 · 2024-04-14T03:40:24Z

Might want to test this against the visibility propagation system since that runs single-threaded and thus can needing fetch reinitialization as often as the transform propagate system.

run many_foxes with all entities Visibility changed every frame,
system{name="bevy_render::view::visibility::visibility_propagate_system"}

from 400us to 240 us

benches/benches/bevy_ecs/world/mod.rs

crates/bevy_ecs/src/entity/mod.rs

james7132 · 2024-04-14T08:45:33Z

crates/bevy_ecs/src/query/point_query.rs

+            .entities
+            .get(entity)
+            .ok_or(QueryEntityError::NoSuchEntity(entity))?;
+        if !D::IS_DENSE || self.last_archetype_id != location.archetype_id {


This is unsound. Please check how the dense checks are done elsewhere. If D::IS_DENSE and F::IS_DENSE, this should use tables and table IDs instead of archetypes.

crates/bevy_ecs/src/query/state.rs

crates/bevy_ecs/src/system/query.rs

james7132 · 2024-04-14T08:52:44Z

crates/bevy_ecs/src/query/point_query.rs

+    ///
+    /// This is always guaranteed to run in `O(1)` time.
+    #[inline]
+    pub fn get(&mut self, entity: Entity) -> Result<D::Item<'w>, QueryEntityError> {


Suggested change

pub fn get(&mut self, entity: Entity) -> Result<D::Item<'w>, QueryEntityError> {

pub fn get(&mut self, entity: Entity) -> Result<D::Item<'_>, QueryEntityError> {

For this to be sound, we cannot allow this borrow to live longer than the borrow on self.

james7132 · 2024-04-14T08:53:42Z

crates/bevy_ecs/src/query/point_query.rs

+    /// `entity` must on the same `World` that the `Query` was generated.
+    /// `entity` must match the `Query` that generate this getter.
+    #[inline]
+    pub unsafe fn get_unchecked(&mut self, entity: Entity) -> D::Item<'w> {


Suggested change

pub unsafe fn get_unchecked(&mut self, entity: Entity) -> D::Item<'w> {

pub unsafe fn get_unchecked(&mut self, entity: Entity) -> D::Item<'_> {

Co-authored-by: James Liu <contact@jamessliu.com>

ItsDoot

Noticed a couple things.

ItsDoot · 2024-08-11T04:44:39Z

crates/bevy_ecs/src/query/point_query.rs

+    pub fn get(&mut self, entity: Entity) -> Result<D::Item<'_>, QueryEntityError> {
+        // SAFETY: system runs without conflicts with other systems.
+        // same-system queries have runtime borrow checks when they conflict
+        unsafe { std::mem::transmute(self.get_unsafe(entity)) }


Suggested change

unsafe { std::mem::transmute(self.get_unsafe(entity)) }

let result = unsafe { self.get_unsafe(entity) };

match result {

Ok(item) => Ok(D::shrink(item)),

Err(err) => Err(err),

}

We can use WorldQuery::shrink instead of a transmute.

ItsDoot · 2024-08-11T04:45:20Z

crates/bevy_ecs/src/query/point_query.rs

+        }
+
+        let item = D::fetch(&mut self.fetch, entity, location.table_row);
+        std::mem::transmute(item)


Suggested change

std::mem::transmute(item)

D::shrink(item)

We can use shrink here as well, instead of the transmute.

re0312 added 4 commits April 11, 2024 11:29

fast getter

da14191

getter

19e42ea

bench

b44c430

cleanup

2761b99

re0312 changed the title ~~Accelarate Query::get~~ Accelerate Query::get Apr 11, 2024

fix cli

747a969

SolarLiner added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Apr 11, 2024

fix doc

7fa8d29

alice-i-cecile requested a review from james7132 April 11, 2024 10:18

alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Apr 11, 2024

alice-i-cecile reviewed Apr 11, 2024

View reviewed changes

james7132 reviewed Apr 11, 2024

View reviewed changes

re0312 added 3 commits April 12, 2024 13:01

review

adf432b

uncheck

5441550

unsafe

0df35be

re0312 and others added 3 commits April 12, 2024 19:44

fix cli

7dfa1cd

fix cli

5ead4aa

Merge branch 'main' into get_opt

ccb1ce3

james7132 reviewed Apr 14, 2024

View reviewed changes

crates/bevy_ecs/src/query/getter.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved

re0312 added 3 commits April 14, 2024 11:29

address review

17a2121

fix cli

e62244c

fix doc

914a4f9

james7132 requested changes Apr 14, 2024

View reviewed changes

re0312 and others added 2 commits April 14, 2024 21:17

Apply suggestions from code review

0f8b91d

Co-authored-by: James Liu <contact@jamessliu.com>

comment

a80e7ba

re0312 added 3 commits April 14, 2024 21:22

Merge branch 'get_opt' of https://github.com/re0312/bevy into get_opt

f532e4d

review

9e6f9e0

doc

f4416f9

re0312 requested a review from james7132 April 15, 2024 02:15

janhohenheim added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jul 17, 2024

ItsDoot suggested changes Aug 11, 2024

View reviewed changes

BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accelerate Query::get #12925

Accelerate Query::get #12925

re0312 commented Apr 11, 2024 •

edited

Loading

alice-i-cecile Apr 11, 2024

alice-i-cecile Apr 11, 2024

james7132 left a comment

re0312 commented Apr 12, 2024

james7132 commented Apr 14, 2024

re0312 commented Apr 14, 2024

james7132 Apr 14, 2024

james7132 Apr 14, 2024

james7132 Apr 14, 2024

ItsDoot left a comment

ItsDoot Aug 11, 2024

ItsDoot Aug 11, 2024


		use super::{DebugCheckedUnwrap, QueryData, QueryEntityError, QueryFilter, QueryState};

		/// A getter used for fast fetch its Component with Specific [`Entity`]

	pub fn get(&mut self, entity: Entity) -> Result<D::Item<'w>, QueryEntityError> {
	pub fn get(&mut self, entity: Entity) -> Result<D::Item<'_>, QueryEntityError> {

	pub unsafe fn get_unchecked(&mut self, entity: Entity) -> D::Item<'w> {
	pub unsafe fn get_unchecked(&mut self, entity: Entity) -> D::Item<'_> {

-        unsafe { std::mem::transmute(self.get_unsafe(entity)) }
+        let result = unsafe { self.get_unsafe(entity) };
+        match result {
+            Ok(item) => Ok(D::shrink(item)),
+            Err(err) => Err(err),
+        }

Accelerate Query::get #12925

Are you sure you want to change the base?

Accelerate Query::get #12925

Conversation

re0312 commented Apr 11, 2024 • edited Loading

Objective

Solution

Changelog

Performance

alice-i-cecile Apr 11, 2024

Choose a reason for hiding this comment

alice-i-cecile Apr 11, 2024

Choose a reason for hiding this comment

james7132 left a comment

Choose a reason for hiding this comment

re0312 commented Apr 12, 2024

james7132 commented Apr 14, 2024

re0312 commented Apr 14, 2024

james7132 Apr 14, 2024

Choose a reason for hiding this comment

james7132 Apr 14, 2024

Choose a reason for hiding this comment

james7132 Apr 14, 2024

Choose a reason for hiding this comment

ItsDoot left a comment

Choose a reason for hiding this comment

ItsDoot Aug 11, 2024

Choose a reason for hiding this comment

ItsDoot Aug 11, 2024

Choose a reason for hiding this comment

re0312 commented Apr 11, 2024 •

edited

Loading