From dcaefaf2f72bd3dd6fcf24e0b750e0eb9c06486b Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Sun, 31 Jul 2022 13:07:03 +0300 Subject: [PATCH 01/12] better flush --- crates/bevy_render/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/bevy_render/src/lib.rs b/crates/bevy_render/src/lib.rs index bebf80dde2fcd..bcda7bca230db 100644 --- a/crates/bevy_render/src/lib.rs +++ b/crates/bevy_render/src/lib.rs @@ -226,7 +226,10 @@ impl Plugin for RenderPlugin { // flushing as "invalid" ensures that app world entities aren't added as "empty archetype" entities by default // these entities cannot be accessed without spawning directly onto them // this _only_ works as expected because clear_entities() is called at the end of every frame. - unsafe { render_app.world.entities_mut() }.flush_as_invalid(); + + // flushing with a no-op instead of setting the ArchetypeId to invalid as with flush_as_invalid is safe because + // the current state of `Entities` ensures that the code path taken will initialize this way + unsafe { render_app.world.entities_mut().flush(|_, _| {}) }; } { From aeb52306fdf1945afeb2b9c8ab2dfc5dd7b3b08d Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 1 Aug 2022 09:12:02 +0300 Subject: [PATCH 02/12] more docs --- crates/bevy_ecs/src/entity/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index e022483fa836c..2deb956a45a0a 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -550,6 +550,9 @@ impl Entities { /// Flush _must_ set the entity location to the correct [`ArchetypeId`] for the given [`Entity`] /// each time init is called. This _can_ be [`ArchetypeId::INVALID`], provided the [`Entity`] /// has not been assigned to an [`Archetype`][crate::archetype::Archetype]. + /// + /// Note: freshly-allocated entities (ones which don't come from the pending list) are guaranteed + /// to be initialized with the invalid archetype. pub unsafe fn flush(&mut self, mut init: impl FnMut(Entity, &mut EntityLocation)) { let free_cursor = self.free_cursor.get_mut(); let current_free_cursor = *free_cursor; From a5f8c03373b914f0cffa03aa81d2c882c1bf650a Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 1 Aug 2022 09:16:10 +0300 Subject: [PATCH 03/12] Add assert for added safety --- crates/bevy_render/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/bevy_render/src/lib.rs b/crates/bevy_render/src/lib.rs index bcda7bca230db..dcf23a8b4c8cd 100644 --- a/crates/bevy_render/src/lib.rs +++ b/crates/bevy_render/src/lib.rs @@ -218,6 +218,13 @@ impl Plugin for RenderPlugin { // reserve all existing app entities for use in render_app // they can only be spawned using `get_or_spawn()` let meta_len = app_world.entities().meta_len(); + + assert_eq!( + render_app.world.entities().len(), + 0, + "An entity was spawned after the entity list was cleared last frame and before the extract stage began. This is not supported", + ); + render_app .world .entities() From a745ad6ce668dac39b0c410c0ed806af3d8bb0d0 Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 1 Aug 2022 10:18:01 +0300 Subject: [PATCH 04/12] switch to ridiculously fast memset --- crates/bevy_ecs/src/entity/mod.rs | 15 +++++++++++++++ crates/bevy_render/src/lib.rs | 14 +++++++------- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 2deb956a45a0a..79f95d402baf7 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -603,6 +603,21 @@ impl Entities { } } + /// # Safety: + /// + /// This function is safe if and only if the world this Entities is on has no entities. + pub unsafe fn flush_and_reserve_invalid_assuming_no_entities(&mut self, count: usize) { + let free_cursor = self.free_cursor.get_mut(); + let alloc_count = -*free_cursor as usize; + *free_cursor = 0; + let meta = &mut self.meta; + meta.reserve(count); + let ptr = meta.as_mut_ptr(); + // the EntityMeta struct only contains integers, and it is valid to have all bytes set to u8::MAX + ptr.write_bytes(u8::MAX, count); + self.len = alloc_count as u32; + } + /// Accessor for getting the length of the vec in `self.meta` #[inline] pub fn meta_len(&self) -> usize { diff --git a/crates/bevy_render/src/lib.rs b/crates/bevy_render/src/lib.rs index dcf23a8b4c8cd..bfe2616afe61e 100644 --- a/crates/bevy_render/src/lib.rs +++ b/crates/bevy_render/src/lib.rs @@ -230,13 +230,13 @@ impl Plugin for RenderPlugin { .entities() .reserve_entities(meta_len as u32); - // flushing as "invalid" ensures that app world entities aren't added as "empty archetype" entities by default - // these entities cannot be accessed without spawning directly onto them - // this _only_ works as expected because clear_entities() is called at the end of every frame. - - // flushing with a no-op instead of setting the ArchetypeId to invalid as with flush_as_invalid is safe because - // the current state of `Entities` ensures that the code path taken will initialize this way - unsafe { render_app.world.entities_mut().flush(|_, _| {}) }; + // This is safe given the clear_entities call in the past frame and the assert above + unsafe { + render_app + .world + .entities_mut() + .flush_and_reserve_invalid_assuming_no_entities(meta_len); + } } { From 9b0e2983d2f913ca4c3a327de2ebc0b32da5c029 Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 1 Aug 2022 10:24:10 +0300 Subject: [PATCH 05/12] fix docs --- crates/bevy_ecs/src/entity/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 79f95d402baf7..abb1a84bd1133 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -603,7 +603,7 @@ impl Entities { } } - /// # Safety: + /// # Safety /// /// This function is safe if and only if the world this Entities is on has no entities. pub unsafe fn flush_and_reserve_invalid_assuming_no_entities(&mut self, count: usize) { From 9f106d8b889e00a69389704f28eef99b20633699 Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 1 Aug 2022 10:30:45 +0300 Subject: [PATCH 06/12] final fixes --- crates/bevy_ecs/src/entity/mod.rs | 3 +-- crates/bevy_render/src/lib.rs | 5 ----- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index abb1a84bd1133..254e554d84597 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -608,14 +608,13 @@ impl Entities { /// This function is safe if and only if the world this Entities is on has no entities. pub unsafe fn flush_and_reserve_invalid_assuming_no_entities(&mut self, count: usize) { let free_cursor = self.free_cursor.get_mut(); - let alloc_count = -*free_cursor as usize; *free_cursor = 0; let meta = &mut self.meta; meta.reserve(count); let ptr = meta.as_mut_ptr(); // the EntityMeta struct only contains integers, and it is valid to have all bytes set to u8::MAX ptr.write_bytes(u8::MAX, count); - self.len = alloc_count as u32; + self.len = count as u32; } /// Accessor for getting the length of the vec in `self.meta` diff --git a/crates/bevy_render/src/lib.rs b/crates/bevy_render/src/lib.rs index bfe2616afe61e..558f52eebd0db 100644 --- a/crates/bevy_render/src/lib.rs +++ b/crates/bevy_render/src/lib.rs @@ -225,11 +225,6 @@ impl Plugin for RenderPlugin { "An entity was spawned after the entity list was cleared last frame and before the extract stage began. This is not supported", ); - render_app - .world - .entities() - .reserve_entities(meta_len as u32); - // This is safe given the clear_entities call in the past frame and the assert above unsafe { render_app From e283fc605733f024fc883884d025fc0783a71931 Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 1 Aug 2022 10:33:44 +0300 Subject: [PATCH 07/12] mirror safety comment on EntityMeta --- crates/bevy_ecs/src/entity/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 254e554d84597..61b259f0962ed 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -634,6 +634,8 @@ impl Entities { } } +// Safety: +// This type must not contain any pointers at any level, and be safe to fully fill with u8::MAX. #[derive(Copy, Clone, Debug)] pub struct EntityMeta { pub generation: u32, From 8a32577381155838ad8bf57d4c295eb95a9d2b78 Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 1 Aug 2022 10:35:15 +0300 Subject: [PATCH 08/12] add repr C --- crates/bevy_ecs/src/entity/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 61b259f0962ed..ee845e8bd9b8d 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -637,6 +637,7 @@ impl Entities { // Safety: // This type must not contain any pointers at any level, and be safe to fully fill with u8::MAX. #[derive(Copy, Clone, Debug)] +#[repr(C)] pub struct EntityMeta { pub generation: u32, pub location: EntityLocation, From becb9e779d3918111c01b0fe4d06e6951af28bdf Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 1 Aug 2022 16:10:35 +0300 Subject: [PATCH 09/12] add DO_UB flag --- crates/bevy_ecs/src/archetype.rs | 1 + crates/bevy_ecs/src/entity/mod.rs | 30 ++++++++++++++++++++++++------ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index cf81b5b633a2e..aeb96adf80adf 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -14,6 +14,7 @@ use std::{ }; #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] +#[repr(transparent)] pub struct ArchetypeId(usize); impl ArchetypeId { diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index ee845e8bd9b8d..b48fb3d4e6a5a 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -42,7 +42,8 @@ pub use map_entities::*; use crate::{archetype::ArchetypeId, storage::SparseSetIndex}; use std::{ convert::TryFrom, - fmt, mem, + fmt, + mem::{self, MaybeUninit}, sync::atomic::{AtomicI64, Ordering}, }; @@ -609,11 +610,25 @@ impl Entities { pub unsafe fn flush_and_reserve_invalid_assuming_no_entities(&mut self, count: usize) { let free_cursor = self.free_cursor.get_mut(); *free_cursor = 0; - let meta = &mut self.meta; - meta.reserve(count); - let ptr = meta.as_mut_ptr(); - // the EntityMeta struct only contains integers, and it is valid to have all bytes set to u8::MAX - ptr.write_bytes(u8::MAX, count); + self.meta.reserve(count); + const DO_UB: bool = false; + if DO_UB { + // the EntityMeta struct only contains integers, and it is valid to have all bytes set to u8::MAX + self.meta.as_mut_ptr().write_bytes(u8::MAX, count); + } else { + self.meta.resize( + count, + EntityMeta { + generation: u32::MAX, + _padding: MaybeUninit::uninit(), + location: EntityLocation { + archetype_id: ArchetypeId::INVALID, + index: usize::MAX, // dummy value, to be filled in + }, + }, + ); + } + self.len = count as u32; } @@ -640,12 +655,14 @@ impl Entities { #[repr(C)] pub struct EntityMeta { pub generation: u32, + pub _padding: MaybeUninit, pub location: EntityLocation, } impl EntityMeta { const EMPTY: EntityMeta = EntityMeta { generation: 0, + _padding: MaybeUninit::uninit(), location: EntityLocation { archetype_id: ArchetypeId::INVALID, index: usize::MAX, // dummy value, to be filled in @@ -655,6 +672,7 @@ impl EntityMeta { /// A location of an entity in an archetype. #[derive(Copy, Clone, Debug)] +#[repr(C)] pub struct EntityLocation { /// The archetype index pub archetype_id: ArchetypeId, From b02a1d741c02174269311fa36210e15c740ba048 Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 1 Aug 2022 16:26:04 +0300 Subject: [PATCH 10/12] fix ub --- crates/bevy_ecs/src/entity/mod.rs | 25 ++++---------------- examples/hello_world.rs | 38 +++++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 26 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index b48fb3d4e6a5a..4a883ccff785e 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -42,8 +42,7 @@ pub use map_entities::*; use crate::{archetype::ArchetypeId, storage::SparseSetIndex}; use std::{ convert::TryFrom, - fmt, - mem::{self, MaybeUninit}, + fmt, mem, sync::atomic::{AtomicI64, Ordering}, }; @@ -611,23 +610,9 @@ impl Entities { let free_cursor = self.free_cursor.get_mut(); *free_cursor = 0; self.meta.reserve(count); - const DO_UB: bool = false; - if DO_UB { - // the EntityMeta struct only contains integers, and it is valid to have all bytes set to u8::MAX - self.meta.as_mut_ptr().write_bytes(u8::MAX, count); - } else { - self.meta.resize( - count, - EntityMeta { - generation: u32::MAX, - _padding: MaybeUninit::uninit(), - location: EntityLocation { - archetype_id: ArchetypeId::INVALID, - index: usize::MAX, // dummy value, to be filled in - }, - }, - ); - } + // the EntityMeta struct only contains integers, and it is valid to have all bytes set to u8::MAX + self.meta.as_mut_ptr().write_bytes(u8::MAX, count); + self.meta.set_len(count); self.len = count as u32; } @@ -655,14 +640,12 @@ impl Entities { #[repr(C)] pub struct EntityMeta { pub generation: u32, - pub _padding: MaybeUninit, pub location: EntityLocation, } impl EntityMeta { const EMPTY: EntityMeta = EntityMeta { generation: 0, - _padding: MaybeUninit::uninit(), location: EntityLocation { archetype_id: ArchetypeId::INVALID, index: usize::MAX, // dummy value, to be filled in diff --git a/examples/hello_world.rs b/examples/hello_world.rs index 3f09f5c4c7ab9..aa7262e4f9522 100644 --- a/examples/hello_world.rs +++ b/examples/hello_world.rs @@ -1,9 +1,37 @@ -use bevy::prelude::*; +use bevy::{ + diagnostic::{FrameTimeDiagnosticsPlugin, LogDiagnosticsPlugin}, + prelude::*, +}; -fn main() { - App::new().add_system(hello_world_system).run(); +#[derive(Component)] +pub struct Nothing; + +#[derive(Bundle)] +pub struct NoBundle { + nothing: Nothing, +} + +fn startup(mut commands: Commands) { + let mut entities = Vec::new(); + for _ in 0..40_000_000 { + entities.push(NoBundle { nothing: Nothing }); + } + + commands.spawn_batch(entities); } -fn hello_world_system() { - println!("hello world"); +fn main() { + App::new() + .insert_resource(WindowDescriptor { + width: 1270.0, + height: 720.0, + title: String::from("Bug"), + ..Default::default() + }) + .insert_resource(ClearColor(Color::rgb(0.211, 0.643, 0.949))) + .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(LogDiagnosticsPlugin::default()) + .add_plugins(DefaultPlugins) + .add_startup_system(startup) + .run(); } From e8be324f0c86da7e887b307493c4a307362a21eb Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Mon, 1 Aug 2022 22:32:46 +0300 Subject: [PATCH 11/12] revert example --- examples/hello_world.rs | 38 +++++--------------------------------- 1 file changed, 5 insertions(+), 33 deletions(-) diff --git a/examples/hello_world.rs b/examples/hello_world.rs index aa7262e4f9522..3f09f5c4c7ab9 100644 --- a/examples/hello_world.rs +++ b/examples/hello_world.rs @@ -1,37 +1,9 @@ -use bevy::{ - diagnostic::{FrameTimeDiagnosticsPlugin, LogDiagnosticsPlugin}, - prelude::*, -}; +use bevy::prelude::*; -#[derive(Component)] -pub struct Nothing; - -#[derive(Bundle)] -pub struct NoBundle { - nothing: Nothing, -} - -fn startup(mut commands: Commands) { - let mut entities = Vec::new(); - for _ in 0..40_000_000 { - entities.push(NoBundle { nothing: Nothing }); - } - - commands.spawn_batch(entities); +fn main() { + App::new().add_system(hello_world_system).run(); } -fn main() { - App::new() - .insert_resource(WindowDescriptor { - width: 1270.0, - height: 720.0, - title: String::from("Bug"), - ..Default::default() - }) - .insert_resource(ClearColor(Color::rgb(0.211, 0.643, 0.949))) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) - .add_plugin(LogDiagnosticsPlugin::default()) - .add_plugins(DefaultPlugins) - .add_startup_system(startup) - .run(); +fn hello_world_system() { + println!("hello world"); } From d500cbe9759265d57d91eb8891eb572feee53547 Mon Sep 17 00:00:00 2001 From: TheRawMeatball Date: Tue, 9 Aug 2022 13:24:17 +0300 Subject: [PATCH 12/12] add safety comment. --- crates/bevy_ecs/src/archetype.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index aeb96adf80adf..98dda00a9cbc4 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -20,6 +20,9 @@ pub struct ArchetypeId(usize); impl ArchetypeId { pub const EMPTY: ArchetypeId = ArchetypeId(0); pub const RESOURCE: ArchetypeId = ArchetypeId(1); + /// # Safety: + /// + /// This must always have an all-1s bit pattern to ensure soundness in fast entity id space allocation. pub const INVALID: ArchetypeId = ArchetypeId(usize::MAX); #[inline]