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

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e94367e
Use T::Storage::STORAGE_TYPE over Components
james7132 Dec 10, 2022
d1e005b
Formatting
james7132 Dec 10, 2022
92e456e
Formatting the safety comment
james7132 Dec 10, 2022
02d0367
Make get_components safe
james7132 Dec 11, 2022
22aae4a
Fix typo.
james7132 Dec 11, 2022
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
11 changes: 9 additions & 2 deletions crates/bevy_ecs/macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
let struct_name = &ast.ident;

TokenStream::from(quote! {
/// SAFETY: ComponentId is returned in field-definition-order. [from_components] and [get_components] use field-definition-order
// SAFETY:
// - ComponentId is returned in field-definition-order. [from_components] and [get_components] use field-definition-order
// - `Bundle::get_components` is exactly once for each member. Rely's on the Component -> Bundle implementation to properly pass
// the correct `StorageType` into the callback.
unsafe impl #impl_generics #ecs_path::bundle::Bundle for #struct_name #ty_generics #where_clause {
fn component_ids(
components: &mut #ecs_path::component::Components,
Expand All @@ -191,7 +194,11 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
}

#[allow(unused_variables)]
fn get_components(self, func: &mut impl FnMut(#ecs_path::ptr::OwningPtr<'_>)) {
#[inline]
fn get_components(
self,
func: &mut impl FnMut(#ecs_path::component::StorageType, #ecs_path::ptr::OwningPtr<'_>)
) {
#(#field_get_components)*
}
}
Expand Down
60 changes: 25 additions & 35 deletions crates/bevy_ecs/src/bundle.rs
View file
Open in desktop
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 @@ -130,7 +130,6 @@ use std::{any::TypeId, collections::HashMap};
/// That is, there is no safe way to implement this trait, and you must not do so.
/// If you want a type to implement [`Bundle`], you must use [`derive@Bundle`](derive@Bundle).
///
///
/// [`Query`]: crate::system::Query
// Some safety points:
// - [`Bundle::component_ids`] must return the [`ComponentId`] for each component type in the
Expand Down Expand Up @@ -159,15 +158,19 @@ pub unsafe trait Bundle: Send + Sync + 'static {
F: for<'a> FnMut(&'a mut T) -> OwningPtr<'a>,
Self: Sized;

// SAFETY:
// The `StorageType` argument passed into [`Bundle::get_components`] must be correct for the
// component being fetched.
//
/// Calls `func` on each value, in the order of this bundle's [`Component`]s. This passes
/// ownership of the component values to `func`.
#[doc(hidden)]
fn get_components(self, func: &mut impl FnMut(OwningPtr<'_>));
fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>));
}

// SAFETY:
// - `Bundle::component_ids` calls `ids` for C's component id (and nothing else)
// - `Bundle::get_components` is called exactly once for C.
// - `Bundle::get_components` is called exactly once for C and passes the component's storage type based on it's associated constant.
// - `Bundle::from_components` calls `func` exactly once for C, which is the exact value returned by `Bundle::component_ids`.
unsafe impl<C: Component> Bundle for C {
fn component_ids(
Expand All @@ -188,8 +191,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]
fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) {
OwningPtr::make(self, |ptr| func(C::Storage::STORAGE_TYPE, ptr));
}
}

Expand All @@ -199,6 +203,8 @@ macro_rules! tuple_impl {
// - `Bundle::component_ids` calls `ids` for each component type in the
// bundle, in the exact order that `Bundle::get_components` is called.
// - `Bundle::from_components` calls `func` exactly once for each `ComponentId` returned by `Bundle::component_ids`.
// - `Bundle::get_components` is exactly once for each member. Rely's on the above implementation to pass the correct
james7132 marked this conversation as resolved.
Show resolved Hide resolved
// `StorageType` into the callback.
unsafe impl<$($name: Bundle),*> Bundle for ($($name,)*) {
#[allow(unused_variables)]
fn component_ids(components: &mut Components, storages: &mut Storages, ids: &mut impl FnMut(ComponentId)){
Expand All @@ -217,7 +223,8 @@ macro_rules! tuple_impl {
}

#[allow(unused_variables, unused_mut)]
fn get_components(self, func: &mut impl FnMut(OwningPtr<'_>)) {
#[inline(always)]
fn get_components(self, func: &mut impl FnMut(StorageType, OwningPtr<'_>)) {
#[allow(non_snake_case)]
let ($(mut $name,)*) = self;
$(
Expand Down Expand Up @@ -254,7 +261,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 +274,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 +387,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 +403,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 +711,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 +729,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 +739,5 @@ unsafe fn initialize_bundle(
bundle_type_name
);

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