diff --git a/CHANGELOG.md b/CHANGELOG.md index 21149846b6..2493219658 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,12 @@ #### Upcoming Changes +* perf: use a more compact representation for `MemoryCell` [#1672](https://github.com/lambdaclass/cairo-vm/pull/1672) + * BREAKING: `Memory::get_value` will now always return `Cow::Owned` variants, code that relied on `Cow::Borrowed` may break + #### [1.0.0-rc2] - 2024-05-02 + * `cairo1-run` CLI: Allow loading arguments from file[#1739](https://github.com/lambdaclass/cairo-vm/pull/1739) * BREAKING: Remove unused `CairoRunner` field `original_steps`[#1742](https://github.com/lambdaclass/cairo-vm/pull/1742) diff --git a/vm/src/vm/runners/builtin_runner/ec_op.rs b/vm/src/vm/runners/builtin_runner/ec_op.rs index 05f88fad45..5f07e819ad 100644 --- a/vm/src/vm/runners/builtin_runner/ec_op.rs +++ b/vm/src/vm/runners/builtin_runner/ec_op.rs @@ -1,5 +1,5 @@ use crate::air_private_input::{PrivateInput, PrivateInputEcOp}; -use crate::stdlib::{borrow::Cow, prelude::*}; +use crate::stdlib::prelude::*; use crate::stdlib::{cell::RefCell, collections::HashMap}; use crate::types::instance_definitions::ec_op_instance_def::{ CELLS_PER_EC_OP, INPUT_CELLS_PER_EC_OP, SCALAR_HEIGHT, @@ -122,14 +122,13 @@ impl EcOpBuiltinRunner { //All input cells should be filled, and be integer values //If an input cell is not filled, return None - let mut input_cells = Vec::<&Felt252>::with_capacity(INPUT_CELLS_PER_EC_OP as usize); + let mut input_cells = Vec::::with_capacity(INPUT_CELLS_PER_EC_OP as usize); for i in 0..INPUT_CELLS_PER_EC_OP as usize { match memory.get(&(instance + i)?) { None => return Ok(None), Some(addr) => { - input_cells.push(match addr { - // Only relocatable values can be owned - Cow::Borrowed(MaybeRelocatable::Int(ref num)) => num, + input_cells.push(match addr.as_ref() { + MaybeRelocatable::Int(num) => *num, _ => { return Err(RunnerError::Memory(MemoryError::ExpectedInteger( Box::new((instance + i)?), @@ -149,21 +148,21 @@ impl EcOpBuiltinRunner { // Assert that if the current address is part of a point, the point is on the curve for pair in &EC_POINT_INDICES[0..2] { if !EcOpBuiltinRunner::point_on_curve( - input_cells[pair.0], - input_cells[pair.1], + &input_cells[pair.0], + &input_cells[pair.1], &alpha, &beta, ) { return Err(RunnerError::PointNotOnCurve(Box::new(( - *input_cells[pair.0], - *input_cells[pair.1], + input_cells[pair.0], + input_cells[pair.1], )))); }; } let result = EcOpBuiltinRunner::ec_op_impl( (input_cells[0].to_owned(), input_cells[1].to_owned()), (input_cells[2].to_owned(), input_cells[3].to_owned()), - input_cells[4], + &input_cells[4], SCALAR_HEIGHT, )?; self.cache.borrow_mut().insert(x_addr, result.0); diff --git a/vm/src/vm/runners/builtin_runner/mod.rs b/vm/src/vm/runners/builtin_runner/mod.rs index 886ed00578..964246b1fd 100644 --- a/vm/src/vm/runners/builtin_runner/mod.rs +++ b/vm/src/vm/runners/builtin_runner/mod.rs @@ -446,7 +446,11 @@ impl BuiltinRunner { for i in 0..n { for j in 0..n_input_cells { let offset = cells_per_instance * i + j; - if let None | Some(None) = builtin_segment.get(offset) { + if builtin_segment + .get(offset) + .filter(|x| x.is_some()) + .is_none() + { missing_offsets.push(offset) } } @@ -463,7 +467,11 @@ impl BuiltinRunner { for i in 0..n { for j in n_input_cells..cells_per_instance { let offset = cells_per_instance * i + j; - if let None | Some(None) = builtin_segment.get(offset) { + if builtin_segment + .get(offset) + .filter(|x| x.is_some()) + .is_none() + { vm.verify_auto_deductions_for_addr( Relocatable::from((builtin_segment_index as isize, offset)), self, @@ -651,6 +659,7 @@ mod tests { use crate::types::program::Program; use crate::vm::errors::memory_errors::InsufficientAllocatedCellsError; use crate::vm::runners::cairo_runner::CairoRunner; + use crate::vm::vm_memory::memory::MemoryCell; use crate::{utils::test_utils::*, vm::vm_core::VirtualMachine}; use assert_matches::assert_matches; @@ -1321,7 +1330,7 @@ mod tests { let mut vm = vm!(); - vm.segments.memory.data = vec![vec![None, None, None]]; + vm.segments.memory.data = vec![vec![MemoryCell::NONE, MemoryCell::NONE, MemoryCell::NONE]]; assert_matches!(builtin.run_security_checks(&vm), Ok(())); } diff --git a/vm/src/vm/runners/builtin_runner/range_check.rs b/vm/src/vm/runners/builtin_runner/range_check.rs index f5c4991a74..e9749e8b8a 100644 --- a/vm/src/vm/runners/builtin_runner/range_check.rs +++ b/vm/src/vm/runners/builtin_runner/range_check.rs @@ -122,8 +122,7 @@ impl RangeCheckBuiltinRunner { // Split value into n_parts parts of less than _INNER_RC_BOUND size. for value in range_check_segment { rc_bounds = value - .as_ref()? - .get_value() + .get_value()? .get_int_ref()? .to_le_digits() // TODO: maybe skip leading zeros @@ -151,8 +150,8 @@ impl RangeCheckBuiltinRunner { pub fn air_private_input(&self, memory: &Memory) -> Vec { let mut private_inputs = vec![]; if let Some(segment) = memory.data.get(self.base) { - for (index, val) in segment.iter().enumerate() { - if let Some(value) = val.as_ref().and_then(|cell| cell.get_value().get_int()) { + for (index, cell) in segment.iter().enumerate() { + if let Some(value) = cell.get_value().and_then(|value| value.get_int()) { private_inputs.push(PrivateInput::Value(PrivateInputValue { index, value })) } } diff --git a/vm/src/vm/runners/cairo_runner.rs b/vm/src/vm/runners/cairo_runner.rs index 0fd7603b14..6e622002bf 100644 --- a/vm/src/vm/runners/cairo_runner.rs +++ b/vm/src/vm/runners/cairo_runner.rs @@ -944,13 +944,13 @@ impl CairoRunner { self.relocated_memory.push(None); for (index, segment) in vm.segments.memory.data.iter().enumerate() { for (seg_offset, cell) in segment.iter().enumerate() { - match cell { + match cell.get_value() { Some(cell) => { let relocated_addr = relocate_address( Relocatable::from((index as isize, seg_offset)), relocation_table, )?; - let value = relocate_value(cell.get_value().clone(), relocation_table)?; + let value = relocate_value(cell, relocation_table)?; if self.relocated_memory.len() <= relocated_addr { self.relocated_memory.resize(relocated_addr + 1, None); } @@ -4098,11 +4098,11 @@ mod tests { vm.segments.memory.data = vec![ vec![ - Some(MemoryCell::new(Felt252::from(0x8000_8023_8012u64).into())), - Some(MemoryCell::new(Felt252::from(0xBFFF_8000_0620u64).into())), - Some(MemoryCell::new(Felt252::from(0x8FFF_8000_0750u64).into())), + MemoryCell::new(Felt252::from(0x8000_8023_8012u64).into()), + MemoryCell::new(Felt252::from(0xBFFF_8000_0620u64).into()), + MemoryCell::new(Felt252::from(0x8FFF_8000_0750u64).into()), ], - vec![Some(MemoryCell::new((0isize, 0usize).into())); 128 * 1024], + vec![MemoryCell::new((0isize, 0usize).into()); 128 * 1024], ]; cairo_runner @@ -4125,9 +4125,9 @@ mod tests { let cairo_runner = cairo_runner!(program); let mut vm = vm!(); - vm.segments.memory.data = vec![vec![Some(MemoryCell::new(mayberelocatable!( + vm.segments.memory.data = vec![vec![MemoryCell::new(mayberelocatable!( 0x80FF_8000_0530u64 - )))]]; + ))]]; vm.builtin_runners = vec![RangeCheckBuiltinRunner::::new(Some(12), true).into()]; @@ -4162,9 +4162,9 @@ mod tests { let mut vm = vm!(); vm.builtin_runners = vec![]; vm.current_step = 10000; - vm.segments.memory.data = vec![vec![Some(MemoryCell::new(mayberelocatable!( + vm.segments.memory.data = vec![vec![MemoryCell::new(mayberelocatable!( 0x80FF_8000_0530u64 - )))]]; + ))]]; vm.trace = Some(vec![TraceEntry { pc: (0, 0).into(), ap: 0, @@ -4185,9 +4185,9 @@ mod tests { let mut vm = vm!(); vm.builtin_runners = vec![RangeCheckBuiltinRunner::::new(Some(8), true).into()]; - vm.segments.memory.data = vec![vec![Some(MemoryCell::new(mayberelocatable!( + vm.segments.memory.data = vec![vec![MemoryCell::new(mayberelocatable!( 0x80FF_8000_0530u64 - )))]]; + ))]]; vm.trace = Some(vec![TraceEntry { pc: (0, 0).into(), ap: 0, @@ -4253,9 +4253,9 @@ mod tests { let mut vm = vm!(); vm.builtin_runners = vec![RangeCheckBuiltinRunner::::new(Some(8), true).into()]; - vm.segments.memory.data = vec![vec![Some(MemoryCell::new(mayberelocatable!( + vm.segments.memory.data = vec![vec![MemoryCell::new(mayberelocatable!( 0x80FF_8000_0530u64 - )))]]; + ))]]; vm.trace = Some(vec![TraceEntry { pc: (0, 0).into(), ap: 0, @@ -4750,7 +4750,7 @@ mod tests { vm.builtin_runners.push(output_builtin.into()); vm.segments.memory.data = vec![ vec![], - vec![Some(MemoryCell::new(MaybeRelocatable::from((0, 0))))], + vec![MemoryCell::new(MaybeRelocatable::from((0, 0)))], vec![], ]; vm.set_ap(1); @@ -4780,8 +4780,8 @@ mod tests { let output_builtin = OutputBuiltinRunner::new(true); vm.builtin_runners.push(output_builtin.into()); vm.segments.memory.data = vec![ - vec![Some(MemoryCell::new(MaybeRelocatable::from((0, 0))))], - vec![Some(MemoryCell::new(MaybeRelocatable::from((0, 1))))], + vec![MemoryCell::new(MaybeRelocatable::from((0, 0)))], + vec![MemoryCell::new(MaybeRelocatable::from((0, 1)))], vec![], ]; vm.set_ap(1); @@ -4814,10 +4814,10 @@ mod tests { vm.builtin_runners.push(bitwise_builtin.into()); cairo_runner.initialize_segments(&mut vm, None); vm.segments.memory.data = vec![ - vec![Some(MemoryCell::new(MaybeRelocatable::from((0, 0))))], + vec![MemoryCell::new(MaybeRelocatable::from((0, 0)))], vec![ - Some(MemoryCell::new(MaybeRelocatable::from((2, 0)))), - Some(MemoryCell::new(MaybeRelocatable::from((3, 5)))), + MemoryCell::new(MaybeRelocatable::from((2, 0))), + MemoryCell::new(MaybeRelocatable::from((3, 5))), ], vec![], ]; diff --git a/vm/src/vm/security.rs b/vm/src/vm/security.rs index b13cf33613..0fd1b3c566 100644 --- a/vm/src/vm/security.rs +++ b/vm/src/vm/security.rs @@ -65,10 +65,10 @@ pub fn verify_secure_runner( // Asumption: If temporary memory is empty, this means no temporary memory addresses were generated and all addresses in memory are real if !vm.segments.memory.temp_data.is_empty() { for value in vm.segments.memory.data.iter().flatten() { - match value.as_ref().map(|x| x.get_value()) { + match value.get_value() { Some(MaybeRelocatable::RelocatableValue(addr)) if addr.segment_index < 0 => { return Err(VirtualMachineError::InvalidMemoryValueTemporaryAddress( - Box::new(*addr), + Box::new(addr), )) } _ => {} diff --git a/vm/src/vm/vm_core.rs b/vm/src/vm/vm_core.rs index 4fcf978b6a..d22a1a8aeb 100644 --- a/vm/src/vm/vm_core.rs +++ b/vm/src/vm/vm_core.rs @@ -695,12 +695,12 @@ impl VirtualMachine { ) .map_err(VirtualMachineError::RunnerError)? { - let value = value.as_ref().map(|x| x.get_value()); - if Some(&deduced_memory_cell) != value && value.is_some() { + let value = value.get_value(); + if Some(&deduced_memory_cell) != value.as_ref() && value.is_some() { return Err(VirtualMachineError::InconsistentAutoDeduction(Box::new(( builtin.name(), deduced_memory_cell, - value.cloned(), + value, )))); } } @@ -3039,8 +3039,8 @@ mod tests { //Check that the following addresses have been accessed: // Addresses have been copied from python execution: let mem = vm.segments.memory.data; - assert!(mem[1][0].as_ref().unwrap().is_accessed()); - assert!(mem[1][1].as_ref().unwrap().is_accessed()); + assert!(mem[1][0].is_accessed()); + assert!(mem[1][1].is_accessed()); } #[test] @@ -3137,15 +3137,15 @@ mod tests { //Check that the following addresses have been accessed: // Addresses have been copied from python execution: let mem = &vm.segments.memory.data; - assert!(mem[0][1].as_ref().unwrap().is_accessed()); - assert!(mem[0][4].as_ref().unwrap().is_accessed()); - assert!(mem[0][6].as_ref().unwrap().is_accessed()); - assert!(mem[1][0].as_ref().unwrap().is_accessed()); - assert!(mem[1][1].as_ref().unwrap().is_accessed()); - assert!(mem[1][2].as_ref().unwrap().is_accessed()); - assert!(mem[1][3].as_ref().unwrap().is_accessed()); - assert!(mem[1][4].as_ref().unwrap().is_accessed()); - assert!(mem[1][5].as_ref().unwrap().is_accessed()); + assert!(mem[0][1].is_accessed()); + assert!(mem[0][4].is_accessed()); + assert!(mem[0][6].is_accessed()); + assert!(mem[1][0].is_accessed()); + assert!(mem[1][1].is_accessed()); + assert!(mem[1][2].is_accessed()); + assert!(mem[1][3].is_accessed()); + assert!(mem[1][4].is_accessed()); + assert!(mem[1][5].is_accessed()); assert_eq!( vm.segments .memory @@ -4273,11 +4273,11 @@ mod tests { //Check that the following addresses have been accessed: // Addresses have been copied from python execution: let mem = &vm.segments.memory.data; - assert!(mem[0][0].as_ref().unwrap().is_accessed()); - assert!(mem[0][1].as_ref().unwrap().is_accessed()); - assert!(mem[0][2].as_ref().unwrap().is_accessed()); - assert!(mem[0][10].as_ref().unwrap().is_accessed()); - assert!(mem[1][1].as_ref().unwrap().is_accessed()); + assert!(mem[0][0].is_accessed()); + assert!(mem[0][1].is_accessed()); + assert!(mem[0][2].is_accessed()); + assert!(mem[0][10].is_accessed()); + assert!(mem[1][1].is_accessed()); assert_eq!( vm.segments .memory @@ -4499,8 +4499,8 @@ mod tests { //Check that the following addresses have been accessed: // Addresses have been copied from python execution: let mem = vm.segments.memory.data; - assert!(mem[1][0].as_ref().unwrap().is_accessed()); - assert!(mem[1][1].as_ref().unwrap().is_accessed()); + assert!(mem[1][0].is_accessed()); + assert!(mem[1][1].is_accessed()); } #[test] @@ -4600,15 +4600,15 @@ mod tests { //Check that the following addresses have been accessed: // Addresses have been copied from python execution: let mem = &vm.segments.memory.data; - assert!(mem[4][1].as_ref().unwrap().is_accessed()); - assert!(mem[4][4].as_ref().unwrap().is_accessed()); - assert!(mem[4][6].as_ref().unwrap().is_accessed()); - assert!(mem[1][0].as_ref().unwrap().is_accessed()); - assert!(mem[1][1].as_ref().unwrap().is_accessed()); - assert!(mem[1][2].as_ref().unwrap().is_accessed()); - assert!(mem[1][3].as_ref().unwrap().is_accessed()); - assert!(mem[1][4].as_ref().unwrap().is_accessed()); - assert!(mem[1][5].as_ref().unwrap().is_accessed()); + assert!(mem[4][1].is_accessed()); + assert!(mem[4][4].is_accessed()); + assert!(mem[4][6].is_accessed()); + assert!(mem[1][0].is_accessed()); + assert!(mem[1][1].is_accessed()); + assert!(mem[1][2].is_accessed()); + assert!(mem[1][3].is_accessed()); + assert!(mem[1][4].is_accessed()); + assert!(mem[1][5].is_accessed()); assert_eq!( vm.segments .memory diff --git a/vm/src/vm/vm_memory/memory.rs b/vm/src/vm/vm_memory/memory.rs index b4b953f5ee..caf1684932 100644 --- a/vm/src/vm/vm_memory/memory.rs +++ b/vm/src/vm/vm_memory/memory.rs @@ -17,28 +17,92 @@ pub struct ValidationRule( pub Box Result, MemoryError>>, ); -#[derive(Clone, Eq, Ord, PartialEq, PartialOrd, Debug)] -pub(crate) struct MemoryCell(MaybeRelocatable, bool); +/// [`MemoryCell`] represents an optimized storage layout for the VM memory. +/// It's specified to have both size an alignment of 32 bytes to optimize cache access. +/// Typical cache sizes are 64 bytes, a few cases might be 128 bytes, meaning 32 bytes aligned to +/// 32 bytes boundaries will never get split into two separate lines, avoiding double stalls and +/// reducing false sharing and evictions. +/// The trade off is extra computation for conversion to our "in-flight" `MaybeRelocatable` and +/// `Felt252` as well as some extra copies. Empirically, this seems to be offset by the improved +/// locality of the bigger structure for Lambdaworks. There is a big hit from the conversions when +/// using the `BigUint` implementation, since those force allocations on the heap, but since that's +/// dropped in later versions anyway it's not a priority. For Lambdaworks the new copies are mostly +/// to the stack, which is typically already in the cache. +/// The layout uses the 4 MSB in the first `u64` as flags: +/// - BIT63: NONE flag, 1 when the cell is actually empty. +/// - BIT62: ACCESS flag, 1 when the cell has been accessed in a way observable to Cairo. +/// - BIT61: RELOCATABLE flag, 1 when the contained value is a `Relocatable`, 0 when it is a +/// `Felt252`. +/// `Felt252` values are stored in big-endian order to keep the flag bits free. +/// `Relocatable` values are stored as native endian, with the 3rd word storing the segment index +/// and the 4th word storing the offset. +#[derive(Copy, Clone, Eq, Ord, PartialEq, PartialOrd, Debug)] +#[repr(align(32))] +pub(crate) struct MemoryCell([u64; 4]); impl MemoryCell { + pub const NONE_MASK: u64 = 1 << 63; + pub const ACCESS_MASK: u64 = 1 << 62; + pub const RELOCATABLE_MASK: u64 = 1 << 61; + pub const NONE: Self = Self([Self::NONE_MASK, 0, 0, 0]); + pub fn new(value: MaybeRelocatable) -> Self { - MemoryCell(value, false) + value.into() + } + + pub fn is_none(&self) -> bool { + self.0[0] & Self::NONE_MASK == Self::NONE_MASK + } + + pub fn is_some(&self) -> bool { + !self.is_none() } pub fn mark_accessed(&mut self) { - self.1 = true + self.0[0] |= Self::ACCESS_MASK; } pub fn is_accessed(&self) -> bool { - self.1 + self.0[0] & Self::ACCESS_MASK == Self::ACCESS_MASK } - pub fn get_value(&self) -> &MaybeRelocatable { - &self.0 + pub fn get_value(&self) -> Option { + self.is_some().then(|| (*self).into()) } +} - pub fn get_value_mut(&mut self) -> &mut MaybeRelocatable { - &mut self.0 +impl From for MemoryCell { + fn from(value: MaybeRelocatable) -> Self { + match value { + MaybeRelocatable::Int(x) => Self(x.to_raw()), + MaybeRelocatable::RelocatableValue(x) => Self([ + Self::RELOCATABLE_MASK, + 0, + // NOTE: hack around signedness + usize::from_ne_bytes(x.segment_index.to_ne_bytes()) as u64, + x.offset as u64, + ]), + } + } +} + +impl From for MaybeRelocatable { + fn from(cell: MemoryCell) -> Self { + debug_assert!(cell.is_some()); + let flags = cell.0[0]; + match flags & MemoryCell::RELOCATABLE_MASK { + MemoryCell::RELOCATABLE_MASK => Self::from(( + // NOTE: hack around signedness + isize::from_ne_bytes((cell.0[2] as usize).to_ne_bytes()), + cell.0[3] as usize, + )), + _ => { + let mut value = cell.0; + // Remove all flag bits + value[0] &= 0x0fffffffffffffff; + Self::Int(Felt252::from_raw(value)) + } + } } } @@ -93,8 +157,8 @@ impl AddressSet { } pub struct Memory { - pub(crate) data: Vec>>, - pub(crate) temp_data: Vec>>, + pub(crate) data: Vec>, + pub(crate) temp_data: Vec>, // relocation_rules's keys map to temp_data's indices and therefore begin at // zero; that is, segment_index = -1 maps to key 0, -2 to key 1... pub(crate) relocation_rules: HashMap, @@ -105,8 +169,8 @@ pub struct Memory { impl Memory { pub fn new() -> Memory { Memory { - data: Vec::>>::new(), - temp_data: Vec::>>::new(), + data: Vec::new(), + temp_data: Vec::new(), relocation_rules: HashMap::new(), validated_addresses: AddressSet::new(), validation_rules: Vec::with_capacity(7), @@ -145,18 +209,18 @@ impl Memory { segment .try_reserve(new_len.saturating_sub(capacity)) .map_err(|_| MemoryError::VecCapacityExceeded)?; - segment.resize(new_len, None); + segment.resize(new_len, MemoryCell::NONE); } // At this point there's *something* in there - match segment[value_offset] { - None => segment[value_offset] = Some(MemoryCell::new(val)), - Some(ref current_cell) => { - if current_cell.get_value() != &val { + match segment[value_offset].get_value() { + None => segment[value_offset] = MemoryCell::new(val), + Some(current_cell) => { + if current_cell != val { //Existing memory cannot be changed return Err(MemoryError::InconsistentMemory(Box::new(( key, - current_cell.get_value().clone(), + current_cell, val, )))); } @@ -178,8 +242,8 @@ impl Memory { &self.data }; let (i, j) = from_relocatable_to_indexes(relocatable); - self.relocate_value(data.get(i)?.get(j)?.as_ref()?.get_value()) - .ok() + let value = data.get(i)?.get(j)?.get_value()?; + Some(Cow::Owned(self.relocate_value(&value).ok()?.into_owned())) } // Version of Memory.relocate_value() that doesn't require a self reference @@ -204,11 +268,18 @@ impl Memory { } // Relocate temporary addresses in memory for segment in self.data.iter_mut().chain(self.temp_data.iter_mut()) { - for cell in segment.iter_mut().flatten() { - let value = cell.get_value_mut(); + for cell in segment.iter_mut() { + let value = cell.get_value(); match value { - MaybeRelocatable::RelocatableValue(addr) if addr.segment_index < 0 => { - *value = Memory::relocate_address(*addr, &self.relocation_rules)?; + Some(MaybeRelocatable::RelocatableValue(addr)) if addr.segment_index < 0 => { + let mut new_cell = MemoryCell::new(Memory::relocate_address( + addr, + &self.relocation_rules, + )?); + if cell.is_accessed() { + new_cell.mark_accessed(); + } + *cell = new_cell; } _ => {} } @@ -224,9 +295,9 @@ impl Memory { s.reserve_exact(data_segment.len()) } for cell in data_segment { - if let Some(cell) = cell { + if let Some(v) = cell.get_value() { // Rely on Memory::insert to catch memory inconsistencies - self.insert(addr, cell.get_value())?; + self.insert(addr, v)?; // If the cell is accessed, mark the relocated one as accessed too if cell.is_accessed() { self.mark_as_accessed(addr) @@ -501,7 +572,7 @@ impl Memory { &mut self.data }; let cell = data.get_mut(i).and_then(|x| x.get_mut(j)); - if let Some(Some(cell)) = cell { + if let Some(cell) = cell { cell.mark_accessed() } } @@ -514,13 +585,7 @@ impl Memory { Some( segment .iter() - .filter(|x| { - if let Some(cell) = x { - cell.is_accessed() - } else { - false - } - }) + .filter(|x| x.is_some() && x.is_accessed()) .count(), ) } @@ -545,9 +610,9 @@ impl From<&Memory> for CairoPieMemory { fn from(mem: &Memory) -> CairoPieMemory { let mut pie_memory = Vec::default(); for (i, segment) in mem.data.iter().enumerate() { - for (j, elem) in segment.iter().enumerate() { - if let Some(cell) = elem { - pie_memory.push(((i, j), cell.get_value().clone())) + for (j, cell) in segment.iter().enumerate() { + if let Some(value) = cell.get_value() { + pie_memory.push(((i, j), value)) } } } @@ -559,17 +624,15 @@ impl fmt::Display for Memory { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { for (i, segment) in self.temp_data.iter().enumerate() { for (j, cell) in segment.iter().enumerate() { - if let Some(cell) = cell { + if let Some(elem) = cell.get_value() { let temp_segment = i + 1; - let elem = cell.get_value(); writeln!(f, "(-{temp_segment},{j}) : {elem}")?; } } } for (i, segment) in self.data.iter().enumerate() { for (j, cell) in segment.iter().enumerate() { - if let Some(cell) = cell { - let elem = cell.get_value(); + if let Some(elem) = cell.get_value() { writeln!(f, "({i},{j}) : {elem}")?; } } @@ -668,9 +731,9 @@ mod memory_tests { fn get_valuef_from_temp_segment() { let mut memory = Memory::new(); memory.temp_data = vec![vec![ - None, - None, - Some(MemoryCell::new(mayberelocatable!(8))), + MemoryCell::NONE, + MemoryCell::NONE, + MemoryCell::new(mayberelocatable!(8)), ]]; assert_eq!( memory.get(&mayberelocatable!(-1, 2)).unwrap().as_ref(), @@ -688,9 +751,7 @@ mod memory_tests { memory.insert(key, &val).unwrap(); assert_eq!( memory.temp_data[0][3], - Some(MemoryCell::new(MaybeRelocatable::from(Felt252::from( - 8_u64 - )))) + MemoryCell::new(MaybeRelocatable::from(Felt252::from(8_u64))) ); } @@ -713,7 +774,10 @@ mod memory_tests { fn insert_and_get_from_temp_segment_failed() { let key = relocatable!(-1, 1); let mut memory = Memory::new(); - memory.temp_data = vec![vec![None, Some(MemoryCell::new(mayberelocatable!(8)))]]; + memory.temp_data = vec![vec![ + MemoryCell::NONE, + MemoryCell::new(mayberelocatable!(8)), + ]]; assert_eq!( memory.insert(key, &mayberelocatable!(5)), Err(MemoryError::InconsistentMemory(Box::new(( @@ -1556,9 +1620,9 @@ mod memory_tests { #[test] fn mark_address_as_accessed() { let mut memory = memory![((0, 0), 0)]; - assert!(!memory.data[0][0].as_ref().unwrap().is_accessed()); + assert!(!memory.data[0][0].is_accessed()); memory.mark_as_accessed(relocatable!(0, 0)); - assert!(memory.data[0][0].as_ref().unwrap().is_accessed()); + assert!(memory.data[0][0].is_accessed()); } #[test] @@ -1597,16 +1661,7 @@ mod memory_tests { #[test] fn memory_cell_get_value() { let cell = MemoryCell::new(mayberelocatable!(1)); - assert_eq!(cell.get_value(), &mayberelocatable!(1)); - } - - #[test] - fn memory_cell_mutate_value() { - let mut cell = MemoryCell::new(mayberelocatable!(1)); - let cell_value = cell.get_value_mut(); - assert_eq!(cell_value, &mayberelocatable!(1)); - *cell_value = mayberelocatable!(2); - assert_eq!(cell.get_value(), &mayberelocatable!(2)); + assert_eq!(cell.get_value(), Some(mayberelocatable!(1))); } use core::cmp::Ordering::*; diff --git a/vm/src/vm/vm_memory/memory_segments.rs b/vm/src/vm/vm_memory/memory_segments.rs index a665508483..7713bad23c 100644 --- a/vm/src/vm/vm_memory/memory_segments.rs +++ b/vm/src/vm/vm_memory/memory_segments.rs @@ -294,8 +294,7 @@ impl MemorySegmentManager { for _ in 0..self.zero_segment_size.saturating_sub(size) { // As zero_segment_index is only accessible to the segment manager // we can asume that it is always valid and index direcly into it - self.memory.data[self.zero_segment_index] - .push(Some(MemoryCell::new(Felt252::ZERO.into()))) + self.memory.data[self.zero_segment_index].push(MemoryCell::new(Felt252::ZERO.into())) } self.zero_segment_size = max(self.zero_segment_size, size); self.zero_segment_index @@ -610,9 +609,9 @@ mod tests { assert_eq!( segments.memory.data[1], vec![ - Some(MemoryCell::new(MaybeRelocatable::from((0, 1)))), - Some(MemoryCell::new(MaybeRelocatable::from((0, 2)))), - Some(MemoryCell::new(MaybeRelocatable::from((0, 3)))), + MemoryCell::new(MaybeRelocatable::from((0, 1))), + MemoryCell::new(MaybeRelocatable::from((0, 2))), + MemoryCell::new(MaybeRelocatable::from((0, 3))), ] ); }