[Merged by Bors] - drop overwritten component data on double insert #2227

Frizi · 2021-05-21T00:28:21Z

Continuing the work on reducing the safety footguns in the code, I've removed one extra UnsafeCell in favour of safe Cell usage inisde ComponentTicks. That change led to discovery of misbehaving component insert logic, where data wasn't properly dropped when overwritten. Apart from that being fixed, some method names were changed to better convey the "initialize new allocation" and "replace existing allocation" semantic.

Depends on #2221, I will rebase this PR after the dependency is merged. For now, review just the last commit.

Frizi · 2021-05-21T00:41:01Z

crates/bevy_ecs/src/lib.rs

+    #[test]
+    fn insert_overwrite_drop() {
+        let (dropck1, dropped1) = DropCk::new_pair();
+        let (dropck2, dropped2) = DropCk::new_pair();
+        let mut world = World::default();
+        world.spawn().insert(dropck1).insert(dropck2);
+        assert_eq!(dropped1.load(Ordering::Relaxed), 1);
+        assert_eq!(dropped2.load(Ordering::Relaxed), 0);
+        drop(world);
+        assert_eq!(dropped1.load(Ordering::Relaxed), 1);
+        assert_eq!(dropped2.load(Ordering::Relaxed), 1);
+    }


Here is the scenario that previously never dropped the first inserted component.

crates/bevy_ecs/src/storage/blob_vec.rs

crates/bevy_ecs/src/world/entity_ref.rs

bjorn3 · 2021-05-21T08:04:28Z

Nice reduction of UnsafeCell usage!

NathanSWard · 2021-05-21T16:34:39Z

Just as a note, this will probably start to fail once #2217 gets merged (or the other way around) since ComponentTicks are no longer Copy.

cart · 2021-05-24T23:43:42Z

crates/bevy_ecs/src/component/mod.rs

 pub struct ComponentTicks {
    pub(crate) added: u32,
-    pub(crate) changed: u32,
+    pub(crate) changed: Cell<u32>,


I think this is actually counterintuitively a reduction in safety. Cell is !Sync, and calling set_changed() from multiple threads at the same time violates that constraint. On main we only provide mutable access to ComponentTicks when the caller has unique access to the corresponding component across all threads, so the old UnsafeCell wrapper doesn't allow invalid access in userspace. But now callers without unique access can call set_changed, which allows cross thread mutation of Cell.

I think technically userspace is still "safe" right now with this pr because there are no public interfaces that expose &ComponentTicks directly (ex: Res<T> has an internal &ComponentTicks reference). But imo this change makes it much easier to accidentally expose unsafe behavior. Its tempting to look at Res<T> and think "we should expose set_changed() here ... its a safe api so we're fine".

I see. I somehow ignored the part where things get sent across threads, and compiler let me do it because of existing pointer casts. I'll try to work around that and get rid of Cell. I still think I can end up with better placement for UnsafeCell, as I believe having it over the whole Vec wasn't correct. Safe alternative would be to use an atomic in place of cell, but It's likely to impact performance, so I'm not doing it right now.

I think at some point we would also think about using GhostCell once its implementation matures. I believe we would be able to remove a lot of unsafety that way.

Cool cool. Definitely happy to consider alternatives. For now, can you revert to the current UnsafeCell impl so we don't need to block the drop fixes on UnsafeCell changes (which appear to be orthogonal).

I don't want to revert it outright, as I believe a lot of those changes are still valid and benefitial. I will make it more similar to previous implementation though and will keep UnsafeCell around.

One reason is that I believe this line inside get_with_ticks is a sneaky UB, and I want to get rid of it

let ticks = &mut *self.ticks.get();

Because even though we only really use that reference in order to get the ticks at specific index, we do create a unique reference to the vector while possibly holding unique references to its parts (some ticks at other indices). It's totally possible to reallocate that vec by accident through shared reference and invalidate all the live ticks references, which should not ever be possible.

For that reason, I want to use Vec<UnsafeCell<ComponentTicks>> instead.

…n component overwrite

…f incorrect Cell in multithreaded context

Frizi · 2021-05-25T20:50:35Z

Okay, I believe it should be correct now. No more Cell in threaded context. There is actually a bit more of UnsafeCell around the code now, but an UB of accessing the aliased vector through unique reference got removed.

bjorn3 · 2021-05-26T08:17:02Z

crates/bevy_ecs/src/query/fetch.rs

@@ -386,7 +387,7 @@ impl<T: Component> WorldQuery for &mut T {
 pub struct WriteFetch<T> {
    storage_type: StorageType,
    table_components: NonNull<T>,
-    table_ticks: *mut ComponentTicks,
+    table_ticks: *const UnsafeCell<ComponentTicks>,


Pointers are allowed to alias, so this UnsafeCell is not necessary. You can store the result of UnsafeCell::get here.

I can't do that without creating a reference to the wrong element. This pointer is being offset before reading, thus it represents a reference to the whole vec slice. I can't get rid of that UnsafeCell and cast that to *mut at the same time. The only correct way to do that right now requires a &UnsafeCell<ComponentTicks> to exist first. This might be changed once UnsafeCell::raw_get gets stabilised.

bjorn3 · 2021-05-26T08:17:21Z

crates/bevy_ecs/src/storage/table.rs

@@ -34,7 +34,7 @@ impl TableId {
 pub struct Column {
    pub(crate) component_id: ComponentId,
    pub(crate) data: BlobVec,
-    pub(crate) ticks: UnsafeCell<Vec<ComponentTicks>>,
+    pub(crate) ticks: Vec<UnsafeCell<ComponentTicks>>,


cart · 2021-05-30T20:15:09Z

Alrighty it hurts, but I've resolved to merge this despite the additional "for loop" query iteration perf regression (in addition to the previous pr's regression). Correctness needs to come first and we can hopefully reclaim the perf when we move to static component storage.

cart · 2021-05-30T20:15:25Z

bors r+

Continuing the work on reducing the safety footguns in the code, I've removed one extra `UnsafeCell` in favour of safe `Cell` usage inisde `ComponentTicks`. That change led to discovery of misbehaving component insert logic, where data wasn't properly dropped when overwritten. Apart from that being fixed, some method names were changed to better convey the "initialize new allocation" and "replace existing allocation" semantic. Depends on #2221, I will rebase this PR after the dependency is merged. For now, review just the last commit. Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2021-05-30T20:34:35Z

Pull request successfully merged into main.

Build succeeded:

Continuing the work on reducing the safety footguns in the code, I've removed one extra `UnsafeCell` in favour of safe `Cell` usage inisde `ComponentTicks`. That change led to discovery of misbehaving component insert logic, where data wasn't properly dropped when overwritten. Apart from that being fixed, some method names were changed to better convey the "initialize new allocation" and "replace existing allocation" semantic. Depends on bevyengine#2221, I will rebase this PR after the dependency is merged. For now, review just the last commit. Co-authored-by: Carter Anderson <mcanders1@gmail.com>

Frizi force-pushed the reinsert-drop branch 2 times, most recently from 9667281 to 79fb468 Compare May 21, 2021 00:38

Frizi commented May 21, 2021

View reviewed changes

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels May 21, 2021

bjorn3 reviewed May 21, 2021

View reviewed changes

crates/bevy_ecs/src/storage/blob_vec.rs Outdated Show resolved Hide resolved

bjorn3 reviewed May 21, 2021

View reviewed changes

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved

Frizi force-pushed the reinsert-drop branch 2 times, most recently from d1e84fd to 547fa26 Compare May 21, 2021 10:16

Frizi mentioned this pull request May 24, 2021

[Merged by Bors] - reduce tricky unsafety and simplify table structure #2221

Closed

cart reviewed May 24, 2021

View reviewed changes

Frizi added 2 commits May 25, 2021 21:15

replace UnsafeCell with Cell in ComponentTicks and fix missing drop o…

95c211c

…n component overwrite

reintroduce UnsafeCell around individual ComponentTicks and get rid o…

5298a26

…f incorrect Cell in multithreaded context

Frizi force-pushed the reinsert-drop branch from 547fa26 to 5298a26 Compare May 25, 2021 20:51

Frizi added 2 commits May 25, 2021 22:53

fix typo in comment

99542f8

remove unnecessary map

ab8dada

bjorn3 reviewed May 26, 2021

View reviewed changes

Merge branch 'main' into reinsert-drop

ccd5837

bors bot changed the title ~~drop overwritten component data on double insert~~ [Merged by Bors] - drop overwritten component data on double insert May 30, 2021

bors bot closed this May 30, 2021

tygyh mentioned this pull request Feb 13, 2024

NonMaxUsize for SparseArrays #11843

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - drop overwritten component data on double insert #2227

[Merged by Bors] - drop overwritten component data on double insert #2227

Frizi commented May 21, 2021 •

edited

Loading

Frizi May 21, 2021

bjorn3 commented May 21, 2021

NathanSWard commented May 21, 2021

cart May 24, 2021

cart May 25, 2021 •

edited

Loading

Frizi May 25, 2021

cart May 25, 2021

Frizi May 25, 2021

Frizi commented May 25, 2021 •

edited

Loading

bjorn3 May 26, 2021

Frizi May 26, 2021

bjorn3 May 26, 2021

bjorn3 May 26, 2021

cart commented May 30, 2021

cart commented May 30, 2021

bors bot commented May 30, 2021

[Merged by Bors] - drop overwritten component data on double insert #2227

[Merged by Bors] - drop overwritten component data on double insert #2227

Conversation

Frizi commented May 21, 2021 • edited Loading

Choose a reason for hiding this comment

bjorn3 commented May 21, 2021

NathanSWard commented May 21, 2021

Choose a reason for hiding this comment

cart May 25, 2021 • edited Loading

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

Frizi commented May 25, 2021 • edited Loading

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

cart commented May 30, 2021

cart commented May 30, 2021

bors bot commented May 30, 2021

Frizi commented May 21, 2021 •

edited

Loading

cart May 25, 2021 •

edited

Loading

Frizi commented May 25, 2021 •

edited

Loading