Make the pointers in Fetch and Query's iterators unions#6396
Closed
james7132 wants to merge 7 commits intobevyengine:mainfrom
Closed
Make the pointers in Fetch and Query's iterators unions#6396james7132 wants to merge 7 commits intobevyengine:mainfrom
james7132 wants to merge 7 commits intobevyengine:mainfrom
Conversation
james7132
added
A-ECS
| archetype_entities: &[], | ||
| table_id_iter: query_state.matched_table_ids.iter(), | ||
| archetype_id_iter: query_state.matched_archetype_ids.iter(), | ||
| id_iter: if Self::IS_DENSE { |
Member
There was a problem hiding this comment.
This should be a match statement to make adding new storage types less error prone.
Member
There was a problem hiding this comment.
IMO we should actually probably replace the IS_DENSE bool with an enum?
Member
Author
There was a problem hiding this comment.
It'd make the const evals for IS_DENSE for fetches notably harder to read without the current boolean composition.
Also I thought we were avoiding match bool?
Member
There was a problem hiding this comment.
Yeah: that's part of why I suggested an enum ;)
I agree on the const evals though, so I'm fine to leave it. Adding new storage types will be very rare, and even then they'll likely be meaningfully characterized as dense or not.
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Well done, needs more docs. The underlying strategy of using a union here, discriminated based on compile time properties makes a lot of sense to me.
I was a bit nervous that the added complexity would make adding more storage types harder in the future, but overall this seems like it pushes us in the right direction there. More type safety, more clarity, and no per-storage-mode overhead.
Can you run some benchmarks? I'm curious to see if this makes a difference. I think we should do this even if it's perf neutral though: it's pretty clearly the right representation.
Member
Author
|
Did a quick round of microbenchmarks. Changes seem to between a small regression and a sizable gain, for the benchmarks that aren't within the noise threshold. Details |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 27, 2024
…#15283) ## Objective - Adopted #6396 ## Solution Same as #6396, we use a compile-time checked `StorageSwitch` union type to select the fetch data based on the component's storage type, saving >= 8 bytes per component fetch in a given query. Note: We forego the Query iteration change as it exists in a slightly different form now on main. ## Testing - All current tests pass locally. --------- Co-authored-by: james7132 <contact@jamessliu.com> Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
robtfm
pushed a commit
to robtfm/bevy
that referenced
this pull request
Oct 4, 2024
…bevyengine#15283) ## Objective - Adopted bevyengine#6396 ## Solution Same as bevyengine#6396, we use a compile-time checked `StorageSwitch` union type to select the fetch data based on the component's storage type, saving >= 8 bytes per component fetch in a given query. Note: We forego the Query iteration change as it exists in a slightly different form now on main. ## Testing - All current tests pass locally. --------- Co-authored-by: james7132 <contact@jamessliu.com> Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
Contributor
|
Backlog cleanup: closing in favour of adopting PR #15283. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a redo of #5085 and an extension of #4800.
Objective
Every
Fetchstruct initializes pointers for both storages, even though only one is ever really used. This both makes the struct bigger, and adds a minuscule amount of overhead when initializing aFetchas it needs to zero out the unused Fetch impl.Solution
Add a compile-time discriminated
unioncalledStorageSwitchwhich includes the different ways a Fetch could represent a pointer to the respective storage. This allows the pointers to share the same space and there's no redundant unused memory in any of the types. (Sole exception beingWriteFetch, which has oneusizeof unused space when used on sparse components).ReadFetchshould be the same size as a normal pointer.As sparse sets always have a reference populated when making a fetch. They're no longer wrapped in an Option, and do not need any unwrapping.
Co-Authored-By: Boxy supbscripter@gmail.com