Mitigate stack overflow on large Bundle inserts

[james7132]

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

[SkiFire13]

I quickly skimmed through the second half of the PR so something might be missing there. If this PR alone fixes the stack overflow I feel like we shouldn't deal with unaligned pointers then (see all the comments with the issues they create)

[james7132]

Have you run benches with the MovingPtr changes? I would still expect the changes to be in the noise, but should probably check.

I haven't run benchmarks since #20772 (comment), but I'm fairly confident for small bundles it's still in the noise. For very small bundles (smaller than a pointer), this may be a unnoticeable regression since we're technically moving more on the stack in those cases, but the cost there should be dwarfed by all the metadata shuffling that insertion needs to wrangle.

One thing I should do is run a bevy_asm_test check against this PR to see if it tangibly changes any of the compiler output.

[james7132]

One thing I should do is run a bevy_asm_test check against this PR to see if it tangibly changes any of the compiler output.

I went ahead and tested this: james7132/bevy_asm_tests@main...stack-overflow-fix. On release, opt-level 3, and fat LTO, there's very little difference. A few extra mov instructions are omitted but that shouldn't have a huge impact on the final output. This likely only has perf improvements in non-release builds.

[cart]

I think it is worth leaving a note here about why we're using MovingPtr here, with a link to the relevant issue.

[james7132]

Added a few non-doc comments to the trait functions where it's relevant.

[cart]

Taking on some complexity to mitigate stack overflows feels worth it. Reasonably straightforward in its current form. I don't love the new unnecessary archetype move / redundant bookkeeping (see my reply in the thread above) ... I think it points to missing tools in our toolkit. I'm willing to make that trade in the short term in the interest of moving forward, but I've opened #20976 to track this, and added it to the 0.18 milestone.

[chescock]

I read the code, but not the full comment thread, so I apologize in advance if I wound up repeating something that was already discussed and explained!

[chescock]

It's odd for a safety requirement to say "should". Should this say "must", or should it be a non-safety doc comment?

Is failing to call apply_effect meant to be unsound, or just a safe leak like mem::forget?

[james7132]

You're right. I changed it to say "must" instead of should, but it's a "at most once before returning to user-space safe code".

[chescock]

Does it still make sense to have Effect as an associated type here? It doesn't seem like it actually gets used, apart from the NoBundleEffect trait bound, which could be moved to Self. We should certainly leave it there for now to make this change smaller, but we may want to remove it as a follow up.

[james7132]

Yep, this is borderline vestigial at this point, but those generic constraints are required for user facing APIs for now. @janis-bhm comment in the same PR notes we should probably keep something like it and return it from get_components instead of using partial_move where we're doing it now, but I ran into a few lifetime tangles along the way trying to implement that. I'll file a separate issue to address this at a later point.

[chescock]

These constraints are a bug fix, because previously the code allowed bundles with effects but ignored the effects, right?

[james7132]

From a user-facing perspective, no, since this type isn't public and we only used it in spaces where that invariant held anyway, but from the perspective easily provable safety invariants, yes.