Skip to content

Conversation

@james7132
Copy link
Member

@james7132 james7132 commented Aug 15, 2025

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 the World anyway.

Solution

Add a new function to Command called apply_raw that takes the unaligned pointer provided by CommandQueue. Provide a default implementation for it using Command::apply. Rework (Raw)CommandQueue::push to work in terms of apply_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 via read_unaligned, but the unaligned resource stored in the command was directly fed to World::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:

  • Implement this extension for EntityCommands, specifically EntityCommands::insert and friends.
  • Benchmark

Testing

Ran bevy_ecs tests and miri.

Future Considerations

  • See if we can get around repeated CommandQueue reallocations without keeping permanently large command buffers around by using bumpalo or implementing our own bump allocation strategy within CommandQueue.
@james7132 james7132 added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Aug 15, 2025
@james7132 james7132 added this to the 0.18 milestone Aug 15, 2025
@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Aug 15, 2025
@james7132 james7132 added D-Complex Quite challenging from either a design or technical perspective. Ask for help! D-Unsafe Touches with unsafe code in some way labels Aug 15, 2025
@james7132
Copy link
Member Author

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.

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 <alice.i.cecile@gmail.com> Co-authored-by: Giacomo Stevanato <giaco.stevanato@gmail.com> Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com> Co-authored-by: Sandor <alexaegis@pm.me>
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 <janis@nirgendwo.xyz> --------- Co-authored-by: Talin <viridia@gmail.com> Co-authored-by: Janis <janis@nirgendwo.xyz> Co-authored-by: Janis <130913856+janis-bhm@users.noreply.github.com> Co-authored-by: Kristoffer Søholm <k.soeholm@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Mike <mike.hsu@gmail.com> Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
@james7132 james7132 closed this Sep 13, 2025
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

2 participants