Skip to content
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.

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

Use dense fetches in Query::get and Query::iter_many #12501

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

james7132
Copy link
Member

Objective

Found while investigating #12476 and #12474. Apparently the Rust compiler cannot optimize out the archetype fetch if it's not used by the fetch type in QueryState::get_unchecked_manual.

Solution

Use the same technique for branching on constants to use dense fetches instead of archetypal fetches. The generated assembly confirms that there's one less memory access for calls to Query::get on dense queries.

Performance

TODO: Microbenchmark

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Mar 15, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Looks fine to me, pending neutral-to-positive microbenchmarks.

@re0312
Copy link
Contributor

re0312 commented Apr 3, 2024

did a benchmark for this,but the results appear strange as there was no noticeable performance gain, even worse than main in table-storage.
image

@alice-i-cecile alice-i-cecile added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Apr 3, 2024
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 S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants