Skip to content
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.

Sign up for GitHub

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

Fix double indirection when applying command queues #11822

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Fix double indirection when applying command queues #11822

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/system/commands/command_queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ struct CommandMeta {
///
/// Returns the size of `T` in bytes.
consume_command_and_get_size:
unsafe fn(value: OwningPtr<Unaligned>, world: &mut Option<&mut World>) -> usize,
unsafe fn(value: OwningPtr<Unaligned>, world: Option<&mut World>) -> usize,
}

/// Densely and efficiently stores a queue of heterogenous types implementing [`Command`].
Expand Down Expand Up @@ -157,7 +157,7 @@ impl CommandQueue {
// SAFETY: The data underneath the cursor must correspond to the type erased in metadata,
// since they were stored next to each other by `.push()`.
// For ZSTs, the type doesn't matter as long as the pointer is non-null.
let size = unsafe { (meta.consume_command_and_get_size)(cmd, &mut world) };
let size = unsafe { (meta.consume_command_and_get_size)(cmd, world.as_deref_mut()) };
Copy link
Member

@james7132 james7132 Feb 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch on this, though I wonder if the compiler will lift the branch out of the loop. Either way, I don't think this is the bottleneck when it comes to command application given what commands typically entail, so probably not a blocker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to remove two mov instructions from outside of the hot loop: james7132/bevy_asm_tests@main...remove-double-deref#diff-8e36b64d5c51682bb463a51ec6e670b2ec99545945bc4753f4e77ef274c5faa0L27. This likely has a stronger impact inside of the commands themselves.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this introduces a branch -- as_deref_mut() should be a no-op here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet the moves getting removed are due to the fact that the compiler no longer has to deal with the possibility of a command swapping the world from None to Some or vice versa.

// Advance the cursor past the command. For ZSTs, the cursor will not move.
// At this point, it will either point to the next `CommandMeta`,
// or the cursor will be out of bounds and the loop will end.
Expand Down