From 3cae9946c3abfd7569c214a61709de060b861840 Mon Sep 17 00:00:00 2001 From: James O'Brien Date: Sun, 21 Jan 2024 15:34:14 -0800 Subject: [PATCH 1/6] Fix memory leak in dynamic example --- examples/ecs/dynamic.rs | 53 +++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/examples/ecs/dynamic.rs b/examples/ecs/dynamic.rs index fa1e84d53f7f2..67eb02ae656b0 100644 --- a/examples/ecs/dynamic.rs +++ b/examples/ecs/dynamic.rs @@ -10,7 +10,7 @@ use bevy::{ query::{QueryBuilder, QueryData}, world::FilteredEntityMut, }, - ptr::OwningPtr, + ptr::{Aligned, OwningPtr}, utils::HashMap, }; @@ -81,6 +81,7 @@ fn main() { Some(Ok(size)) => size, _ => 0, }; + // Register our new component to the world with a layout specified by it's size // SAFETY: [u64] is Send + Sync let id = world.init_component_with_descriptor(unsafe { ComponentDescriptor::new_with_layout( @@ -100,44 +101,44 @@ fn main() { } "s" => { let mut to_insert_ids = Vec::new(); - let mut to_insert_ptr = Vec::new(); + let mut to_insert_data = Vec::new(); rest.split(',').for_each(|component| { let mut component = component.split_whitespace(); let Some(name) = component.next() else { return; }; + + // Get the id for the component with the given name let Some(&id) = component_names.get(name) else { println!("Component {} does not exist", name); return; }; + + // Calculate the length for the array based on the layout created for this component id let info = world.components().get_info(id).unwrap(); let len = info.layout().size() / std::mem::size_of::(); - let mut values: Vec = component + let values: Vec = component .take(len) .filter_map(|value| value.parse::().ok()) .collect(); - // SAFETY: - // - All components will be interpreted as [u64] - // - len and layout are taken directly from the component descriptor - let ptr = unsafe { - let data = std::alloc::alloc_zeroed(info.layout()).cast::(); - data.copy_from(values.as_mut_ptr(), values.len()); - let non_null = NonNull::new_unchecked(data.cast()); - OwningPtr::new(non_null) - }; - + // Collect the id and array to be inserted onto our entity to_insert_ids.push(id); - to_insert_ptr.push(ptr); + to_insert_data.push(values); }); let mut entity = world.spawn_empty(); + + // Construct an `OwningPtr` for each component in `to_insert_data` + let to_insert_ptr = to_owned_ptrs(&mut to_insert_data); + // SAFETY: // - Component ids have been taken from the same world - // - The pointer with the correct layout + // - Each array is created to the layout specified in the world unsafe { entity.insert_by_ids(&to_insert_ids, to_insert_ptr.into_iter()); } + println!("Entity spawned with id: {:?}", entity.id()); } "q" => { @@ -162,6 +163,8 @@ fn main() { len, ) }; + + // If we have write access, increment each value once if filtered_entity.access().has_write(id) { data.iter_mut().for_each(|data| { *data += 1; @@ -181,6 +184,23 @@ fn main() { } } +// Constructs `OwningPtr` for each item in `data` +// By sharing the lifetime of `data` with the resulting ptrs we ensure drop the data before use +fn to_owned_ptrs(data: &mut [Vec]) -> Vec> { + let mut ptrs = Vec::new(); + for data in data.iter_mut() { + let ptr = data.as_mut_ptr(); + // SAFETY: + // - Pointers are guaranteed to be non-null + // - Data pointed to won't be dropped until `to_insert_data` is dropped + unsafe { + let non_null = NonNull::new_unchecked(ptr.cast()); + ptrs.push(OwningPtr::new(non_null)) + } + } + ptrs +} + fn parse_term( str: &str, builder: &mut QueryBuilder, @@ -189,10 +209,12 @@ fn parse_term( let mut matched = false; let str = str.trim(); match str.chars().next() { + // Optional term Some('?') => { builder.optional(|b| parse_term(&str[1..], b, components)); matched = true; } + // Reference term Some('&') => { let mut parts = str.split_whitespace(); let first = parts.next().unwrap(); @@ -208,6 +230,7 @@ fn parse_term( matched = true; } } + // With term Some(_) => { if let Some(&id) = components.get(str) { builder.with_id(id); From 13ac8276567853462a2c10d4f28cecc1f547da20 Mon Sep 17 00:00:00 2001 From: James O'Brien Date: Sun, 21 Jan 2024 15:44:33 -0800 Subject: [PATCH 2/6] Zero out extra values --- examples/ecs/dynamic.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/examples/ecs/dynamic.rs b/examples/ecs/dynamic.rs index 67eb02ae656b0..f07848f4c993c 100644 --- a/examples/ecs/dynamic.rs +++ b/examples/ecs/dynamic.rs @@ -117,10 +117,11 @@ fn main() { // Calculate the length for the array based on the layout created for this component id let info = world.components().get_info(id).unwrap(); let len = info.layout().size() / std::mem::size_of::(); - let values: Vec = component + let mut values: Vec = component .take(len) .filter_map(|value| value.parse::().ok()) .collect(); + values.resize(len, 0); // Collect the id and array to be inserted onto our entity to_insert_ids.push(id); From 191b0dc729460028e2b78ed68750aeac3b70c190 Mon Sep 17 00:00:00 2001 From: James O'Brien Date: Sun, 21 Jan 2024 15:50:25 -0800 Subject: [PATCH 3/6] Fix formatting --- examples/ecs/dynamic.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/ecs/dynamic.rs b/examples/ecs/dynamic.rs index f07848f4c993c..50c67f84159b4 100644 --- a/examples/ecs/dynamic.rs +++ b/examples/ecs/dynamic.rs @@ -196,7 +196,7 @@ fn to_owned_ptrs(data: &mut [Vec]) -> Vec> { // - Data pointed to won't be dropped until `to_insert_data` is dropped unsafe { let non_null = NonNull::new_unchecked(ptr.cast()); - ptrs.push(OwningPtr::new(non_null)) + ptrs.push(OwningPtr::new(non_null)); } } ptrs From 49462dcd469ae33cecb4cf75af38a10623914c3e Mon Sep 17 00:00:00 2001 From: James O'Brien Date: Sun, 21 Jan 2024 15:52:31 -0800 Subject: [PATCH 4/6] Making naming more consistent --- examples/ecs/dynamic.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/ecs/dynamic.rs b/examples/ecs/dynamic.rs index 50c67f84159b4..d77d0fa72dfb3 100644 --- a/examples/ecs/dynamic.rs +++ b/examples/ecs/dynamic.rs @@ -131,7 +131,7 @@ fn main() { let mut entity = world.spawn_empty(); // Construct an `OwningPtr` for each component in `to_insert_data` - let to_insert_ptr = to_owned_ptrs(&mut to_insert_data); + let to_insert_ptr = to_owning_ptrs(&mut to_insert_data); // SAFETY: // - Component ids have been taken from the same world @@ -187,7 +187,7 @@ fn main() { // Constructs `OwningPtr` for each item in `data` // By sharing the lifetime of `data` with the resulting ptrs we ensure drop the data before use -fn to_owned_ptrs(data: &mut [Vec]) -> Vec> { +fn to_owning_ptrs(data: &mut [Vec]) -> Vec> { let mut ptrs = Vec::new(); for data in data.iter_mut() { let ptr = data.as_mut_ptr(); From 2c228bc573e1d667fab89142df6528265cc9c97d Mon Sep 17 00:00:00 2001 From: James O'Brien Date: Sun, 21 Jan 2024 15:55:02 -0800 Subject: [PATCH 5/6] Fix typo in comment --- examples/ecs/dynamic.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/ecs/dynamic.rs b/examples/ecs/dynamic.rs index d77d0fa72dfb3..40cd3aefe480b 100644 --- a/examples/ecs/dynamic.rs +++ b/examples/ecs/dynamic.rs @@ -186,7 +186,7 @@ fn main() { } // Constructs `OwningPtr` for each item in `data` -// By sharing the lifetime of `data` with the resulting ptrs we ensure drop the data before use +// By sharing the lifetime of `data` with the resulting ptrs we ensure we don't drop the data before use fn to_owning_ptrs(data: &mut [Vec]) -> Vec> { let mut ptrs = Vec::new(); for data in data.iter_mut() { From aac7458ba5a034dc73109c0f109094c595af1313 Mon Sep 17 00:00:00 2001 From: James O'Brien Date: Sun, 21 Jan 2024 16:18:04 -0800 Subject: [PATCH 6/6] Improve naming --- examples/ecs/dynamic.rs | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/examples/ecs/dynamic.rs b/examples/ecs/dynamic.rs index 40cd3aefe480b..3537f335fca90 100644 --- a/examples/ecs/dynamic.rs +++ b/examples/ecs/dynamic.rs @@ -185,21 +185,22 @@ fn main() { } } -// Constructs `OwningPtr` for each item in `data` -// By sharing the lifetime of `data` with the resulting ptrs we ensure we don't drop the data before use -fn to_owning_ptrs(data: &mut [Vec]) -> Vec> { - let mut ptrs = Vec::new(); - for data in data.iter_mut() { - let ptr = data.as_mut_ptr(); - // SAFETY: - // - Pointers are guaranteed to be non-null - // - Data pointed to won't be dropped until `to_insert_data` is dropped - unsafe { - let non_null = NonNull::new_unchecked(ptr.cast()); - ptrs.push(OwningPtr::new(non_null)); - } - } - ptrs +// Constructs `OwningPtr` for each item in `components` +// By sharing the lifetime of `components` with the resulting ptrs we ensure we don't drop the data before use +fn to_owning_ptrs(components: &mut [Vec]) -> Vec> { + components + .iter_mut() + .map(|data| { + let ptr = data.as_mut_ptr(); + // SAFETY: + // - Pointers are guaranteed to be non-null + // - Memory pointed to won't be dropped until `components` is dropped + unsafe { + let non_null = NonNull::new_unchecked(ptr.cast()); + OwningPtr::new(non_null) + } + }) + .collect() } fn parse_term(