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.

[urben1680]

I must say I am actually curious for ComponentInfo being a component on the respective ComponentId entity.

Generally, I am a bit unhappy how many places now need to do assertions on IsResource being a required component. I would have hoped this could be more centralized within the component registration.

This being a change request could be resolved by understanding things better from answers to my questions.

I want to note I know too little of assets to review the world_asset file btw.

[Victoronz]

Overall, I do like and want Components as Entities to go through, but I think we are being too lenient with the UB here.

The access problems we run into when moving ECS architecture into the ECS are not trivial and have been a long-standing friction point. I do not think they should be left for a follow-up, and instead be done with this PR, even if it then increases in size.

In reply to @chescock (I can't respond in-thread for some reason):

do we rely on required components for soundness anywhere already? Under my impression (and this may very well be wrong, I don't have overview there), this infrastructure was in the space of "if something goes wrong, that is a logic error".

If so, we should be careful to make sure that the required component space aligns with being designed for soundness, otherwise we may run into other design complications over there.

No, this is definitely stretching what required components are intended for! Normally they can't be relied on for soundness because you can always remove them later. Which is why we need to prevent that by panicking in the on_discard hook. Which is also stretching what hooks are intended for :).

What we really want here is archetype invariants, so hopefully we can do something cleaner once we have those.

I think we're hitting a pain point here:
Judging from what the maintainers/SMEs have stated before, we want to be careful of how we impact other work that is usually in separate areas, and this has a chance of introducing friction in such other areas, hooks and required components.

In my experience with Rust and Bevy, a solution that is unsound if it fails, and one where failure would mean a logic error diverge in some design aspects, and trying to combine the two will often result in friction.
Given that hooks and required components seem not designed for this, I think we need due diligence and/with maintainer/SME sign-off here, because we cannot easily undo dependencies used for soundness.

It seems to me that the underlying problem might need greater architectural changes to address properly. As of right now, the reliance on hooks and required components seems to me like it still circumvents the issue, as I hit on here.

I gather, @Trashtalk217, that you'd rather focus on the code, and less these overarching design interactions/questions. But I do think they need to come up and be discussed when they are as core as this; regardless which way is decided later. That is what I'd think most aligned with the maintainer/SME opinion process-wise.
Such discussion does not need to fully live on the PR! However, they should at least see proper mention, as these PRs are the history people look back on. This way, it'll be easier to handle fallout were anything to go wrong down the line — hopefully without blame falling on specific people!

More actionably, the questions I'd like to see maintainer/SME judgment on:
Can we rely on hooks and required components for soundness?
Does this solve the UB/these access issues in a manner we won't want to undo later? (Both for this PR and for X-As-Entities in general)
Process-wise, how do we want to do due diligence without holding up this specific work for too long?

Keep in mind that "We don't see the need to address/discuss this at this point" are valid opinions/judgments!

[alice-i-cecile]

I've taken a look at this and I agree with @Victoronz.

Can we rely on hooks and required components for soundness?

Hooks, if we have no other choice. Required components, in their current form, no. Without an airtight implementation of archetype invariants I am not comfortable relying on this sort of strategy to avoid UB, and "just panic" is the worst sort of UB prevention.

[SkiFire13]

From my unaddressed comment in the linked PR:

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.

On the other hand note that you have introduced a HashMap lookup into every ComponentId -> ComponentInfo path! This happens for example when calling hooks, which is arguably pretty common too.

Sidenote: do we have a benchmark for hooks to measure this?

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.

I'm not sure how this PR solves the issue. You'll still have the issue that the EntityId in the new world needs to match the ComponentId of the resource.

[Trashtalk217]

As far as I know, we don't have benchmarks to measure hook access. The particular performance problem you describe can be fixed through #24102.

With regards to the bevy_world_serialization problem, this is where it gets clever. Mapping entities between worlds has already been solved through SceneEntityMapper. The idea is that we can use this just as well for ComponentIds when ComponentIds are just entities.

[SkiFire13]

Note that EntityEquivalentHashSet does not have a well defined order, it will be deterministic but unpredictable. You can see this e.g. in the access tests where you had to update some of them due to the order changing.

[Trashtalk217]

I'm aware.

[SkiFire13]

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

I can see some rather big regressions here:

group                                                                                                     after                                    before
-----                                                                                                     -----                                    ------
added_archetypes/archetype_count/10000                                                                    2.99     36.5±1.69ms        ? ?/sec      1.00     12.2±0.16ms        ? ?/sec
all_added_detection/50000_entities_ecs::change_detection::Sparse                                          1.72     84.7±0.81µs        ? ?/sec      1.00     49.4±2.98µs        ? ?/sec
all_added_detection/5000_entities_ecs::change_detection::Sparse                                           1.71      8.5±0.52µs        ? ?/sec      1.00      4.9±0.44µs        ? ?/sec
despawn_world_recursive/10000_entities                                                                    1.12  1547.4±24.08µs        ? ?/sec      1.00   1375.8±8.73µs        ? ?/sec
despawn_world_recursive/100_entities                                                                      1.14     16.4±1.42µs        ? ?/sec      1.00     14.4±1.31µs        ? ?/sec
ecs::resources::get                                                                                       1.30      8.1±0.45ns        ? ?/sec      1.00      6.2±0.52ns        ? ?/sec
ecs::resources::get_mut                                                                                   1.23      9.0±0.47ns        ? ?/sec      1.00      7.4±0.60ns        ? ?/sec
ecs::world::world_get::world_query_get_components_mut/unchecked_5_components_50000_entities               2.02  1547.9±74.62µs        ? ?/sec      1.00   766.5±38.31µs        ? ?/sec
empty_archetypes/for_each/1000                                                                            2.46     18.4±1.52µs        ? ?/sec      1.00      7.5±1.00µs        ? ?/sec
empty_archetypes/for_each/10000                                                                           25.88  276.3±58.63µs        ? ?/sec      1.00     10.7±0.97µs        ? ?/sec
empty_archetypes/iter/100                                                                                 1.18      8.4±1.02µs        ? ?/sec      1.00      7.1±0.51µs        ? ?/sec
empty_archetypes/iter/1000                                                                                2.58     19.1±2.33µs        ? ?/sec      1.00      7.4±0.84µs        ? ?/sec
empty_archetypes/iter/10000                                                                               27.51  279.2±54.97µs        ? ?/sec      1.00     10.1±2.66µs        ? ?/sec
empty_archetypes/par_for_each/10                                                                          1.15     10.1±1.13µs        ? ?/sec      1.00      8.7±0.65µs        ? ?/sec
empty_archetypes/par_for_each/100                                                                         3.70     32.4±3.23µs        ? ?/sec      1.00      8.8±1.06µs        ? ?/sec
empty_archetypes/par_for_each/1000                                                                        33.88  329.2±23.02µs        ? ?/sec      1.00      9.7±0.85µs        ? ?/sec
empty_archetypes/par_for_each/10000                                                                       191.38     3.3±0.06ms        ? ?/sec     1.00     17.3±2.24µs        ? ?/sec
empty_systems/0_systems                                                                                   1.14     16.1±4.09ns        ? ?/sec      1.00     14.2±1.49ns        ? ?/sec
events_iter/size_16_events_1000                                                                           1.20  776.3±115.84ns        ? ?/sec      1.00   647.5±49.46ns        ? ?/sec
events_iter/size_16_events_10000                                                                          1.32      8.5±0.57µs        ? ?/sec      1.00      6.4±0.53µs        ? ?/sec
events_iter/size_4_events_100                                                                             1.32     85.9±5.74ns        ? ?/sec      1.00     64.9±5.00ns        ? ?/sec
events_iter/size_4_events_1000                                                                            1.27   826.7±94.24ns        ? ?/sec      1.00   648.9±66.35ns        ? ?/sec
multiple_archetypes_none_changed_detection/100_archetypes_10000_entities_ecs::change_detection::Table     1.33    727.2±4.41µs        ? ?/sec      1.00    546.0±5.28µs        ? ?/sec
multiple_archetypes_none_changed_detection/100_archetypes_1000_entities_ecs::change_detection::Sparse     2.76    215.7±5.87µs        ? ?/sec      1.00     78.1±2.80µs        ? ?/sec
multiple_archetypemultiple_archetypes_none_changed_detection/5_archetypes_1000_entities_ecs::change_detection::Table        1.48      3.4±0.05µs        ? ?/sec      1.00      2.3±0.25µs        ? ?/sec
s_none_changed_detection/100_archetypes_1000_entities_ecs::change_detection::Table      2.11    100.3±2.18µs        ? ?/sec      1.00     47.5±0.95µs        ? ?/sec
multiple_archetypes_none_changed_detection/100_archetypes_100_entities_ecs::change_detection::Table       1.53      9.2±0.37µs        ? ?/sec      1.00      6.0±0.24µs        ? ?/sec
multiple_archetypes_none_changed_detection/100_archetypes_10_entities_ecs::change_detection::Sparse       1.24  1420.8±139.83ns        ? ?/sec     1.00  1147.1±161.44ns        ? ?/sec
multiple_archetypes_none_changed_detection/100_archetypes_10_entities_ecs::change_detection::Table        1.39  1212.4±121.47ns        ? ?/sec     1.00   869.3±85.26ns        ? ?/sec
multiple_archetypes_none_changed_detection/20_archetypes_10000_entities_ecs::change_detection::Sparse     1.22   325.4±22.68µs        ? ?/sec      1.00   266.6±11.61µs        ? ?/sec
multiple_archetypes_none_changed_detection/20_archetypes_10000_entities_ecs::change_detection::Table      1.49    140.3±3.66µs        ? ?/sec      1.00     94.2±0.89µs        ? ?/sec
multiple_archetypes_none_changed_detection/20_archetypes_1000_entities_ecs::change_detection::Table       1.52     13.9±0.24µs        ? ?/sec      1.00      9.1±0.17µs        ? ?/sec
multiple_archetypes_none_changed_detection/20_archetypes_100_entities_ecs::change_detection::Table        1.52  1526.8±146.79ns        ? ?/sec     1.00  1007.4±15.22ns        ? ?/sec

Some are pretty niche, but I find interesting that ecs::resources::get and ecs::resources::get_mut are there given that this PR claimed to speed up resource lookup.

Edit: or are after/before perhaps switched?

[Trashtalk217]

I don't think after/before were switched. As per my comment, while the micro benchmarks show some regressions, performance is not the only reason for this PR. Furthermore, the stress tests (which are more representative for real world usecases) don't show a significant decrease in performance.

Further furthermore: A fix for any performance issues already exists in #24102, but that's something best left for a separate PR.

[Trashtalk217]

It's also possible that my laptop (bless their soul) is not super reliable for benchmarks, but it tried its best.

[SkiFire13]

Given that now ComponentIds use EntityIds, the highest ComponentId might become proportional to the number of entities rather than components, which might be higher than usual. Have you tried looking into how much this grows in practice? This might be important especially because Tables use SparseSets keyed by ComponentIds, so it's important that these don't grow too much.

[Trashtalk217]

Good catch! Sadly, there's not really any good EntityEquivalent data structure for ImmutableSparseSet and adding it, in my opinion, is out of scope for now. This is definitely good future work!