-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Introduce Command::apply_raw #20593
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
Closed
Closed
Introduce Command::apply_raw #20593
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I went ahead and tried piping this through (Dynamic)Bundle. There's some UB with how it affects insertion of relationships, will need to investigate. |
This was referenced Aug 24, 2025
This was referenced Sep 5, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 7, 2025
…raw pointer (#20877) # Objective Make moving potentially large values, like those seen in #20571 and those seen by #20772, safer and easier to review. ## Solution Introduce `MovingPtr<'a, T>` as a wrapper around `NonNull<T>`. This type: - Wraps a pointer and is thus cheap to pass through to functions. - Acts like a `Box<T>` that does not own the allocation it points to. It will drop the value it points to when it's dropped, but will not deallocate when it's dropped. - Acts like a `OwningPtr` in that it owns the values it points to and has an associated lifetime, but it has a concrete type. - As it owns the value, it does not implement `Clone` or `Copy`. - Does not support arbitrary pointer arithmetic other than to get `MovingPtr`s of the value's fields. - Does not support casting to types other than `ManuallyDrop<T>` and `MaybeUninit<T>`. - Has methods that consume the `MovingPtr` that copies the value into a target pointer or reads it onto the stack. - Provide unsafe functions for partially moving values of members out and returns a `MovingPtr<'a, MaybeUninit<T>>` in its stead. - Optionally supports unaligned pointers like `OwningPtr` for use cases like #20593. - Provides `From` impl for converting to `OwningPtr` to type erasure without loss of the lifetime or alignment requirements. - Provides a `TryFrom` impl to attempt to convert an unaligned instance into a aligned one. Can be combined with `DebugCheckedUnwrap` to assert that the conversion is sound. - The `deconstruct_moving_ptr` provides a less error-prone way of decomposing a `MovingPtr` into separate `MovingPtr` for its fields. This design is loosely based on the outptr proposal for [in-place construction](rust-lang/lang-team#336 (comment)), but currently eschews the requirements for a derive macro. ## Testing CI, new doc tests pass. --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Giacomo Stevanato <[email protected]> Co-authored-by: Chris Russell <[email protected]> Co-authored-by: Sandor <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 12, 2025
# Objective Fix #20571. ## Solution * Avoid passing the bundle by value further than one layer deep, and pass a `MovingPtr<'_, T>` of the Bundle instead. * Pass `MovingPtr<'_, Self>` to `DynamicBundle::get_components` and its recursive children. * Instead of returning an `BundleEffect`, directly apply the effect from a `MovingPtr<'_, MaybeUninit<Self>>`. * Remove the now unused `BundleEffect` trait. This should avoid most if not all extra stack copies of the bundle and its components. This won't 100% fix stack overflows via bundles, but it does mitigate them until much larger bundles are used. This started as a subset of the changes made in #20593. ## Testing Ran `cargo r --example feathers --features="experimental_bevy_feathers"` on Windows, no stack overflow. Co-Authored By: janis <[email protected]> --------- Co-authored-by: Talin <[email protected]> Co-authored-by: Janis <[email protected]> Co-authored-by: Janis <[email protected]> Co-authored-by: Kristoffer Søholm <[email protected]> Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Mike <[email protected]> Co-authored-by: Chris Russell <[email protected]>
a6b342f
to
069fd87
Compare
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
D-Complex
Quite challenging from either a design or technical perspective. Ask for help!
D-Unsafe
Touches with unsafe code in some way
S-Waiting-on-Author
The author needs to make changes or address concerns before this can be merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Partially address #20420. Command application currently requires a
ptr::read_unaligned
call ,which reads a complete copy of the command onto the stack before applying it to the World. This require quite a bit of extra memory read and written when the bundle or resource is otherwise very large, particularly if that memory is just going to be moved into theWorld
anyway.Solution
Add a new function to
Command
calledapply_raw
that takes the unaligned pointer provided byCommandQueue
. Provide a default implementation for it usingCommand::apply
. Rework(Raw)CommandQueue::push
to work in terms ofapply_raw
.This is an optional optimization and does not always need to be implemented by all
Command
types, just those likely to move a large amount of memory around.An example of how to evade this extra copy was implemented for
insert_resource
, where the relevant metadata was pulled viaread_unaligned
, but the unaligned resource stored in the command was directly fed toWorld::insert_resource_by_id
to skip the stack copy.This unfortunately means we cannot use closure based Commands for these command types, but thankfully the number of performance sensitive commands like these is fairly low, and the concrete types do not need to be exposed as a public part of the interface.
TODO:
EntityCommands
, specificallyEntityCommands::insert
and friends.Testing
Ran bevy_ecs tests and miri.
Future Considerations
CommandQueue
reallocations without keeping permanently large command buffers around by usingbumpalo
or implementing our own bump allocation strategy withinCommandQueue
.