Use Component Column ids in tables to skip component maps in queries #19063
+364
−154
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
In preparation for components as entities, we will need to switch
ImmutableSparseSet<ComponentId, T>
toHashMap<ComponentId, T>
. This will be a pretty massive regression for those lookups. This PR aims to improve performance (both for components as entities and just for now) by skipping the hottest such component map:Table::columns
.Solution
I tried for longer than I'd like to admit to add per-table/storage caches to
QueryState
. (It kinda works, but justifying safety is tricky.)Then I had an idea, and this is something that I think extends to other component maps too! Right now we have an id into something dense (table) and then from there we map a component id to a column. But what if instead we started with an id into something sparse (new table component meta) and then from there we index a dense table id to a column? This is kinda like swapping the rows and columns of a abstract table. That's the general idea here.
We can extend this in a similar way to component maps in archetypes.
Note that we could take this a step further and physically move the columns from the
Table
to the newTablesComponentMeta
. That has some conveniences: A sparse set is just a one column table now (effectively), and we can save another indexing op when setting the table in a query. But it also has the big downside that moving between tables would need an extra indexing op for each component. IDK if that would be worth it, so I left it for now, instead creating column ids.Testing
CI
Future Work
Currently, creating a query fetch still needs the component map. If we expand
init_state
andget_state
's signatures to include all world metadata, we could put the index of the component metadata directly in the query state, skipping the map entirely.Also, right now it is still possible to get a component's data from a table directly (which does go through a component map). We should explore removing this extra data and sending everything though the new (faster) way. (And caching the column id where possible!)
Benches
Benchmarks show roughly even with main over all. Most benches fluctuate between ±5 or 6 % compared to main. Some benches take 2.x as long as main. Some things, like change detection see 50% improvements. In general, performance is 20%-50% improved over "normal" query iteration (due to 1 less indexing op) but is much worse over specific gets (ex:
Query::get
and friends) by 100%-100% (due to one more indexing op).Note that
world_query_get/50000_entities_table_wide
is up 500%, a major outlier. This is because it can not cache the column id, it actually doesn't even go through a query. So the extra indexing op is responsible for the 500% regression. If it were just a query get (which has some caching now) this would only be a 33% regression. (Which should be improved with more caching and would be way worse with a hashmap.)Interpreting these benches , I take away a few things: 1) There are significant performance wins to be had for caching table columns in more places, even without sparse component ids. 2) If adding one indexing op can cause 500% regression, imaging what a hashmap would do! Caching this is a must. 3) More specifically, we should look into expanding signatures of
Query::init_state
and friends to remove the current hashmap lookup ininit_fetch
.Benches differing by 20%