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.

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

[Merged by Bors] - Remove unnecessary branching from bundle insertion #6902

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
6 changes: 5 additions & 1 deletion crates/bevy_ecs/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,11 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
}

#[allow(unused_variables)]
fn get_components(self, func: &mut impl FnMut(#ecs_path::ptr::OwningPtr<'_>)) {
#[inline]
unsafe fn get_components(
self,
func: &mut impl FnMut(#ecs_path::component::StorageType, #ecs_path::ptr::OwningPtr<'_>)
) {
#(#field_get_components)*
}
}
Expand Down
55 changes: 22 additions & 33 deletions crates/bevy_ecs/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
Archetype, ArchetypeId, ArchetypeRow, Archetypes, BundleComponentStatus, ComponentStatus,
SpawnBundleStatus,
},
component::{Component, ComponentId, Components, StorageType, Tick},
component::{Component, ComponentId, ComponentStorage, Components, StorageType, Tick},
entity::{Entities, Entity, EntityLocation},
storage::{SparseSetIndex, SparseSets, Storages, Table, TableRow},
};
Expand Down Expand Up @@ -161,8 +161,12 @@ pub unsafe trait Bundle: Send + Sync + 'static {

/// Calls `func` on each value, in the order of this bundle's [`Component`]s. This passes
/// ownership of the component values to `func`.
///
/// # Safety
///
/// The passed in `StorageType` needs to be correct for the provided component
james7132 marked this conversation as resolved.
Show resolved Hide resolved
#[doc(hidden)]
fn get_components(self, func: &mut impl FnMut(OwningPtr<'_>));
unsafe fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>));
}

// SAFETY:
Expand All @@ -188,8 +192,9 @@ unsafe impl<C: Component> Bundle for C {
func(ctx).read()
}

fn get_components(self, func: &mut impl FnMut(OwningPtr<'_>)) {
OwningPtr::make(self, func);
#[inline]
unsafe fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) {
OwningPtr::make(self, |ptr| func(C::Storage::STORAGE_TYPE, ptr));
}
}

Expand Down Expand Up @@ -217,7 +222,8 @@ macro_rules! tuple_impl {
}

#[allow(unused_variables, unused_mut)]
fn get_components(self, func: &mut impl FnMut(OwningPtr<'_>)) {
#[inline(always)]
unsafe fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) {
#[allow(non_snake_case)]
let ($(mut $name,)*) = self;
$(
Expand Down Expand Up @@ -254,7 +260,6 @@ impl SparseSetIndex for BundleId {
pub struct BundleInfo {
pub(crate) id: BundleId,
pub(crate) component_ids: Vec<ComponentId>,
pub(crate) storage_types: Vec<StorageType>,
}

impl BundleInfo {
Expand All @@ -268,11 +273,6 @@ impl BundleInfo {
&self.component_ids
}

#[inline]
pub fn storage_types(&self) -> &[StorageType] {
&self.storage_types
}

pub(crate) fn get_bundle_inserter<'a, 'b>(
&'b self,
entities: &'a mut Entities,
Expand Down Expand Up @@ -386,9 +386,9 @@ impl BundleInfo {
// NOTE: get_components calls this closure on each component in "bundle order".
// bundle_info.component_ids are also in "bundle order"
let mut bundle_component = 0;
bundle.get_components(&mut |component_ptr| {
bundle.get_components(&mut |storage_type, component_ptr| {
let component_id = *self.component_ids.get_unchecked(bundle_component);
match self.storage_types[bundle_component] {
match storage_type {
StorageType::Table => {
let column = table.get_column_mut(component_id).unwrap();
// SAFETY: bundle_component is a valid index for this bundle
Expand All @@ -402,8 +402,11 @@ impl BundleInfo {
}
}
StorageType::SparseSet => {
let sparse_set = sparse_sets.get_mut(component_id).unwrap();
sparse_set.insert(entity, component_ptr, change_tick);
sparse_sets.get_mut(component_id).unwrap().insert(
entity,
component_ptr,
change_tick,
);
}
}
bundle_component += 1;
Expand Down Expand Up @@ -707,10 +710,9 @@ impl Bundles {
let mut component_ids = Vec::new();
T::component_ids(components, storages, &mut |id| component_ids.push(id));
let id = BundleId(bundle_infos.len());
// SAFETY: T::component_id ensures info was created
let bundle_info = unsafe {
initialize_bundle(std::any::type_name::<T>(), component_ids, id, components)
};
let bundle_info =
// SAFETY: T::component_id ensures info was created
unsafe { initialize_bundle(std::any::type_name::<T>(), component_ids, id) };
bundle_infos.push(bundle_info);
id
});
Expand All @@ -726,16 +728,7 @@ unsafe fn initialize_bundle(
bundle_type_name: &'static str,
component_ids: Vec<ComponentId>,
id: BundleId,
components: &mut Components,
) -> BundleInfo {
let mut storage_types = Vec::new();

for &component_id in &component_ids {
// SAFETY: component_id exists and is therefore valid
let component_info = components.get_info_unchecked(component_id);
storage_types.push(component_info.storage_type());
}

let mut deduped = component_ids.clone();
deduped.sort();
deduped.dedup();
Expand All @@ -745,9 +738,5 @@ unsafe fn initialize_bundle(
bundle_type_name
);

BundleInfo {
id,
component_ids,
storage_types,
}
BundleInfo { id, component_ids }
}