Separate component and resource access #14561

cBournhonesque · 2024-07-31T17:01:18Z

Objective

Fixes Incorrect handling of EntityRef/Mut Queries + Resources resulting in false conflicts #13139
Fixes Split access model between components and resources #7255
Separates component from resource access so that we can correctly handles edge cases like the issue above
Inspired from Split Resource and Component Access #14472

Solution

Update access to have component fields and resource fields

Testing

Added some unit tests

crates/bevy_ecs/src/query/access.rs

alice-i-cecile

This is my preferred approach. This is a straightforward and more correct model.

Code is good, tests are nice. There's a couple of docs that you missed updating but this is fundamentally an approval.

re0312 · 2024-08-01T15:03:45Z

crates/bevy_ecs/src/query/access.rs

+    /// The exclusively-accessed components.
+    component_writes: FixedBitSet,
+    /// All accessed resources.
+    resource_read_and_writes: FixedBitSet,


Why introduce extra FixedBitSets for resources here? IMO, we only need to split 'reads/writes_all' into 'read/write_all_component/resource' because Components and Resources share ID space

It's to check the has_any_resource that is needed to check compatibility

There's also a correctness hazard in sharing the bit sets for resources and components. There's a risk that the component Foo clashes with the resource Foo, for types which are both components and resources. This isn't the right memory model, and care must be taken far elsewhere in the stack (when assigning component ids) to avoid it if we don't separate it here.

A test to validate that behavior would be nice actually.

Not for this PR, but we should discuss whether to split the ID space. With this PR each bitset is going to be oversized and do extra work for the id's of the other type.

what I am most worried about is that Access is used in many places (such as EntityFilterRef), Perhaps we can move it to a new struct CombinedAccess and used it for' FilteredAccessSet::combined_access, with hhis way, we can limit the influence of this pr.

Not for this PR, but we should discuss whether to split the ID space. With this PR each bitset is going to be oversized and do extra work for the id's of the other type.

Note that this probably won't be a problem anymore if we go ahead with this
#14385
which removes the bitsets.
(which will probably be necessary since with relations the ComponentIds will become very high, so the memory used by FixedBitSet will become prohibitive)

what I am most worried about is that Access is used in many places (such as EntityFilterRef), Perhaps we can move it to a new struct CombinedAccess and used it for' FilteredAccessSet::combined_access, with hhis way, we can limit the influence of this pr.

Your concern is the increased memory usage because of 2 FixedBitSets?
Yes i think the solution would probably be #14385

Your concern is the increased memory usage because of 2 FixedBitSets?

yeah ,Two FixedBitSets (even not used) occupy extra 48 bytes, we only use it for combined access,
With #14385, this number is even Larger

A test to validate that behavior would be nice actually.

@alice-i-cecile added a test that checks that a resource/component that share the same componentId (same struct) do not conflict with each other

yeah ,Two FixedBitSets (even not used) occupy extra 48 bytes, we only use it for combined access,
With #14385, this number is even Larger

On the contrary with #14385 we don't have a problem anymore, no?
14385 uses a Vec<usize> for the access; so if there are no resource accessed we would just have an empty vec.

Or do you mean stack usage and not heap usage?

with #14385 It will make the problem more serious.

Access is just a struct, whether it occupies stack or heap depends on how you use it

Update Access and FilteredAccess to use sorted vecs instead of FixedBitSet #14385 uses SmallVec<[usize;N> (N default to 64),so it occupies 24+8 * 64 =536 bytes (* 2 = 1072 bytes ) even without any usage.

Olle-Lukowski

Good changes, and always good to add some test cases!

alice-i-cecile · 2024-08-02T13:50:23Z

I want to merge #14579 first, to ensure that we have more test cases in place.

alice-i-cecile · 2024-08-02T14:55:03Z

@re0312 while I share your nervousness about performance, the existing model is both wrong and confusing IMO. The test added in daf6981 won't pass using the access model on main. We should ensure correctness first, then evaluate techniques to improve memory usage and other performance-related elements.

re0312 · 2024-08-02T15:38:04Z

@re0312 while I share your nervousness about performance, the existing model is both wrong and confusing IMO. The test added in daf6981 won't pass using the access model on main. We should ensure correctness first, then evaluate techniques to improve memory usage and other performance-related elements.

yeah. totally agree with you , I don't intend to block this , I'm merely expressing my concern about it. Access is still used in many performance-sensitive places (such as dynamic query iteration, where an Access::clone is performed on each matched entity ).
but I don't object to left performance optimization to later pr.

# Objective Re-enable some tests in `entity_ref.rs` that are marked as `#[ignore]`, but that pass after #14561. ## Solution Remove `#[ignore]` from those tests.

# Objective Re-enable some tests in `entity_ref.rs` that are marked as `#[ignore]`, but that pass after bevyengine#14561. ## Solution Remove `#[ignore]` from those tests.

cBournhonesque added 2 commits July 31, 2024 12:12

separate component/resource access

ab3dfd0

fix resource vs component access

c59ffa1

cBournhonesque changed the title ~~Cb/resource access~~ Separate component and resource access Jul 31, 2024

cBournhonesque added A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jul 31, 2024

cBournhonesque mentioned this pull request Jul 31, 2024

Split Resource and Component Access #14472

Closed

cargo fmt

34bb10a

alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 31, 2024

alice-i-cecile added this to the 0.15 milestone Jul 31, 2024

alice-i-cecile reviewed Jul 31, 2024

View reviewed changes

crates/bevy_ecs/src/query/access.rs Outdated Show resolved Hide resolved

alice-i-cecile approved these changes Jul 31, 2024

View reviewed changes

cBournhonesque added 2 commits July 31, 2024 15:11

update docs

c63f893

update doc

8563cb3

re0312 reviewed Aug 1, 2024

View reviewed changes

add test for a resource that is also a component

daf6981

Olle-Lukowski approved these changes Aug 2, 2024

View reviewed changes

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 2, 2024

alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Aug 6, 2024

alice-i-cecile added this pull request to the merge queue Aug 6, 2024

Merged via the queue into bevyengine:main with commit 3a664b0 Aug 6, 2024
28 checks passed

Shatur mentioned this pull request Aug 6, 2024

Replace exclusive system for receiving with regular system in Bevy 0.15 projectharmonia/bevy_replicon#318

Closed

BorisBoutillier mentioned this pull request Oct 3, 2024

Query<EntityMut> system param unexpectedly conflicts with some Res<T> one #15621

Closed

chescock mentioned this pull request Nov 6, 2024

Enable EntityRef tests that now pass. #16263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate component and resource access #14561

Separate component and resource access #14561

cBournhonesque commented Jul 31, 2024 •

edited

Loading

alice-i-cecile left a comment

re0312 Aug 1, 2024

cBournhonesque Aug 1, 2024

alice-i-cecile Aug 1, 2024

hymm Aug 1, 2024

re0312 Aug 1, 2024

cBournhonesque Aug 1, 2024

re0312 Aug 1, 2024 •

edited

Loading

cBournhonesque Aug 1, 2024

cBournhonesque Aug 2, 2024

re0312 Aug 2, 2024 •

edited

Loading

Olle-Lukowski left a comment

alice-i-cecile commented Aug 2, 2024

alice-i-cecile commented Aug 2, 2024 •

edited

Loading

re0312 commented Aug 2, 2024

Separate component and resource access #14561

Separate component and resource access #14561

Conversation

cBournhonesque commented Jul 31, 2024 • edited Loading

Objective

Solution

Testing

alice-i-cecile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

re0312 Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

re0312 Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Olle-Lukowski left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Aug 2, 2024

alice-i-cecile commented Aug 2, 2024 • edited Loading

re0312 commented Aug 2, 2024

cBournhonesque commented Jul 31, 2024 •

edited

Loading

re0312 Aug 1, 2024 •

edited

Loading

re0312 Aug 2, 2024 •

edited

Loading

alice-i-cecile commented Aug 2, 2024 •

edited

Loading