Components as entities (v2)

[Trashtalk217]

See #23988.

Objective

There are two primary problems we currently have with resources-as-components (#19731) .

Performance regression #23039

In order to get access to a resource entity, you usually need to go through two lookups: TypeId -> ComponentId -> Entity. The ComponentId -> Entity lookup can potentially be removed, speeding up resource lookup.

World Asset Serialization (previously in bevy_scene) #22968

Currently, one big downside of resources-as-components is that any components on resource entities aren't serialized. This forms a barrier for implementing required components for resources. Implementing this correctly is tricky. One of the problems we run into is that IsResource(ComponentId) cannot be directly copied over because ComponentIds are not consistent between worlds.

Solution

We change ComponentId(usize) into ComponentId(Entity) and use the EntityAllocator to create ComponentIds. Next, we store all resources on the ComponentId entity. This makes the ComponentId -> Entity lookup a no-op. And we can also use the MapEntities machinery to serialize and deserialize worlds.

Future Work

In the future, we can look at adding parts of ComponentInfo as a component to the component entities. That sounds confusing, because it is. This would improve the introspection ability of the ECS (being able to query for metadata about the components). This, however, requires quite a bit of weird bootstrapping, which I don't know how to do and is not necessary for this PR.

Testing

This is principally a performance PR, and while I will be doing some benchmarking myself, I don't have the hardware. So I'll be asking someone to give it a once-over when it's ready.

Change

The primary difference between this and #23988, is that this is not reliant on Entity Ranges (#24102). Because of this, there is a possibility for a performance regression with regards to Components.components going from a SparseArray to HashMap. There is an additional hash operation. Additionally, the fields on Access have changed from a FixedBitSet to a HashSet, so the set operations are likely slower.

[Trashtalk217]

Performance Testing

I did some reasonably extensive performance testing. I ran both the entire ecs benchmark and several of our stress test examples. I've got the following data:

• cargo bench -p benches --bench ecs -> benchmarks.txt

And the three examples I ran through were

• cargo run --release --example bevymark -- --waves 60 --per-wave 500 --benchmark --mode mesh2d
• cargo run --release --example many_components 100000 1000 1000
• running bevy_city with mangohud.

name	frametime before (main)	frametime after (pr)
bevymark	37.5 ms	37.8 ms
many_components	6.6 ms	6.5 ms
bevy_city	90.5 ms	90.5 ms

Conclusion

From the measurements I took I've come to the conclusion that this PR mostly keeps performance the same. At worst it degrades slightly (mostly based on the microbenchmarks). The best solution to fix the resource performance issue is likely still going to be #24058 or something like it.

However!

I still think this PR is worth merging, because

1. It's a step to storing a meta-component on the component entity, allowing for more introspection in the ecs.
2. Users can tag component entities with their own data ComponentInfo extensions #24338.
3. It removes an indirection in the code via ResourceEntities and with it some unsafe code.
4. The (in my opinion minor) loss of performance can in the future be offset by introducing entity ranges (Entity ranges #24102) and hybrid datastructures. For more information checkout the entity ranges PR (or ask!).
5. It makes working with resources more consistent. Before, world.spawn((Res1, Res2)) would store Res2 on the resource entity of Res1, so Res2 would be stored on the wrong location. In this PR, this would throw a warning and remove both Res1 and Res2 because neither are on the appropriate entity.

Important Note

When review and discussing this PR, I prefer if you mostly kept it anchored to some line of code (even if only vaguely related). This makes it easier to follow particular threads of argument.

[Victoronz]

Looks good! I don't believe the #24102 is necessary for this, and can be left for later.

With this change, it becomes possible to express uniqueness regarding ComponentId collections/iterators as well. There are now likely several sections of code where it would make sense to convert the previous ComponentId collection/iterator types to their EntitySet version. That would make for a good follow-up PR!

One place for which this is especially relevant is DynamicComponentFetch, the current dynamic way to access the Components of an entity disjointly.
This functionality should fall under similar design considerations as #18234, though that PR has gotten stalled.

[SkiFire13]

This still uses a SparseSet over ComponentIds.

Note however that this is accessed once per table when iterating queries (that is, NOT when the QueryState/Query is constructed, but rather every time it's iterated, and multiple times per iteration), so changing it to a EntityHashMap might regress performance for query iteration.

[Trashtalk217]

I want to place a brief note with regards to this commit: fb25d44

Here I formally replace a couple of ImmutableSparseArray<ComponentId, ...>s. I did this because sparse arrays take up space relative to the highest id stored, instead of the number of ids. Since ComponentIds can get pretty high due to this PR, this is potentially a memory issue.

I've replaced them with ComponentIdMaps (basically HashMaps). This solves the space issue, but takes more time. As per @SkiFire13, the following operations are likely slower: Query iteration, hook access, and archetype creation.

To figure out how much slower (if at all), I (or someone with a better PC) will have to rerun the benchmarks.

EDIT: To re-iterate, this is a solvable problem. Namely, #24102 could make it so the first N components are dense and from that point onwards it's some pretty basic data structure work.

[ItsDoot]

Couldn't this be &'w EntityAllocator since EntityAllocator::alloc takes &self?

[ItsDoot]

Should be able to downgrade &'w mut EntityAllocator to a shared reference in ComponentsRegistrator as well.

[SkiFire13]

This is still a couple of type still using SparseSet<ComponentId>:

• RemovedComponentMessages
• NonSends
• SparseSets

This could however be fine since these are all "one-instance" types, so we would not end up paying the cost for every table or archetype.

[ItsDoot]

We should be able to remove this field completely, because all it stores is the StorageType for the given ComponentId, which we can fetch from Components as needed.

[ItsDoot]

Box<[ComponentId]>, the component list is static for a given archetype.

[chescock]

This looks like it's reverting the gains from #23151. I think you might need a version of ComponentIdMap based on sorted boxed slices so that we can do a merge without needing to hash anything.

[SkiFire13]

I think you might need a version of ComponentIdMap based on sorted boxed slices

This would be trivial with an IndexMap-based ComponentIdMap

[chescock]

This would be trivial with an IndexMap-based ComponentIdMap

Yeah, but you'd still need to make sure the components are sorted... and at that point the HashMap part isn't buying you much because you can use binary search for lookups.

I don't know of a good off-the-shelf implementation of a map backed by sorted slices, though.

Or, I guess you could just use a BTreeMap, but we aren't trying to modify it after creation so the extra complexity over a sorted slice doesn't seem worthwhile.

[SkiFire13]

but you'd still need to make sure the components are sorted...

That's the same as what we have been doing with boxed slices, just with an additional hashmap backing lookups though.

and at that point the HashMap part isn't buying you much because you can use binary search for lookups.

I'm not sure that binary search has the same performance characteristics as a hash lookup.

[chescock]

That's the same as what we have been doing with boxed slices, just with an additional hashmap backing lookups though.

Yup, exactly. I'm just arguing that the hashmap isn't worth the overhead at that point, and we could drop down to just two boxed slices. We'd have to profile, of course!

I'm not sure that binary search has the same performance characteristics as a hash lookup.

Yeah, it has slightly worse big-O, but log(N) isn't bad even for large N, and it uses less memory and has a better constant factor. For that matter, a lot of archetypes will be small enough that a linear scan might even be fastest!

[chescock]

Sorry, let me rephrase that: Yup, IndexMap with sorted components is a good idea and we should try it! And then, once that's working, I have some crazy ideas to micro optimize it further that we could test!

[SkiFire13]

Yeah, it has slightly worse big-O, but log(N) isn't bad even for large N, and it uses less memory and has a better constant factor.

I'm not so sure about the constant factor, as it definitely depends on the cost of the hash function you're using. Binary search also has terrible branch prediction (it's the worst case for it!)

For that matter, a lot of archetypes will be small enough that a linear scan might even be fastest!

Yes, for small arrays linear scans might be better than both binary search and hash tables. In fact rustc even a small map optimization where it uses an arrayvec for small maps and a hashmap for bigger ones, with the threshold being 8 (but it could be tweaked).