Resource storage

[SpecificProtagonist]

Objective

Resources as components tracking issue: #19731

#20934 changed resources to be components stored in sparse set storage.

This means accessing a resource looks up TypeId → ComponentId, ComponentId → Entiy & SparseSet, Entity → TableRow and TableRow → component data. This seems to have caused a performance regression: #23039

Solution

Keep resources as components, but remove ResourceEntities in favor of a dedicated storage type for resource components. Looking up a resource now looks more like TypeId → ComponentId, ComponentId → component data.

Micro-benchmarks vs main (9bd7e1):

ecs::resources::get     time:   [3.6000 ns 3.6062 ns 3.6133 ns]
                        change: [−47.292% −46.931% −46.547%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 14 outliers among 100 measurements (14.00%)
  12 (12.00%) high mild
  2 (2.00%) high severe

Benchmarking ecs::resources::get_mut: Collecting 100 samples in estimated 5.0000 s (1.1B iteratecs::resources::get_mut time:   [4.3944 ns 4.3993 ns 4.4049 ns]
                        change: [−43.998% −43.560% −43.172%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high severe

Benchmarking ecs::resources::insert_remove: Collecting 100 samples in estimated 5.0001 s (54M iecs::resources::insert_remove
                        time:   [91.680 ns 91.762 ns 91.882 ns]
                        change: [−6.4805% −3.0014% +0.4886%] (p = 0.06 > 0.05)
                        No change in performance detected.

Running systems with Res/ResMut system params should also be faster.

Related work

#23988 makes components themselves entities. Besides letting you apply ecs tools like relationships to components, it would also remove ResourceEntities.

This would result in part of the speedup seen here. Whether having a dedicated resource storage is worth it can should be reconsidered when components-as-entities is in.

Testing

Testing of correctness is pretty well covered by the existing tests that supported entities as resources.

I'd like someone to validate the performance improvements though. I also haven't measured if this impacts build time, binary size or memory usage, but I don't expect significant changes there.

[SpecificProtagonist]

A 1-element column is rather silly, I know. I'll inline it if requested.

[alice-i-cecile]

Unless there's performance gains here I think this is simpler.

[SpecificProtagonist]

I haven't put too much consideration into what map data structure is best here yet.

[chescock]

I haven't put too much consideration into what map data structure is best here yet.

I would expect a SparseSet like this to be pretty efficient, and it's similar to what we had before resources-as-entities.

[chescock]

Awesome!

[chescock]

I think ignoring the inserted values like this will be unsound. The hook will move the archetype back soon after, but other hooks and observers can briefly see a world where the duplicate resource entity is in an archetype with the resource component but does not have its own value.

That is, it's possible to query for &ResourceValue in a hook, and the get_with_entity(entity).debug_checked_unwrap() lines will be UB on the duplicate entities.

Unless we have a way to prevent the initial archetype move, I think we might need some auxiliary storage somewhere to hold the duplicate values temporarily. :(

Maybe we could store the values in the Column, and have something like duplicates: Vec<Entity> that we have to check in queries? That wouldn't affect resource-specific APIs like Res since they'd never be the real resource values, and it wouldn't allocate anything when there aren't duplicates. I think the main cost would be some extra codegen in queries.

[alice-i-cecile]

Fundamentally, I think we need to move this check to occur earlier. Preventing the inital archetype move is the cleanest and most robust way to do this.

I suspect that this is possible: we just need to scan ResourceStorage for pre-existing resources before archetype moves. It looks like ResourceStorage::insert, BundleInfo::write_components and BundleInserter::insert / spawn_at are the methods we would need to touch.

[SpecificProtagonist]

Fundamentally, I think we need to move this check to occur earlier. Preventing the inital archetype move is the cleanest and most robust way to do this.

Currently trying this out. Looks like it should be possible to do so soundly and without perf penality, I just need to be very careful and quadruple check everything (and be very clear in the safety comments).

[SpecificProtagonist]

Not entirely sure which of the two is the best solution.

I've implemented that invalid writes don't happen in the first place. Fails Stacked Borrows, but valid under Tree Borrows. I'll need to do more investigation to figure out the exact cause and find a workaround.

[alice-i-cecile]

Please add yourself and this PR to the resources-as-components release notes. This is a critical part of the story.

[alice-i-cecile]

I really like the core idea, but we need to a) fix the insert_just_failed messiness b) rerun benchmarks comparing to #24077 c) do a sanity check on many_foxes or something.

[alice-i-cecile]

Cutting from the milestone: the perf regression is already mostly addressed, and we're still not sure that we want this on top of resources-as-components.

I still want this, but it's not worth blocking the release on.

[Shatur]

If we introduce it later, it will be a big breaking change since users can start using it for different entities. I think it's quite a useful feature, see #24058 (comment). Unless we add a separate support for singletone components.

[alice-i-cecile]

The ability to insert a resource on any entity is a useful feature. For example, I could have ActiveCharacter that is automatically unique: once I insert it on a new character entity, it's removed from the old one.

This functionality will still work after this PR: it's just changing "how is the data on an entity stored", not "which entities can resources be associated with". Insertion, querying and so on will all work just as before: this is purely an internals change. We can and probably should add a test for this pattern however.

Having a special case for resources will also make networking harder because I'll have to create a special case for resources.

This was already the case in Bevy 0.1-0.18. I also don't think that this will make things dramatically harder for networking. Resources are special, and warrant special treatment.

[Shatur]

it's just changing "how is the data on an entity stored"

Ah, so users will be able to insert resources on any entities, they will just be stored differently internally? Then I just misunderstood the description (I haven't looked into the code yet, sorry).

This was already the case in Bevy 0.1-0.18.

That is true, and both Replicon and Lightyear don't have resource replication right now because we were waiting for components-as-resources to land. With the current main, resource replication works just like component replication, with no special handling required (here is the branch for Replicon).

However, if this PR only affects the storage, almost no special logic will be needed even after the PR. I'll just iterate over components as usual, get the storage, and obtain the pointer. I'll only have to add one more case for Resource storage in addition to the already existing SparseSet and Table. So it's all good! Thanks for the clarification.

[SpecificProtagonist]

The ability to insert a resource on any entity is a useful feature. For example, I could have ActiveCharacter that is automatically unique: once I insert it on a new character entity, it's removed from the old one.

To be clear: This isn't how it works on main, the insertion gets undone. We could make it work that way, but it would be in conflict with the plan for components-as-entities: There a resource is attached to itself.

[Shatur]

You're right, my bad:

bevy/crates/bevy_ecs/src/resource.rs

Lines 144 to 173 in 0376e71

	if let Some(original_entity) = world.resource_entities.get(resource_component_id) {
	if !world.entities().contains(original_entity) {
	let name = world
	.components()
	.get_name(resource_component_id)
	.expect("resource is registered");
	panic!(
	"Resource entity {} of {} has been despawned, when it's not supposed to be.",
	original_entity, name
	);
	}
	if original_entity != context.entity {
	// the resource already exists and the new one should be removed
	world
	.commands()
	.entity(context.entity)
	.remove_by_id(resource_component_id);
	world
	.commands()
	.entity(context.entity)
	.remove_by_id(context.component_id);
	let name = world
	.components()
	.get_name(resource_component_id)
	.expect("resource is registered");
	warn!("Tried inserting the resource {} while one already exists.
	Resources are unique components stored on a single entity.
	Inserting on a different entity, when one already exists, causes the new value to be removed.", name);
	}

I didn't have much time to play with main in an actual project and for some reason I assumed it works this way. Thanks for the clarification and sorry for the fuss!