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

Turn apply_deferred into a ZST System #16642

Merged
merged 8 commits into from
Dec 5, 2024

Conversation

ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Dec 4, 2024

Objective

Solution

By changing apply_deferred from being an ordinary system that ends up as an ExclusiveFunctionSystem, and instead into a ZST struct that implements System manually, we save ~320 bytes per instance of apply_deferred in any schedule.

Testing

  • All current tests pass.

Migration Guide

  • If you were previously calling the special apply_deferred system via apply_deferred(world), don't.

@ItsDoot ItsDoot added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Dec 4, 2024
@ItsDoot ItsDoot added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Dec 4, 2024
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Nice! I'm in favour of a breaking change to rename this ApplyDeferred. Users aren't supposed to actually call this as a function anyway, and giving it a PascalCase name should help highlight to users that it isn't a normal system.

Memory saving is also nice, especially since this system will be reused a lot in a typical application.

crates/bevy_ecs/src/schedule/executor/mod.rs Outdated Show resolved Hide resolved
@ItsDoot ItsDoot 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 Dec 4, 2024
@ItsDoot
Copy link
Contributor Author

ItsDoot commented Dec 5, 2024

I've gone ahead and renamed apply_deferred to ApplyDeferred and added a deprecated alias for the old identifier. It's not a big lift on our end to include it (4 lines), so for the odd dev still using apply_deferred directly, this will give them an immediate actionable solution.

@ItsDoot ItsDoot added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 5, 2024
@ItsDoot ItsDoot requested a review from hymm December 5, 2024 00:53
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 5, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 5, 2024
Merged via the queue into bevyengine:main with commit f87b9fe Dec 5, 2024
33 of 34 checks passed
@ItsDoot ItsDoot deleted the apply-deferred-memory branch December 13, 2024 16:48
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-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants