Cache component columns in archetypes #19096
Open
Conversation
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
This is still worth pursuing, but it should be separate
ElliottjPierce
added
A-ECS
|
Note also that #19063 is generally slightly faster across the board, but not by a lot in most places. Also, does this need a migration guide? It removes the public |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Objective
This is an alternative to #19063 that uses the existing plan here.
The goal is to reduce the lookups on component maps, maps where
ComponentIdis the key. This will be really important when these maps move from a sparse set to a hash map to support components as entities.Solution
Store a new
ComponentRecordin the existingComponentIndexinstead of aArchetypeRecord. This data structure tracks the archetypes that have a given component. Now, they also track the table columns of that component. In a query fetch, use aHashMapto get thatComponentRecordand then index the table columns. (Instead of mapping on eachset_table.)Future work
We can do better than this. If we add some form of
WorldQuery::upgrade_statethat triggers when any archetype matches the query, then we can skip all component maps ininit_fetch.We should also look into caching the location of the
ComponentRecordinComponentInfo. Right now, to get an arbitrary component without a query, we have to map type id to component id, table id to get column id, and then look up the entity in the column. We could just map the type id to component id and component record id. Then we'd just be indexing twice instead of hash mapping once. Although,World::getlike this is uncommon, and a better solution may be to just force users to make a query which will be faster anyway unless they really only want one value ever. But that's blocked on #13358 (which is closed but the closing pr was later reverted IIRC). Bottom line, is we need assets as entities and I believe #18540 before we can safely turn&mut Worldthere to&Worldwithout loosing functionality.Testing
CiI
Performance
Benchmarks
Performance is not great.
The only place that is significantly faster than main is change detection, which is only sped up because the column is cached. In other words, the improvement could be trivially replicated without these changes.
In general, performance is slightly worse across the board, and much worse in a few places, especially sparse table queries and
Query::get. I want to drive home again that this is just adding an indexing op, that caches a sparse set lookup. I can only imagine what would happen if the sparse set of component ids became a hash map.