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 <[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]>
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants