Implement Drop for CommandQueue
#10746
Conversation
| @@ -9,8 +9,11 @@ struct CommandMeta { | |||
| /// SAFETY: The `value` must point to a value of type `T: Command`, | |||
| /// where `T` is some specific type that was used to produce this metadata. | |||
| /// | |||
| /// `world` is optional to allow this one function pointer to perform double-duty as a drop. | |||
There was a problem hiding this comment.
I've done this to ensure CommandMeta is still exactly 8 bytes in size. Since the inner Drop implementation for C is specific to C, this is the only way to capture it without making CommandMeta at least 16 bytes large.
| /// Returns the size of `T` in bytes. | ||
| apply_command_and_get_size: unsafe fn(value: OwningPtr<Unaligned>, world: &mut World) -> usize, | ||
| consume_command_and_get_size: | ||
| unsafe fn(value: OwningPtr<Unaligned>, world: &mut Option<&mut World>) -> usize, |
There was a problem hiding this comment.
Using &mut Option<&mut World> to allow for the return of the Option<&mut World> back to the CommandQueue after consume_command_and_get_size is done with it.
There was a problem hiding this comment.
You can avoid the extra &mut by reborrowing the &mut World before calling consume_command_and_get_size, for example by passing world.as_deref_mut()
There was a problem hiding this comment.
I've reduced the usage of the &mut Option<&mut World>, but I can't fully eliminate it. CommandQueue itself is only passed a mutable reference to World, and I don't see a way to convert &mut World into &mut Option<World>. In fact, I think such a transform is nonsensical.
| /// Returns the size of `T` in bytes. | ||
| apply_command_and_get_size: unsafe fn(value: OwningPtr<Unaligned>, world: &mut World) -> usize, | ||
| consume_command_and_get_size: |
There was a problem hiding this comment.
I've renamed this field for clarity.
| // Apply command to the provided world... | ||
| Some(world) => command.apply(world), | ||
| // ...or discard it. | ||
| None => drop(command), |
There was a problem hiding this comment.
An explicit drop call is not required, since it will be implicitly called once the scope of this closure ends. I've added it here purely for clarity of purpose.
ItsDoot
added
C-Bug
james7132
added
the
S-Needs-Benchmarking
|
The extra branch here on each command is in the hotpath for command application. Please run the command microbenchmarks and/or the stress tests to see if this has a tangible impact on that perf. |
|
Ok so I tried comparing this change to the Bench OutputTo me, it looks like
It appears that the added branch is being falsely predicted, and the check does add an increase in execution time. If anyone else can replicate this behaviour, I'll mark this as Draft and work on an alternate solution that avoids any changes on the hot path. If anyone has some more specific benchmarking instructions I could follow I'd really appreciate it. I'm still fairly new to Criterion, and this is my first time benchmarking Bevy as well. |
|
Looking into the |
|
Running just the Bench Output
If this is still an unacceptable level of regression, one alternative is to increase the size of |
|
Ok adding an extra function pointer to A near 100% regressionIt makes sense why adding it tanks performance, since most of that benchmark is just reading and writing the |
james7132
removed
the
S-Needs-Benchmarking
…re possible Also removed one branch from `apply`. Revert "Updated `CommandQueue` Changes to avoid `&mut Option<&mut World>` where possible"
bushrat011899
force-pushed
the
ImplementDropForCommandQueue
branch
from
baa628f to
74de7b2
Compare
alice-i-cecile
added
the
S-Ready-For-Final-Review
| @@ -105,13 +105,14 @@ impl CommandQueue { | |||
| // flush the previously queued entities | |||
| world.flush(); | |||
|
|
|||
| self.apply_maybe_world(Some(world)); | |||
| self.apply_or_drop(Some(world)); | |||
There was a problem hiding this comment.
Fair enough, that does read a lot better!
|
Renamed again to |
# Objective - Fixes #10676, preventing a possible memory leak for commands which owned resources. ## Solution Implemented `Drop` for `CommandQueue`. This has been done entirely in the private API of `CommandQueue`, ensuring no breaking changes. Also added a unit test, `test_command_queue_inner_drop_early`, based on the reproduction steps as outlined in #10676. ## Notes I believe this can be applied to `0.12.1` as well, but I am uncertain of the process to make that kind of change. Please let me know if there's anything I can do to help with the back-porting of this change. --------- Co-authored-by: Carter Anderson <[email protected]>
Objective
CommandQueueleaks all containedCommands #10676, preventing a possible memory leak for commands which owned resources.Solution
Implemented
DropforCommandQueue. This has been done entirely in the private API ofCommandQueue, ensuring no breaking changes. Also added a unit test,test_command_queue_inner_drop_early, based on the reproduction steps as outlined in #10676.Notes
I believe this can be applied to
0.12.1as well, but I am uncertain of the process to make that kind of change. Please let me know if there's anything I can do to help with the back-porting of this change.