Remove inbounds to _is_alive function

[ameligrana]

This function was unsafe before and could cause UB since it is called directly from the public is_alive

[github-actions]

⚠️ 20 benchmark regressions detected!

Click to expand benchmark results

Time is per entity/N, allocations are totals. Allocations are only shown for current.

N	Time main	Time curr	Factor	Allocs	Bytes
query_create
1000	2.77ns	2.78ns	1.00	0	0
query_create_filter
1000	2.77ns	2.76ns	1.00	0	0
query_posvel_1k_arch
100	6.80ns	6.81ns	1.00	0	0
1000	9.91ns	9.90ns	1.00	0	0
10000	2.45ns	2.48ns	1.01	0	0
100000	1.13ns	1.13ns	1.00	0	0
1000000	0.90ns	0.95ns	1.06	0	0
query_posvel_1k_arch_cached
100	6.57ns	6.44ns	0.98	0	0
1000	9.16ns	9.04ns	0.99	0	0
10000	2.40ns	2.49ns	1.04	0	0
100000	1.16ns	1.14ns	0.98	0	0
1000000	0.92ns	0.93ns	1.01	0	0
query_posvel_32_arch
100	2.16ns	2.16ns	1.00	0	0
1000	0.87ns	0.85ns	0.97	0	0
10000	0.44ns	0.44ns	1.01	0	0
100000	0.76ns	0.73ns	0.95	0	0
1000000	0.77ns	0.79ns	1.02	0	0
query_posvel_cold
100	0.81ns	0.74ns	0.93	0	0
1000	0.37ns	0.37ns	0.99	0	0
10000	0.38ns	0.40ns	1.07	0	0
100000	0.81ns	0.85ns	1.04	0	0
1000000	1.17ns	1.15ns	0.95	0	0
query_posvel_fields
100	0.37ns	0.37ns	1.00	0	0
1000	0.23ns	0.24ns	1.06	0	0
10000	0.33ns	0.35ns	1.04	0	0
100000	0.66ns	0.66ns	1.00	0	0
1000000	0.73ns	0.77ns	1.08	0	0
query_posvel_fields_broadcast
100	3.47ns	3.46ns	0.99	0	0
1000	3.35ns	3.35ns	1.00	0	0
10000	3.42ns	3.41ns	0.98	0	0
100000	3.46ns	3.46ns	1.00	0	0
1000000	3.76ns	3.89ns	1.10	0	0
query_posvel_hot
100	0.38ns	0.38ns	1.00	0	0
1000	0.23ns	0.23ns	1.00	0	0
10000	0.33ns	0.33ns	1.00	0	0
100000	0.66ns	0.66ns	1.00	0	0
1000000	0.74ns	0.77ns	1.06	0	0
query_posvel_soa
100	0.93ns	0.93ns	1.00	0	0
1000	0.75ns	0.75ns	1.00	0	0
10000	0.81ns	0.81ns	1.00	0	0
100000	0.92ns	0.92ns	1.00	0	0
1000000	0.92ns	0.98ns	1.06	0	0
query_posvel_soa_unpack
100	0.42ns	0.42ns	0.99	0	0
1000	0.22ns	0.22ns	1.00	0	0
10000	0.33ns	0.33ns	1.00	0	0
100000	0.60ns	0.60ns	1.00	0	0
1000000	0.69ns	0.73ns	1.07	0	0
world_add_remove_1
100	50.11ns	51.33ns	1.02	0	0
10000	50.79ns	52.17ns	1.03	0	0
world_add_remove_1_batch
100	6.10ns	6.10ns	1.00	0	0
10000	5.40ns	5.34ns	0.99	0	0
world_add_remove_1_large
100	55.26ns	56.22ns	1.02	0	0
10000	55.96ns	56.51ns	1.01	0	0
world_add_remove_1_soa
100	57.13ns	58.44ns	1.02	0	0
10000	58.19ns	59.42ns	1.02	0	0
world_add_remove_8
100	97.61ns	97.51ns	1.00	0	0
10000	100.75ns	100.04ns	0.99	0	0
world_add_remove_8_large
100	111.46ns	111.84ns	1.00	0	0
10000	114.14ns	114.20ns	1.00	0	0
world_add_remove_8_soa
100	124.59ns	125.82ns	1.01	0	0
10000	128.39ns	130.96ns	1.02	0	0
world_copy_entity_5
100	27.69ns	27.72ns	1.00	0	0
10000	27.41ns	27.47ns	1.00	0	0
world_get_1
100	1.03ns	1.21ns	⚠️ 1.17	0	0
10000	0.96ns	1.13ns	⚠️ 1.19	0	0
world_get_1_soa
100	1.58ns	1.79ns	⚠️ 1.14	0	0
10000	1.51ns	1.71ns	⚠️ 1.13	0	0
world_get_5
100	3.06ns	3.14ns	1.03	0	0
10000	3.00ns	3.09ns	1.03	0	0
world_get_rel
100	0.93ns	1.55ns	⚠️ 1.67	0	0
10000	0.84ns	1.48ns	⚠️ 1.76	0	0
world_new_entities_1
100	3.18ns	3.23ns	1.02	0	0
10000	2.36ns	2.37ns	1.00	0	0
world_new_entities_1_def
100	3.36ns	3.42ns	1.02	0	0
10000	2.44ns	2.49ns	1.02	0	0
world_new_entities_5
100	5.63ns	5.56ns	0.99	0	0
10000	5.02ns	5.02ns	1.00	0	0
world_new_entities_5_def
100	5.58ns	5.61ns	1.00	0	0
10000	4.68ns	4.62ns	0.99	0	0
world_new_entity_1
100	14.63ns	14.75ns	1.01	0	0
10000	14.52ns	14.58ns	1.00	0	0
world_new_entity_1_rel
100	36.51ns	36.21ns	0.99	0	0
10000	36.31ns	35.85ns	0.99	0	0
world_new_entity_1_soa
100	16.88ns	17.08ns	1.01	0	0
10000	16.80ns	16.97ns	1.01	0	0
world_new_entity_5
100	25.97ns	25.93ns	1.00	0	0
10000	27.23ns	26.67ns	0.98	0	0
world_new_entity_5_rel
100	54.15ns	53.31ns	0.98	0	0
10000	56.25ns	54.66ns	0.97	0	0
world_new_entity_5_soa
100	36.25ns	39.64ns	1.09	0	0
10000	37.11ns	40.50ns	1.09	0	0
world_posvel
100	2.71ns	3.39ns	⚠️ 1.25	0	0
1000	2.74ns	3.44ns	⚠️ 1.25	0	0
10000	2.80ns	3.41ns	⚠️ 1.22	0	0
100000	2.80ns	3.43ns	⚠️ 1.22	0	0
world_remove_entities_5
100	2.09ns	2.11ns	1.01	0	0
10000	1.00ns	0.98ns	0.98	0	0
world_remove_entity_5
100	22.21ns	22.82ns	1.03	0	0
10000	21.98ns	22.36ns	1.02	0	0
world_resource
1	7.08ns	7.04ns	1.00	0	0
world_set_1
100	2.45ns	3.23ns	⚠️ 1.27	0	0
10000	2.53ns	3.30ns	⚠️ 1.27	0	0
world_set_1_soa
100	2.48ns	3.23ns	⚠️ 1.28	0	0
10000	2.52ns	3.33ns	⚠️ 1.30	0	0
world_set_5
100	6.98ns	8.09ns	⚠️ 1.15	0	0
10000	7.62ns	8.85ns	⚠️ 1.16	0	0
world_set_rel
100	45.14ns	45.21ns	1.00	0	0
10000	40.81ns	41.60ns	1.02	0	0
world_set_rel_batch
100	3.59ns	3.66ns	1.02	0	0
10000	2.09ns	2.09ns	1.00	0	0
world_update_1
100	2.22ns	3.45ns	⚠️ 1.51	0	0
10000	2.28ns	3.57ns	⚠️ 1.53	0	0
world_update_5
100	9.74ns	10.79ns	⚠️ 1.11	0	0
10000	10.16ns	11.22ns	⚠️ 1.10	0	0

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[ameligrana]

the effect on performance is kind of bad, though I don't think we have better alternatives

[ameligrana]

By the way, this avoids UB in two cases as far as I can see:

• asking if entities collected in the past are alive after a reset!
• asking if entities collected in a different world are alive

Maybe the second case is being too paranoid, if we want to fix only the first one I think less impactful changes are possible

[mlange-42]

I think both are not really valid use cases, as the function is not guarenteed to return the correct result. Both cases may produce entities that are not really valid, but are seen as alive due to having the "correct" generation.

So maybe it would be sufficient to just enable bounds checks, but not catch that case explicitly?

[ameligrana]

Yes this is sensible to me...Though enabling bound checking has the same perf degradation than this more or less

[mlange-42]

Ok, maybe another strategy: we can probably reset the pool without resizing. Will make reset more costly, but we can avoid bounds checks.

[ameligrana]

yes, though this will work just for the first case, not the second one, though to me it is okay that the second one remains unsafe since it seems really unimportant

[mlange-42]

Maybe we should really recycle/"kill" all entities. For a reset world, we would then get the true alive state.