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

Make function pointers of ecs Reflect* public #8687

Merged
merged 1 commit into from
Jun 19, 2023
Merged

Make function pointers of ecs Reflect* public #8687

merged 1 commit into from
Jun 19, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented May 26, 2023 •
edited
Loading

Repetitively fetching ReflectResource and ReflectComponent from the TypeRegistry is costly.

We want to access the underlying fns. to do so, we expose the ReflectResourceFns and ReflectComponentFns stored in ReflectResource and ReflectComponent.

Changelog

  • Add the fn_pointers methods to ReflectResource and ReflectComponent returning the underlying ReflectResourceFns and ReflectComponentFns

@nicopap nicopap added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events A-Reflection Runtime information about types labels May 26, 2023
@nicopap
Copy link
Contributor Author

nicopap commented May 26, 2023 •
edited
Loading

Note, I decided to not implement Deref purely because my IDE didn't auto-import Deref and I went with the assumption that it was for a good reason.

Also implementing it as a method gives the opportunity to tell the user to use the wrapping methods rather than the function pointers. Though it's easy to implement it as a Deref, please tell if I should do that instead.

@MrGVSV
Copy link
Member

MrGVSV commented May 26, 2023 •
edited
Loading

Have you considered just cloning ReflectComponent and ReflectResource? That should have the same effect considering they're just wrappers around their Reflect*Fns data. Or are you trying to store just the pointer directly?

@nicopap
Copy link
Contributor Author

nicopap commented May 26, 2023

I'm trying to just store the pointer directly. Reflect*Fns is 10 different functions (in the case of ReflectComponentsFns). I only need one of them. storing the ReflectComponent directly is a lot of dead weight. Especially for my use case where I store a list of those.

MrGVSV
MrGVSV approved these changes May 26, 2023
View reviewed changes
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Just a couple thoughts, but a sensible change overall.

crates/bevy_ecs/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/reflect.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile self-requested a review June 10, 2023 06:19
@nicopap
Make function pointers of ecs Reflect* public
b99cdf7 
This allows caching the function pointers, preventing the overhead of
fetching the ReflectResource from the type registry.
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 19, 2023
@alice-i-cecile
Copy link
Member

Nice and simple. Because this is advanced I think I prefer the explicit getter method over the very implicit Deref.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 19, 2023
Merged via the queue into bevyengine:main with commit 28e9c52 Jun 19, 2023
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 19, 2023
@nicopap @james7132
Make function pointers of ecs Reflect* public (bevyengine#8687)
2fa82e0 
Repetitively fetching ReflectResource and ReflectComponent from the
TypeRegistry is costly.

We want to access the underlying `fn`s. to do so, we expose the
`ReflectResourceFns` and `ReflectComponentFns` stored in ReflectResource
and ReflectComponent.

---

## Changelog

- Add the `fn_pointers` methods to `ReflectResource` and
`ReflectComponent` returning the underlying `ReflectResourceFns` and
`ReflectComponentFns`
@nicopap nicopap deleted the public-reflect-fns branch August 30, 2023 13:43
nicopap added a commit to nicopap/bevy that referenced this pull request Sep 10, 2023
@nicopap
Provide getters for fields of ReflectFromPtr
81a94a8 
The reasoning is similar to bevyengine#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.
@nicopap nicopap mentioned this pull request Sep 10, 2023
Merged
nicopap added a commit to nicopap/bevy that referenced this pull request Sep 12, 2023
@nicopap
Provide getters for fields of ReflectFromPtr
6e78ad9 
The reasoning is similar to bevyengine#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.
github-merge-queue bot pushed a commit that referenced this pull request Sep 18, 2023
@nicopap
Provide getters for fields of ReflectFromPtr (#9748)
0bd4ea7 
# Objective

The reasoning is similar to #8687.

I'm building a dynamic query. Currently, I store the ReflectFromPtr in
my dynamic `Fetch` type.

[See relevant
code](https://github.com/nicopap/bevy_mod_dynamic_query/blob/97ba68ae1e13f15cabfac1dcd58c2483396dfd3f/src/fetches.rs#L14-L17)

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.

## Solution

- Make the fields public through methods.

---

## Changelog

- Add `from_ptr` and `from_ptr_mut` methods to `ReflectFromPtr` to
access the underlying function pointers
- `ReflectFromPtr::as_reflect_ptr` is now `ReflectFromPtr::as_reflect`
- `ReflectFromPtr::as_reflect_ptr_mut` is now
`ReflectFromPtr::as_reflect_mut`

## Migration guide

- `ReflectFromPtr::as_reflect_ptr` is now `ReflectFromPtr::as_reflect`
- `ReflectFromPtr::as_reflect_ptr_mut` is now
`ReflectFromPtr::as_reflect_mut`
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@nicopap
Provide getters for fields of ReflectFromPtr (bevyengine#9748)
835fc96 
# Objective

The reasoning is similar to bevyengine#8687.

I'm building a dynamic query. Currently, I store the ReflectFromPtr in
my dynamic `Fetch` type.

[See relevant
code](https://github.com/nicopap/bevy_mod_dynamic_query/blob/97ba68ae1e13f15cabfac1dcd58c2483396dfd3f/src/fetches.rs#L14-L17)

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.

## Solution

- Make the fields public through methods.

---

## Changelog

- Add `from_ptr` and `from_ptr_mut` methods to `ReflectFromPtr` to
access the underlying function pointers
- `ReflectFromPtr::as_reflect_ptr` is now `ReflectFromPtr::as_reflect`
- `ReflectFromPtr::as_reflect_ptr_mut` is now
`ReflectFromPtr::as_reflect_mut`

## Migration guide

- `ReflectFromPtr::as_reflect_ptr` is now `ReflectFromPtr::as_reflect`
- `ReflectFromPtr::as_reflect_ptr_mut` is now
`ReflectFromPtr::as_reflect_mut`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@MrGVSV MrGVSV MrGVSV approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events A-Reflection Runtime information about types C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nicopap @MrGVSV @alice-i-cecile