From c98364bd2732ce93a16f9a2aa1a6fc5ee16697de Mon Sep 17 00:00:00 2001 From: Wodann Date: Fri, 8 May 2020 18:14:53 +0200 Subject: [PATCH] feat(memory): map structs with different memory kinds this also fixes "identity" memory mapping, where only an old type pointer needed to be replaced with a new pointer to the same type information --- crates/mun_memory/src/gc/mark_sweep.rs | 240 ++++++++++++++++++++++--- crates/mun_memory/src/mapping.rs | 87 +++++++-- crates/mun_runtime/tests/memory.rs | 204 +++++++++++++++++++++ 3 files changed, 491 insertions(+), 40 deletions(-) diff --git a/crates/mun_memory/src/gc/mark_sweep.rs b/crates/mun_memory/src/gc/mark_sweep.rs index 564f490c2..abd475133 100644 --- a/crates/mun_memory/src/gc/mark_sweep.rs +++ b/crates/mun_memory/src/gc/mark_sweep.rs @@ -2,7 +2,7 @@ use crate::{ cast, gc::{Event, GcPtr, GcRuntime, Observer, RawGcPtr, Stats, TypeTrace}, mapping::{self, FieldMapping, MemoryMapper}, - TypeLayout, + TypeDesc, TypeLayout, }; use mapping::{Conversion, Mapping}; use parking_lot::RwLock; @@ -207,7 +207,7 @@ where impl MemoryMapper for MarkSweep where - T: TypeLayout + TypeTrace + Clone + Eq + Hash, + T: TypeDesc + TypeLayout + TypeTrace + Clone + Eq + Hash, O: Observer, { fn map_memory(&self, mapping: Mapping) -> Vec { @@ -225,62 +225,248 @@ where }) .collect(); - for (old_ty, conversion) in mapping.conversions { + // Update type pointers of types that didn't change + for (old_ty, new_ty) in mapping.identical { for object_info in objects.values_mut() { if object_info.ty == old_ty { - map_fields(object_info, &conversion); + object_info.set(ObjectInfo { + ptr: object_info.ptr, + roots: object_info.roots, + color: object_info.color, + ty: new_ty.clone(), + }); } } } - return deleted; + let mut new_allocations = Vec::new(); + + for (old_ty, conversion) in mapping.conversions.iter() { + for object_info in objects.values_mut() { + if object_info.ty == *old_ty { + let src = unsafe { NonNull::new_unchecked(object_info.ptr) }; + let dest = unsafe { + NonNull::new_unchecked(std::alloc::alloc_zeroed(conversion.new_ty.layout())) + }; + + map_fields( + self, + &mut new_allocations, + &mapping.conversions, + &conversion.field_mapping, + src, + dest, + ); + + unsafe { std::alloc::dealloc(src.as_ptr(), old_ty.layout()) }; + + object_info.set(ObjectInfo { + ptr: dest.as_ptr(), + roots: object_info.roots, + color: object_info.color, + ty: conversion.new_ty.clone(), + }); + } + } + } + + // Retroactively store newly allocated objects + // This cannot be done while mapping because we hold a mutable reference to objects + for object in new_allocations { + let ty = object.ty.clone(); + // We want to return a pointer to the `ObjectInfo`, to + // be used as handle. + let handle = (object.as_ref().deref() as *const _ as RawGcPtr).into(); + objects.insert(handle, object); - fn map_fields( - object_info: &mut Pin>>, - conversion: &Conversion, - ) { - let ptr = unsafe { std::alloc::alloc_zeroed(conversion.new_ty.layout()) }; + self.log_alloc(handle, ty); + } - for map in conversion.field_mapping.iter() { + return deleted; + + fn map_fields( + gc: &MarkSweep, + new_allocations: &mut Vec>>>, + conversions: &HashMap>, + mapping: &[Option>], + src: NonNull, + dest: NonNull, + ) where + T: TypeDesc + TypeLayout + TypeTrace + Clone + Eq + Hash, + O: Observer, + { + for map in mapping.iter() { if let Some(FieldMapping { old_offset, new_offset, action, }) = map { - let src = { - let mut src = object_info.ptr as usize; + let field_src = { + let mut src = src.as_ptr() as usize; src += old_offset; src as *mut u8 }; - let dest = { - let mut dest = ptr as usize; + let field_dest = { + let mut dest = dest.as_ptr() as usize; dest += new_offset; dest as *mut u8 }; match action { mapping::Action::Cast { old, new } => { - if !cast::try_cast_from_to( - *old, - *new, - unsafe { NonNull::new_unchecked(src) }, - unsafe { NonNull::new_unchecked(dest) }, + if let Some(old_memory_kind) = old.memory_kind() { + let new_memory_kind = new.memory_kind().expect( + "We are dealing with a struct and we do not directly compare fundamental types and struct type, so its counterpart must also be a struct.", + ); + + // When the name is the same, we are dealing with the same struct, + // but different internals + let is_same_struct = old.name() == new.name(); + + // If the same struct changed, there must also be a conversion + let conversion = conversions.get(old); + + if old_memory_kind == abi::StructMemoryKind::Value { + if new_memory_kind == abi::StructMemoryKind::Value { + // struct(value) -> struct(value) + if is_same_struct { + // Map in-memory struct to in-memory struct + map_fields( + gc, + new_allocations, + conversions, + &conversion.as_ref().unwrap().field_mapping, + unsafe { NonNull::new_unchecked(field_src) }, + unsafe { NonNull::new_unchecked(field_dest) }, + ); + } else { + // Use previously zero-initialized memory + } + } else { + // struct(value) -> struct(gc) + let object = alloc_obj(new.clone()); + + // We want to return a pointer to the `ObjectInfo`, to be used as handle. + let handle = (object.as_ref().deref() as *const _ + as RawGcPtr) + .into(); + + if is_same_struct { + // Map in-memory struct to heap-allocated struct + map_fields( + gc, + new_allocations, + conversions, + &conversion.as_ref().unwrap().field_mapping, + unsafe { NonNull::new_unchecked(field_src) }, + unsafe { NonNull::new_unchecked(object.ptr) }, + ); + } else { + // Zero initialize heap-allocated object + unsafe { + std::ptr::write_bytes( + (*object).ptr, + 0, + new.layout().size(), + ) + }; + } + + // Write handle to field + let field_handle = field_dest.cast::(); + unsafe { *field_handle = handle }; + + new_allocations.push(object); + } + } else if new_memory_kind == abi::StructMemoryKind::GC { + // struct(gc) -> struct(gc) + let field_src = field_src.cast::(); + let field_dest = field_dest.cast::(); + + if is_same_struct { + // Only copy the `GcPtr`. Memory will already be mapped. + unsafe { + *field_dest = *field_src; + } + } else { + let object = alloc_obj(new.clone()); + + // We want to return a pointer to the `ObjectInfo`, to + // be used as handle. + let handle = (object.as_ref().deref() as *const _ + as RawGcPtr) + .into(); + + // Zero-initialize heap-allocated object + unsafe { + std::ptr::write_bytes( + object.ptr, + 0, + new.layout().size(), + ) + }; + + // Write handle to field + unsafe { + *field_dest = handle; + } + + new_allocations.push(object); + } + } else { + // struct(gc) -> struct(value) + let field_handle = unsafe { *field_src.cast::() }; + + // Convert the handle to our internal representation + // Safety: we already hold a write lock on `objects`, so + // this is legal. + let obj: *mut ObjectInfo = field_handle.into(); + let obj = unsafe { &*obj }; + + if is_same_struct { + if obj.ty == *old { + // The object still needs to be mapped + // Map heap-allocated struct to in-memory struct + map_fields( + gc, + new_allocations, + conversions, + &conversion.as_ref().unwrap().field_mapping, + unsafe { NonNull::new_unchecked(obj.ptr) }, + unsafe { NonNull::new_unchecked(field_dest) }, + ); + } else { + // The object was already mapped + debug_assert!(obj.ty == *new); + + // Copy from heap-allocated struct to in-memory struct + unsafe { + std::ptr::copy_nonoverlapping( + obj.ptr, + field_dest, + obj.ty.layout().size(), + ) + }; + } + } else { + // Use previously zero-initialized memory + } + } + } else if !cast::try_cast_from_to( + *old.guid(), + *new.guid(), + unsafe { NonNull::new_unchecked(field_src) }, + unsafe { NonNull::new_unchecked(field_dest) }, ) { // Failed to cast. Use the previously zero-initialized value instead } } mapping::Action::Copy { size } => unsafe { - std::ptr::copy_nonoverlapping(src, dest, *size) + std::ptr::copy_nonoverlapping(field_src, field_dest, *size); }, } } } - object_info.set(ObjectInfo { - ptr, - roots: object_info.roots, - color: object_info.color, - ty: conversion.new_ty.clone(), - }); } } } diff --git a/crates/mun_memory/src/mapping.rs b/crates/mun_memory/src/mapping.rs index f4573f76e..b994a4319 100644 --- a/crates/mun_memory/src/mapping.rs +++ b/crates/mun_memory/src/mapping.rs @@ -8,28 +8,29 @@ use std::{ hash::Hash, }; -pub struct Mapping { +pub struct Mapping { pub deletions: HashSet, pub conversions: HashMap>, + pub identical: Vec<(T, T)>, } -pub struct Conversion { - pub field_mapping: Vec>, +pub struct Conversion { + pub field_mapping: Vec>>, pub new_ty: T, } /// Description of the mapping of a single field. When stored together with the new index, this /// provides all information necessary for a mapping function. -pub struct FieldMapping { +pub struct FieldMapping { pub old_offset: usize, pub new_offset: usize, - pub action: Action, + pub action: Action, } /// The `Action` to take when mapping memory from A to B. #[derive(Eq, PartialEq)] -pub enum Action { - Cast { old: abi::Guid, new: abi::Guid }, +pub enum Action { + Cast { old: T, new: T }, Copy { size: usize }, } @@ -41,8 +42,11 @@ where pub fn new(old: &[T], new: &[T]) -> Self { let diff = diff(old, new); - let mut deletions = HashSet::new(); let mut conversions = HashMap::new(); + let mut deletions = HashSet::new(); + let mut insertions = HashSet::new(); + + let mut identical = Vec::new(); for diff in diff.iter() { match diff { @@ -58,13 +62,70 @@ where let new_ty = unsafe { *new.get_unchecked(*new_index) }; conversions.insert(old_ty, unsafe { field_mapping(old_ty, new_ty, diff) }); } - Diff::Insert { .. } | Diff::Move { .. } => (), + Diff::Insert { index } => { + insertions.insert(unsafe { *new.get_unchecked(*index) }); + } + Diff::Move { + old_index, + new_index, + } => identical.push(unsafe { + ( + *old.get_unchecked(*old_index), + *new.get_unchecked(*new_index), + ) + }), } } + // These candidates are used to collect a list of `new_index -> old_index` mappings for + // identical types. + let mut new_candidates: HashSet = new + .iter() + // Filter non-struct types + .filter(|ty| ty.memory_kind().is_some()) + // Filter inserted structs + .filter(|ty| !insertions.contains(*ty)) + .cloned() + .collect(); + + let mut old_candidates: HashSet = old + .iter() + // Filter non-struct types + .filter(|ty| ty.memory_kind().is_some()) + // Filter deleted structs + .filter(|ty| !deletions.contains(*ty)) + // Filter edited types + .filter(|ty| { + if let Some(conversion) = conversions.get(*ty) { + // Remove its new counterpart too + new_candidates.remove(&conversion.new_ty); + false + } else { + true + } + }) + .cloned() + .collect(); + + // Remove moved types from the candidates, since we already know they are identical + for (old_ty, new_ty) in identical.iter() { + old_candidates.remove(old_ty); + new_candidates.remove(new_ty); + } + + // Find matching (old_ty, new_ty) pairs + for old_ty in old_candidates { + let new_ty = new_candidates.take(&old_ty).unwrap(); + identical.push((old_ty, new_ty)); + } + + // We should have matched all remaining candidates + debug_assert!(new_candidates.is_empty()); + Self { deletions, conversions, + identical, } } } @@ -77,7 +138,7 @@ where /// # Safety /// /// Expects the `diff` to be based on `old_ty` and `new_ty`. If not, it causes undefined behavior. -pub unsafe fn field_mapping + TypeLayout>( +pub unsafe fn field_mapping + TypeLayout>( old_ty: T, new_ty: T, diff: &[FieldDiff], @@ -179,8 +240,8 @@ pub unsafe fn field_mapping + TypeLayout>( new_offset: usize::from(*new_offsets.get_unchecked(new_index)), action: if desc.action == ActionDesc::Cast { Action::Cast { - old: *old_field.1.guid(), - new: *new_fields.get_unchecked(new_index).1.guid(), + old: old_field.1.clone(), + new: new_fields.get_unchecked(new_index).1.clone(), } } else { Action::Copy { @@ -196,7 +257,7 @@ pub unsafe fn field_mapping + TypeLayout>( } /// A trait used to map allocated memory using type differences. -pub trait MemoryMapper { +pub trait MemoryMapper { /// Maps its allocated memory using the provided `mapping`. /// /// A `Vec` is returned containing all objects of types that were deleted. The diff --git a/crates/mun_runtime/tests/memory.rs b/crates/mun_runtime/tests/memory.rs index 82c94e645..736a8d52c 100644 --- a/crates/mun_runtime/tests/memory.rs +++ b/crates/mun_runtime/tests/memory.rs @@ -532,3 +532,207 @@ fn delete_used_struct() { assert_eq!(foo.get::("b").unwrap(), b); assert_eq!(foo.get::("c").unwrap(), c); } + +#[test] +fn nested_structs() { + let mut driver = TestDriver::new( + r#" + struct(gc) GcStruct(f32, f32); + struct(value) ValueStruct(f32, f32); + + struct(gc) GcWrapper(GcStruct, ValueStruct) + struct(value) ValueWrapper(GcStruct, ValueStruct); + + pub fn new_gc_struct(a: f32, b: f32) -> GcStruct { + GcStruct(a, b) + } + + pub fn new_value_struct(a: f32, b: f32) -> ValueStruct { + ValueStruct(a, b) + } + + pub fn new_gc_wrapper(a: GcStruct, b: ValueStruct) -> GcWrapper { + GcWrapper(a, b) + } + + pub fn new_value_wrapper(a: GcStruct, b: ValueStruct) -> ValueWrapper { + ValueWrapper(a, b) + } + "#, + ); + + let a = -3.14f32; + let b = 6.18f32; + let gc_struct: StructRef = invoke_fn!(driver.runtime_mut(), "new_gc_struct", a, b).unwrap(); + let value_struct: StructRef = + invoke_fn!(driver.runtime_mut(), "new_value_struct", a, b).unwrap(); + + let gc_wrapper: StructRef = invoke_fn!( + driver.runtime_mut(), + "new_gc_wrapper", + gc_struct.clone(), + value_struct.clone() + ) + .unwrap(); + + let value_wrapper: StructRef = invoke_fn!( + driver.runtime_mut(), + "new_value_wrapper", + gc_struct.clone(), + value_struct.clone() + ) + .unwrap(); + + // Tests mapping of `gc -> gc`, `value -> value` + driver.update( + r#" + struct(gc) GcStruct(f64, f64); + struct(value) ValueStruct(f64, f64); + + struct(gc) GcWrapper(GcStruct, ValueStruct) + struct(value) ValueWrapper(GcStruct, ValueStruct); + "#, + ); + + let gc_0 = gc_wrapper.get::("0").unwrap(); + assert_eq!(gc_0.get::("0"), Ok(a.into())); + assert_eq!(gc_0.get::("1"), Ok(b.into())); + + let gc_1 = gc_wrapper.get::("1").unwrap(); + assert_eq!(gc_1.get::("0"), Ok(a.into())); + assert_eq!(gc_1.get::("1"), Ok(b.into())); + + let value_0 = value_wrapper.get::("0").unwrap(); + assert_eq!(value_0.get::("0"), Ok(a.into())); + assert_eq!(value_0.get::("1"), Ok(b.into())); + + let value_1 = value_wrapper.get::("1").unwrap(); + assert_eq!(value_1.get::("0"), Ok(a.into())); + assert_eq!(value_1.get::("1"), Ok(b.into())); + + // Tests an identity mapping + driver.update( + r#" + struct(gc) GcStruct(f64, f64); + struct(value) ValueStruct(f64, f64); + + struct(gc) GcWrapper(GcStruct, ValueStruct) + struct(value) ValueWrapper(GcStruct, ValueStruct); + "#, + ); + + let gc_0 = gc_wrapper.get::("0").unwrap(); + assert_eq!(gc_0.get::("0"), Ok(a.into())); + assert_eq!(gc_0.get::("1"), Ok(b.into())); + + let gc_1 = gc_wrapper.get::("1").unwrap(); + assert_eq!(gc_1.get::("0"), Ok(a.into())); + assert_eq!(gc_1.get::("1"), Ok(b.into())); + + let value_0 = value_wrapper.get::("0").unwrap(); + assert_eq!(value_0.get::("0"), Ok(a.into())); + assert_eq!(value_0.get::("1"), Ok(b.into())); + + let value_1 = value_wrapper.get::("1").unwrap(); + assert_eq!(value_1.get::("0"), Ok(a.into())); + assert_eq!(value_1.get::("1"), Ok(b.into())); + + // Tests mapping of `gc -> value`, `value -> gc` + driver.update( + r#" + struct(value) GcStruct(f64, f64); + struct(gc) ValueStruct(f64, f64); + + struct(gc) GcWrapper(GcStruct, ValueStruct) + struct(value) ValueWrapper(GcStruct, ValueStruct); + "#, + ); + + assert_eq!(gc_0.get::("0"), Ok(a.into())); + assert_eq!(gc_0.get::("1"), Ok(b.into())); + + assert_eq!(gc_1.get::("0"), Ok(a.into())); + assert_eq!(gc_1.get::("1"), Ok(b.into())); + + assert_eq!(value_0.get::("0"), Ok(a.into())); + assert_eq!(value_0.get::("1"), Ok(b.into())); + + assert_eq!(value_1.get::("0"), Ok(a.into())); + assert_eq!(value_1.get::("1"), Ok(b.into())); + + // Tests mapping of different struct type, when `gc -> value`, `value -> gc`, and + // retention of an old library (due to removal of `GcStruct` and `ValueStruct`) + driver.update( + r#" + struct(gc) GcStruct2(f64); + struct(value) ValueStruct2(f64); + + struct(gc) GcWrapper(GcStruct2, ValueStruct2) + struct(value) ValueWrapper(GcStruct2, ValueStruct2); + "#, + ); + + // Existing, rooted objects should remain untouched + assert_eq!(gc_0.get::("0"), Ok(a.into())); + assert_eq!(gc_0.get::("1"), Ok(b.into())); + + assert_eq!(gc_1.get::("0"), Ok(a.into())); + assert_eq!(gc_1.get::("1"), Ok(b.into())); + + assert_eq!(value_0.get::("0"), Ok(a.into())); + assert_eq!(value_0.get::("1"), Ok(b.into())); + + assert_eq!(value_1.get::("0"), Ok(a.into())); + assert_eq!(value_1.get::("1"), Ok(b.into())); + + // The values in the wrappers should have been updated + let mut gc_0 = gc_wrapper.get::("0").unwrap(); + assert_eq!(gc_0.get::("0"), Ok(0.0)); + gc_0.set::("0", a.into()).unwrap(); + + let mut gc_1 = gc_wrapper.get::("1").unwrap(); + assert_eq!(gc_1.get::("0"), Ok(0.0)); + gc_1.set::("0", a.into()).unwrap(); + + let mut value_0 = value_wrapper.get::("0").unwrap(); + assert_eq!(value_0.get::("0"), Ok(0.0)); + value_0.set::("0", a.into()).unwrap(); + + let mut value_1 = value_wrapper.get::("1").unwrap(); + assert_eq!(value_1.get::("0"), Ok(0.0)); + value_1.set::("0", a.into()).unwrap(); + + // Tests mapping of different struct type, when `gc -> gc`, `value -> value` + driver.update( + r#" + struct(gc) GcStruct(f64, f64); + struct(value) ValueStruct(f64, f64); + + struct(gc) GcWrapper(GcStruct, ValueStruct) + struct(value) ValueWrapper(GcStruct, ValueStruct); + "#, + ); + + // Existing, rooted objects should remain untouched + assert_eq!(gc_0.get::("0"), Ok(a.into())); + assert_eq!(gc_1.get::("0"), Ok(a.into())); + assert_eq!(value_0.get::("0"), Ok(a.into())); + assert_eq!(value_1.get::("0"), Ok(a.into())); + + // The values in the wrappers should have been updated + let gc_0 = gc_wrapper.get::("0").unwrap(); + assert_eq!(gc_0.get::("0"), Ok(0.0)); + assert_eq!(gc_0.get::("1"), Ok(0.0)); + + let gc_1 = gc_wrapper.get::("1").unwrap(); + assert_eq!(gc_1.get::("0"), Ok(0.0)); + assert_eq!(gc_1.get::("1"), Ok(0.0)); + + let value_0 = value_wrapper.get::("0").unwrap(); + assert_eq!(value_0.get::("0"), Ok(0.0)); + assert_eq!(value_0.get::("1"), Ok(0.0)); + + let value_1 = value_wrapper.get::("1").unwrap(); + assert_eq!(value_1.get::("0"), Ok(0.0)); + assert_eq!(value_1.get::("1"), Ok(0.0)); +}