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

Improve codegen for world validation #9464

Merged
merged 6 commits into from
Sep 21, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Aug 17, 2023

Objective

Improve code-gen for QueryState::validate_world and SystemState::validate_world.

Solution

  • Move panics into separate, non-inlined functions, to reduce the code size of the outer methods.
  • Mark the panicking functions with #[cold] to help the compiler optimize for the happy path.
  • Mark the functions with #[track_caller] to make debugging easier.

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A simple quality-of-life change that makes Bevy easier to use labels Aug 17, 2023
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

I think I remember this being better, and it's a pattern already used in Bevy, but don't remember if it has any actual impact

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.

Small style nits. The motivation and logic behind the change makes sense to me. Could you rebase this PR to the latest main? I want to test this against bevy_asm_tests to see the output difference.

crates/bevy_ecs/src/query/state.rs Show resolved Hide resolved
crates/bevy_ecs/src/system/function_system.rs Show resolved Hide resolved
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.

It does seem to work. There was a rough 300 fewer lines of assembly generated across several test cases for QueryState, a 14% decrease in codegen. Though the bulk of that seems to be in the symbols for panicking, this is still a decent win.

I'll kick off a merge once CI is fixed.

@james7132 james7132 added this pull request to the merge queue Sep 21, 2023
Merged via the queue into bevyengine:main with commit e60249e Sep 21, 2023
21 checks passed
@JoJoJet JoJoJet deleted the optimize_validate_world branch September 21, 2023 21:28
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Improve code-gen for `QueryState::validate_world` and
`SystemState::validate_world`.

## Solution

* Move panics into separate, non-inlined functions, to reduce the code
size of the outer methods.
* Mark the panicking functions with `#[cold]` to help the compiler
optimize for the happy path.
* Mark the functions with `#[track_caller]` to make debugging easier.

---------

Co-authored-by: James Liu <contact@jamessliu.com>
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 C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants