From 6e78ad91824a4b0f628049d77bc869d3922189ed Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sun, 10 Sep 2023 17:26:02 +0200 Subject: [PATCH 1/3] Provide getters for fields of ReflectFromPtr The reasoning is similar to #8687. I'm building a dynamic query. Currently, I store the ReflectFromPtr in my dynamic `Fetch` type. However, `ReflectFromPtr` is: - 16 bytes for TypeId - 8 bytes for the non-mutable function pointer - 8 bytes for the mutable function pointer It's a lot, it adds 32 bytes to my base `Fetch` which is only `ComponendId` (8 bytes) for a total of 40 bytes. I only need one function per fetch, reducing the total dynamic fetch size to 16 bytes. Since I'm querying the components by the ComponendId associated with the function pointer I'm using, I don't need the TypeId, it's a redundant check. In fact, I've difficulties coming up with situations where checking the TypeId beforehand is relevant. So to me, if ReflectFromPtr makes sense as a public API, exposing the function pointers also makes sense. --- crates/bevy_reflect/src/type_registry.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/crates/bevy_reflect/src/type_registry.rs b/crates/bevy_reflect/src/type_registry.rs index aeaba79a2f6d5..57c179aa982f5 100644 --- a/crates/bevy_reflect/src/type_registry.rs +++ b/crates/bevy_reflect/src/type_registry.rs @@ -528,8 +528,8 @@ impl Deserialize<'a> + Reflect> FromType for ReflectDeserialize { #[derive(Clone)] pub struct ReflectFromPtr { type_id: TypeId, - to_reflect: for<'a> unsafe fn(Ptr<'a>) -> &'a dyn Reflect, - to_reflect_mut: for<'a> unsafe fn(PtrMut<'a>) -> &'a mut dyn Reflect, + to_reflect: unsafe fn(Ptr) -> &dyn Reflect, + to_reflect_mut: unsafe fn(PtrMut) -> &mut dyn Reflect, } impl ReflectFromPtr { @@ -553,6 +553,26 @@ impl ReflectFromPtr { pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mut dyn Reflect { (self.to_reflect_mut)(val) } + /// Get a function pointer to turn a `Ptr` into `&dyn Reflect` for + /// the type this was constructed for. + /// + /// # Safety + /// When calling the unsafe function returned by this method you must ensure that: + /// - The input `Ptr` points to the `Reflect` type this [`ReflectFromPtr`] + /// was constructed for. + pub fn get_to_reflect(&self) -> unsafe fn(Ptr) -> &dyn Reflect { + self.to_reflect + } + /// Get a function pointer to turn a `PtrMut` into `&mut dyn Reflect` for + /// the type this was constructed for. + /// + /// # Safety + /// When calling the unsafe function returned by this method you must ensure that: + /// - The input `PtrMut` points to the `Reflect` type this [`ReflectFromPtr`] + /// was constructed for. + pub fn get_to_reflect_mut(&self) -> unsafe fn(PtrMut) -> &mut dyn Reflect { + self.to_reflect_mut + } } impl FromType for ReflectFromPtr { From c1314d6ff599c11722e30e51884080d258061822 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Tue, 12 Sep 2023 13:02:02 +0200 Subject: [PATCH 2/3] Rename ReflectFromPtr methods --- crates/bevy_ecs/src/change_detection.rs | 2 +- crates/bevy_reflect/src/type_registry.rs | 46 +++++++++++++----------- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index a00a5cd6bb66b..12e6959e17cd6 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -1164,7 +1164,7 @@ mod tests { let mut new = value.map_unchanged(|ptr| { // SAFETY: The underlying type of `ptr` matches `reflect_from_ptr`. - let value = unsafe { reflect_from_ptr.as_reflect_ptr_mut(ptr) }; + let value = unsafe { reflect_from_ptr.as_reflect_mut(ptr) }; value }); diff --git a/crates/bevy_reflect/src/type_registry.rs b/crates/bevy_reflect/src/type_registry.rs index 57c179aa982f5..479708c949e40 100644 --- a/crates/bevy_reflect/src/type_registry.rs +++ b/crates/bevy_reflect/src/type_registry.rs @@ -521,57 +521,63 @@ impl Deserialize<'a> + Reflect> FromType for ReflectDeserialize { /// let reflect_data = type_registry.get(std::any::TypeId::of::()).unwrap(); /// let reflect_from_ptr = reflect_data.data::().unwrap(); /// // SAFE: `value` is of type `Reflected`, which the `ReflectFromPtr` was created for -/// let value = unsafe { reflect_from_ptr.as_reflect_ptr(value) }; +/// let value = unsafe { reflect_from_ptr.as_reflect(value) }; /// /// assert_eq!(value.downcast_ref::().unwrap().0, "Hello world!"); /// ``` #[derive(Clone)] pub struct ReflectFromPtr { type_id: TypeId, - to_reflect: unsafe fn(Ptr) -> &dyn Reflect, - to_reflect_mut: unsafe fn(PtrMut) -> &mut dyn Reflect, + from_ptr: unsafe fn(Ptr) -> &dyn Reflect, + from_ptr_mut: unsafe fn(PtrMut) -> &mut dyn Reflect, } impl ReflectFromPtr { - /// Returns the [`TypeId`] that the [`ReflectFromPtr`] was constructed for + /// Returns the [`TypeId`] that the [`ReflectFromPtr`] was constructed for. pub fn type_id(&self) -> TypeId { self.type_id } + /// Convert `Ptr` into `&dyn Reflect`. + /// /// # Safety /// /// `val` must be a pointer to value of the type that the [`ReflectFromPtr`] was constructed for. /// This can be verified by checking that the type id returned by [`ReflectFromPtr::type_id`] is the expected one. - pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect { - (self.to_reflect)(val) + pub unsafe fn as_reflect<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect { + (self.from_ptr)(val) } + /// Convert `PtrMut` into `&mut dyn Reflect`. + /// /// # Safety /// /// `val` must be a pointer to a value of the type that the [`ReflectFromPtr`] was constructed for /// This can be verified by checking that the type id returned by [`ReflectFromPtr::type_id`] is the expected one. - pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mut dyn Reflect { - (self.to_reflect_mut)(val) + pub unsafe fn as_reflect_mut<'a>(&self, val: PtrMut<'a>) -> &'a mut dyn Reflect { + (self.from_ptr_mut)(val) } /// Get a function pointer to turn a `Ptr` into `&dyn Reflect` for - /// the type this was constructed for. + /// the type this [`ReflectFromPtr`] was constructed for. /// /// # Safety + /// /// When calling the unsafe function returned by this method you must ensure that: - /// - The input `Ptr` points to the `Reflect` type this [`ReflectFromPtr`] + /// - The input `Ptr` points to the `Reflect` type this `ReflectFromPtr` /// was constructed for. - pub fn get_to_reflect(&self) -> unsafe fn(Ptr) -> &dyn Reflect { - self.to_reflect + pub fn from_ptr(&self) -> unsafe fn(Ptr) -> &dyn Reflect { + self.from_ptr } /// Get a function pointer to turn a `PtrMut` into `&mut dyn Reflect` for - /// the type this was constructed for. + /// the type this [`ReflectFromPtr`] was constructed for. /// /// # Safety + /// /// When calling the unsafe function returned by this method you must ensure that: - /// - The input `PtrMut` points to the `Reflect` type this [`ReflectFromPtr`] + /// - The input `PtrMut` points to the `Reflect` type this `ReflectFromPtr` /// was constructed for. - pub fn get_to_reflect_mut(&self) -> unsafe fn(PtrMut) -> &mut dyn Reflect { - self.to_reflect_mut + pub fn from_ptr_mut(&self) -> unsafe fn(PtrMut) -> &mut dyn Reflect { + self.from_ptr_mut } } @@ -579,12 +585,12 @@ impl FromType for ReflectFromPtr { fn from_type() -> Self { ReflectFromPtr { type_id: std::any::TypeId::of::(), - to_reflect: |ptr| { + from_ptr: |ptr| { // SAFE: only called from `as_reflect`, where the `ptr` is guaranteed to be of type `T`, // and `as_reflect_ptr`, where the caller promises to call it with type `T` unsafe { ptr.deref::() as &dyn Reflect } }, - to_reflect_mut: |ptr| { + from_ptr_mut: |ptr| { // SAFE: only called from `as_reflect_mut`, where the `ptr` is guaranteed to be of type `T`, // and `as_reflect_ptr_mut`, where the caller promises to call it with type `T` unsafe { ptr.deref_mut::() as &mut dyn Reflect } @@ -621,7 +627,7 @@ mod test { { let value = PtrMut::from(&mut value); // SAFETY: reflect_from_ptr was constructed for the correct type - let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_ptr_mut(value) }; + let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_mut(value) }; match dyn_reflect.reflect_mut() { bevy_reflect::ReflectMut::Struct(strukt) => { strukt.field_mut("a").unwrap().apply(&2.0f32); @@ -632,7 +638,7 @@ mod test { { // SAFETY: reflect_from_ptr was constructed for the correct type - let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_ptr(Ptr::from(&value)) }; + let dyn_reflect = unsafe { reflect_from_ptr.as_reflect(Ptr::from(&value)) }; match dyn_reflect.reflect_ref() { bevy_reflect::ReflectRef::Struct(strukt) => { let a = strukt.field("a").unwrap().downcast_ref::().unwrap(); From 2152258324e7f9184e495a5cfb845da1bd934a63 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Mon, 18 Sep 2023 08:19:35 +0200 Subject: [PATCH 3/3] Fix tests compilation --- crates/bevy_ecs/src/change_detection.rs | 2 +- crates/bevy_reflect/src/type_registry.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 12e6959e17cd6..6e2449810cd3f 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -829,7 +829,7 @@ impl<'a> MutUntyped<'a> { /// # let mut_untyped: MutUntyped = unimplemented!(); /// # let reflect_from_ptr: bevy_reflect::ReflectFromPtr = unimplemented!(); /// // SAFETY: from the context it is known that `ReflectFromPtr` was made for the type of the `MutUntyped` - /// mut_untyped.map_unchanged(|ptr| unsafe { reflect_from_ptr.as_reflect_ptr_mut(ptr) }); + /// mut_untyped.map_unchanged(|ptr| unsafe { reflect_from_ptr.as_reflect_mut(ptr) }); /// ``` pub fn map_unchanged(self, f: impl FnOnce(PtrMut<'a>) -> &'a mut T) -> Mut<'a, T> { Mut { diff --git a/crates/bevy_reflect/src/type_registry.rs b/crates/bevy_reflect/src/type_registry.rs index 479708c949e40..b7ce14d8301c6 100644 --- a/crates/bevy_reflect/src/type_registry.rs +++ b/crates/bevy_reflect/src/type_registry.rs @@ -586,13 +586,13 @@ impl FromType for ReflectFromPtr { ReflectFromPtr { type_id: std::any::TypeId::of::(), from_ptr: |ptr| { - // SAFE: only called from `as_reflect`, where the `ptr` is guaranteed to be of type `T`, - // and `as_reflect_ptr`, where the caller promises to call it with type `T` + // SAFETY: `from_ptr_mut` is either called in `ReflectFromPtr::as_reflect` + // or returned by `ReflectFromPtr::from_ptr`, both lay out the invariants + // required by `deref` unsafe { ptr.deref::() as &dyn Reflect } }, from_ptr_mut: |ptr| { - // SAFE: only called from `as_reflect_mut`, where the `ptr` is guaranteed to be of type `T`, - // and `as_reflect_ptr_mut`, where the caller promises to call it with type `T` + // SAFETY: same as above, but foor `as_reflect_mut`, `from_ptr_mut` and `deref_mut`. unsafe { ptr.deref_mut::() as &mut dyn Reflect } }, }