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

Remove unnecessary branch in query iteration #12844

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

re0312
Copy link
Contributor

@re0312 re0312 commented Apr 2, 2024

Objective

  • Since Reduced TableRow as Casting #10811,Bevy uses assert in the hot path of iteration. The for_each method has an assert in the outer loop to help the compiler remove unnecessary branching in the internal loop.
  • However , for style iterations do not receive the same treatment. it still have a branch check in the internal loop, which could potentially hurt performance.

Solution

  • use TableRow::from_u32 instead of TableRow::from_usize to avoid unnecessary branch.

Before

image

After ----------------------------------------------------------------------------

image

@JMS55 JMS55 requested a review from james7132 April 2, 2024 03:50
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

I'd actually suggest downgrading the assert in TableRow::from_usize to a debug_assert. Anything touching query iteration and storage shouldn't be panicking in a release build (see #12107 and #12149).

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

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Can you also do this for TableId?

@james7132 james7132 added this pull request to the merge queue Apr 2, 2024
Merged via the queue into bevyengine:main with commit 06738bf Apr 2, 2024
27 checks passed
chompaa pushed a commit to chompaa/bevy that referenced this pull request Apr 5, 2024
# Objective

- Since bevyengine#10811,Bevy uses `assert `in the hot path of iteration. The
`for_each `method has an assert in the outer loop to help the compiler
remove unnecessary branching in the internal loop.
- However , ` for` style iterations do not receive the same treatment.
it still have a branch check in the internal loop, which could
potentially hurt performance.

## Solution

- use `TableRow::from_u32 ` instead of ` TableRow::from_usize` to avoid
unnecessary branch.

Before


![image](https://github.com/bevyengine/bevy/assets/45868716/f6d2a1ac-2129-48ff-97bf-d86713ddeaaf)



After
----------------------------------------------------------------------------


![image](https://github.com/bevyengine/bevy/assets/45868716/bfe5a9ee-ba6c-4a80-85b0-1c6d43adfe8c)
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants