Components as entities (v2)#24728
Conversation
…ents-as-entities-alt
Performance TestingI 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:
And the three examples I ran through were
ConclusionFrom 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
Important NoteWhen 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. |
There was a problem hiding this comment.
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.
chescock
left a comment
There was a problem hiding this comment.
Looks good!
I'm fairly certain this is sufficiently1 sound. The checks in Res ensure that IsResource is a required component, and the panic in IsResource::on_discard ensures it can never be removed, so Res can only access entities with IsResource, matching the access that it claims.
This is a bit of an abuse of both required components and hooks, of course :). In particular, the panic in on_discard can mean that other on_discard hooks ran, but then the discard didn't happen. But user hooks could already panic there, so that's not a new problem. And hopefully this can become more principled when we have proper archetype invariants.
Left a few minor style nits I noticed on another read. The apply_settings_to_world one might be a bug, but none of the others should be blocking.
Footnotes
-
I think there may be a soundness hole if you insert
IsResourcewith the wrong ID so that the required component doesn't insert one with the right ID. But the badIsResourcewill immediately be removed by a command from itson_inserthook, so you would need to do the insert in anotheron_inserthook that runs its commands first. And that hole goes away completely if we switchIsResourceto a ZST, which we probably want to do anyway as a follow-up. ↩
| let d1 = query.get(&world, e1).unwrap(); | ||
| // NameOrEntity Display for entities without a Name should be {index}v{generation} | ||
| assert_eq!(d1.to_string(), "1v0"); | ||
| assert_eq!(d1.to_string(), "9v0"); |
There was a problem hiding this comment.
The 9 here seems brittle.
| assert_eq!(d1.to_string(), "9v0"); | |
| assert_eq!(d1.to_string(), e1.to_string()); |
There was a problem hiding this comment.
I actually really like this test as it is, because it's a great marker whenever the default number of entities increases. This is the one test I kind of like to be brittle.
| pub unsafe fn get_resource_by_id(self, component_id: ComponentId) -> Option<Ptr<'w>> { | ||
| // SAFETY: We have permission to access the resource of `component_id`. | ||
| let entity = unsafe { self.resource_entities() }.get(component_id)?; | ||
| let entity = component_id.entity(); |
There was a problem hiding this comment.
My mistake @chescock, I didn't understand how this change could result in UB.
No worries! My fault for not explaining it well. Sometimes I'm way too verbose, so I try to avoid that but then wind up editing out the important parts :).
Also, if you can, mark any solved conversation as 'resolved' so I know your concerns have been addressed.
I think only you can do that? It always seems backwards to me, but I think that's how GitHub works. I can add thumbs-up reactions to your responses, though! And I can go approve the PR :).
EDIT: Ended up doing something slightly different, it should still work.
Ah, I see, you're checking for the presence of the required component. That's a clever approach! It only costs a hash lookup at startup per Res, and we're already paying one of those to map T to component_id, so I wouldn't expect it to be a significant perf cost.
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.
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
There was a problem hiding this comment.
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.
|
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):
I think we're hitting a pain point here: 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. 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. More actionably, the questions I'd like to see maintainer/SME judgment on: Keep in mind that "We don't see the need to address/discuss this at this point" are valid opinions/judgments! |
|
I've taken a look at this and I agree with @Victoronz.
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. |
| @@ -0,0 +1,36 @@ | |||
| --- | |||
There was a problem hiding this comment.
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
HashMaplookup into everyComponentId -> ComponentInfopath! 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
EntityIdin the new world needs to match theComponentIdof the resource.
There was a problem hiding this comment.
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.
| pub type ComponentIdMap<V> = EntityEquivalentHashMap<ComponentId, V>; | ||
|
|
||
| /// A set of [`ComponentId`]s. | ||
| pub type ComponentIdSet = EntityEquivalentHashSet<ComponentId>; |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,36 @@ | |||
| --- | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's also possible that my laptop (bless their soul) is not super reliable for benchmarks, but it tried its best.
| pub(super) ids: &'w mut ComponentIds, | ||
| pub(super) allocator: &'w mut EntityAllocator, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
…ents-as-entities-alt
897d46e to
a7a2972
Compare
See #23988.
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.componentsgoing from aSparseArraytoHashMap. There is an additional hash operation. Additionally, the fields onAccesshave changed from aFixedBitSetto aHashSet, so the set operations are likely slower.