Skip to content

Turn apply_deferred into a ZST System #16642

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.

Sign up for GitHub

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

Jump to bottom
Merged
alice-i-cecile merged 8 commits into from
Dec 5, 2024
Merged

Turn apply_deferred into a ZST System #16642

alice-i-cecile merged 8 commits into from
Dec 5, 2024

Conversation

@ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Dec 4, 2024 •
edited
Loading

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 commented Dec 4, 2024
View reviewed changes
@ItsDoot ItsDoot added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Dec 4, 2024
ItsDoot
ItsDoot commented Dec 4, 2024
View reviewed changes
bushrat011899
bushrat011899 approved these changes Dec 4, 2024
View reviewed changes
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.

@ItsDoot ItsDoot mentioned this pull request Dec 4, 2024
Draft
@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
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@ItsDoot @ecoskey
Turn apply_deferred into a ZST System (bevyengine#16642)
25203c6 
# Objective

- Required by bevyengine#16622 due to differing implementations of `System` by
`FunctionSystem` and `ExclusiveFunctionSystem`.
- Optimize the memory usage of instances of `apply_deferred` in system
schedules.

## 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hymm hymm Awaiting requested review from hymm

3 more reviewers

@chescock chescock chescock approved these changes

@urben1680 urben1680 urben1680 left review comments

@bushrat011899 bushrat011899 bushrat011899 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ItsDoot @bushrat011899 @chescock @urben1680 @alice-i-cecile