diff --git a/CHANGELOG.md b/CHANGELOG.md index de8cf7b9d0..4e65b6eff3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,14 @@ #### Upcoming Changes +* perf: use a more compact representation for `MemoryCell` [#xxxx](https://github.com/lambdaclass/cairo-vm/pull/xxxx) + * Required updating `lambdaworks-math` to version 0.5.0 for missing features + * Required adding methods `from_raw` and `raw` to `cairo-felt` for efficient conversions + * BREAKING: `Memory::get_value` will now always return `Cow::Owned` variants, code that relied on `Cow::Borrowed` may break + #### [0.9.2] - 2024-01-3 -* Change ec_op_impl() to use ProjectivePoint [#1534](https://github.com/lambdaclass/cairo-vm/pull/1534) +* perf: change ec_op_impl() to use ProjectivePoint [#1534](https://github.com/lambdaclass/cairo-vm/pull/1534) #### [0.9.1] - 2023-11-16 diff --git a/Cargo.lock b/Cargo.lock index e213786e36..aaa29fb0cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -194,15 +194,6 @@ version = "1.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9b34d609dfbaf33d6889b2b7106d3ca345eacad44200913df5ba02bfd31d2ba9" -[[package]] -name = "atomic-polyfill" -version = "0.1.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e3ff7eb3f316534d83a8a2c3d1674ace8a5a71198eba31e2e2b597833f699b28" -dependencies = [ - "critical-section", -] - [[package]] name = "autocfg" version = "1.1.0" @@ -278,12 +269,6 @@ version = "1.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c3ac9f8b63eca6fd385229b3675f6cc0dc5c8a5c8a54a59d4f52ffd670d87b0c" -[[package]] -name = "byteorder" -version = "1.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1fd0f2584146f6f2ef48085050886acf353beff7305ebd1ae69500e27c67f64b" - [[package]] name = "cairo-felt" version = "0.8.7" @@ -981,12 +966,6 @@ dependencies = [ "itertools 0.10.5", ] -[[package]] -name = "critical-section" -version = "1.1.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7059fff8937831a9ae6f0fe4d658ffabf58f2ca96aa9dec1c889f936f705f216" - [[package]] name = "crossbeam-deque" version = "0.8.3" @@ -1324,15 +1303,6 @@ version = "1.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "eabb4a44450da02c90444cf74558da904edde8fb4e9035a9a6a4e15445af0bd7" -[[package]] -name = "hash32" -version = "0.2.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b0c35f58762feb77d74ebe43bdbc3210f09be9fe6742234d573bacc26ed92b67" -dependencies = [ - "byteorder", -] - [[package]] name = "hashbrown" version = "0.12.3" @@ -1353,19 +1323,6 @@ dependencies = [ "serde", ] -[[package]] -name = "heapless" -version = "0.7.16" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "db04bc24a18b9ea980628ecf00e6c0264f3c1426dac36c00cb49b6fbad8b0743" -dependencies = [ - "atomic-polyfill", - "hash32", - "rustc_version", - "spin 0.9.8", - "stable_deref_trait", -] - [[package]] name = "heck" version = "0.3.3" @@ -1564,13 +1521,9 @@ dependencies = [ [[package]] name = "lambdaworks-math" -version = "0.1.3" +version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "66ebb7299e567bbc393b50eef9de8db7728605567b7e5cc31634e34b4c8875ba" -dependencies = [ - "heapless", - "rand", -] +checksum = "9ee7dcab3968c71896b8ee4dc829147acc918cffe897af6265b1894527fe3add" [[package]] name = "lazy_static" @@ -1578,7 +1531,7 @@ version = "1.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" dependencies = [ - "spin 0.5.2", + "spin", ] [[package]] @@ -2449,15 +2402,6 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6e63cff320ae2c57904679ba7cb63280a3dc4613885beafb148ee7bf9aa9042d" -[[package]] -name = "spin" -version = "0.9.8" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6980e8d7511241f8acf4aebddbb1ff938df5eebe98691418c4468d0b72a96a67" -dependencies = [ - "lock_api", -] - [[package]] name = "sprs" version = "0.7.1" @@ -2469,12 +2413,6 @@ dependencies = [ "num-traits 0.1.43", ] -[[package]] -name = "stable_deref_trait" -version = "1.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a8f112729512f8e442d81f95a8a7ddf2b7c6b8a1a6f509a95864142b30cab2d3" - [[package]] name = "starknet-crypto" version = "0.6.1" diff --git a/felt/Cargo.toml b/felt/Cargo.toml index d772df2868..52df66b5a6 100644 --- a/felt/Cargo.toml +++ b/felt/Cargo.toml @@ -23,7 +23,7 @@ lazy_static = { version = "1.4.0", default-features = false, features = [ "spin_no_std", ] } serde = { version = "1.0", features = ["derive"], default-features = false } -lambdaworks-math = { version = "0.1.2", default-features = false, optional = true } +lambdaworks-math = { version = "0.5.0", default-features = false, optional = true } arbitrary = { version = "1.3.0", features = ["derive"], optional = true } proptest = { version = "1.2.0", optional = true } diff --git a/felt/src/lib_bigint_felt.rs b/felt/src/lib_bigint_felt.rs index 1c9f3e0479..350b8ef8f9 100644 --- a/felt/src/lib_bigint_felt.rs +++ b/felt/src/lib_bigint_felt.rs @@ -119,6 +119,19 @@ impl Felt252 { value.into() } + pub fn from_raw(mut raw: [u64; 4]) -> Self { + raw[0] &= 0xfffffffffffffff; + raw.reverse(); + let slice = unsafe { core::slice::from_raw_parts(raw.as_ptr() as *const u8, 32) }; + Self::from_bytes_le(slice) + } + + pub fn raw(&self) -> [u64; 4] { + let mut raw = self.to_le_digits(); + raw.reverse(); + raw + } + #[deprecated] pub fn modpow(&self, exponent: &Felt252, modulus: &Felt252) -> Self { Self { diff --git a/felt/src/lib_lambdaworks.rs b/felt/src/lib_lambdaworks.rs index 4fd06b38d8..84c6262879 100644 --- a/felt/src/lib_lambdaworks.rs +++ b/felt/src/lib_lambdaworks.rs @@ -105,12 +105,29 @@ from_num!(i32, i64); // TODO: move to upstream? impl From for Felt252 { fn from(value: i64) -> Self { - let value = if !value.is_negative() { - FieldElement::new(UnsignedInteger::from_u64(value as u64)) + if !value.is_negative() { + Self { + value: FieldElement::new(UnsignedInteger::from_u64(value as u64)), + } } else { let abs_minus_one = UnsignedInteger::from_u64(-(value + 1) as u64); - FieldElement::zero() - FieldElement::one() - FieldElement::new(abs_minus_one) - }; + Self::zero() + - Self::one() + - Self { + value: FieldElement::new(abs_minus_one), + } + } + } +} + +impl From<&BigInt> for Felt252 { + fn from(value: &BigInt) -> Self { + let val = value.mod_floor(&CAIRO_PRIME_BIGUINT.to_bigint().expect("cannot fail")); + let mut limbs = [0; 4]; + for (i, l) in (0..4).rev().zip(val.iter_u64_digits()) { + limbs[i] = l; + } + let value = FieldElement::new(UnsignedInteger::from_limbs(limbs)); Self { value } } } @@ -118,13 +135,18 @@ impl From for Felt252 { // TODO: move to upstream? impl From for Felt252 { fn from(value: i128) -> Self { - let value = if !value.is_negative() { - FieldElement::new(UnsignedInteger::from_u128(value as u128)) + if !value.is_negative() { + Self { + value: FieldElement::new(UnsignedInteger::from_u128(value as u128)), + } } else { let abs_minus_one = UnsignedInteger::from_u128(-(value + 1) as u128); - FieldElement::zero() - FieldElement::one() - FieldElement::new(abs_minus_one) - }; - Self { value } + Self::zero() + - Self::one() + - Self { + value: FieldElement::new(abs_minus_one), + } + } } } @@ -213,6 +235,17 @@ impl Felt252 { value.into() } + pub fn from_raw(mut raw: [u64; 4]) -> Self { + raw[0] &= 0xfffffffffffffff; + Self { + value: FieldElement::from_raw(UnsignedInteger::from_limbs(raw)), + } + } + + pub fn raw(&self) -> [u64; 4] { + self.value.to_raw().limbs + } + pub fn iter_u64_digits(&self) -> impl Iterator { self.value.representative().limbs.into_iter().rev() } @@ -701,37 +734,33 @@ impl<'a> Rem<&'a Felt252> for Felt252 { impl Zero for Felt252 { fn zero() -> Self { Self { - value: FieldElement::from_raw(&Stark252PrimeField::ZERO), + value: FieldElement::from_raw(Stark252PrimeField::ZERO), } } fn is_zero(&self) -> bool { - self.value == FieldElement::from_raw(&Stark252PrimeField::ZERO) + self.value == FieldElement::from_raw(Stark252PrimeField::ZERO) } } impl One for Felt252 { fn one() -> Self { - let value = FieldElement::from_raw(&Stark252PrimeField::ONE); + let value = FieldElement::from_raw(Stark252PrimeField::ONE); Self { value } } fn is_one(&self) -> bool { - self.value == FieldElement::from_raw(&Stark252PrimeField::ONE) + self.value == FieldElement::from_raw(Stark252PrimeField::ONE) } } impl Bounded for Felt252 { fn min_value() -> Self { - Self { - value: FieldElement::zero(), - } + Self::zero() } fn max_value() -> Self { - Self { - value: FieldElement::zero() - FieldElement::one(), - } + Self::zero() - Self::one() } } diff --git a/vm/src/hint_processor/builtin_hint_processor/hint_utils.rs b/vm/src/hint_processor/builtin_hint_processor/hint_utils.rs index d8b53f25d7..33e3baaa9d 100644 --- a/vm/src/hint_processor/builtin_hint_processor/hint_utils.rs +++ b/vm/src/hint_processor/builtin_hint_processor/hint_utils.rs @@ -260,7 +260,7 @@ mod tests { assert_matches!( get_integer_from_var_name("value", &vm, &ids_data, &ApTracking::new()), - Ok(Cow::Borrowed(x)) if x == &Felt252::new(1) + Ok(Cow::Owned(x)) if x == Felt252::new(1) ); } diff --git a/vm/src/vm/runners/builtin_runner/ec_op.rs b/vm/src/vm/runners/builtin_runner/ec_op.rs index 212c99492b..4b24de8a6f 100644 --- a/vm/src/vm/runners/builtin_runner/ec_op.rs +++ b/vm/src/vm/runners/builtin_runner/ec_op.rs @@ -1,4 +1,4 @@ -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::{ EcOpInstanceDef, CELLS_PER_EC_OP, INPUT_CELLS_PER_EC_OP, @@ -183,14 +183,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(self.n_input_cells as usize); + let mut input_cells = Vec::::with_capacity(self.n_input_cells as usize); for i in 0..self.n_input_cells 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.clone(), _ => { return Err(RunnerError::Memory(MemoryError::ExpectedInteger( Box::new((instance + i)?), @@ -210,8 +209,8 @@ 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, ) { @@ -222,9 +221,9 @@ impl EcOpBuiltinRunner { }; } 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[0].clone(), input_cells[1].clone()), + (input_cells[2].clone(), input_cells[3].clone()), + &input_cells[4], #[allow(deprecated)] self.ec_op_builtin.scalar_height, )?; diff --git a/vm/src/vm/runners/builtin_runner/mod.rs b/vm/src/vm/runners/builtin_runner/mod.rs index 4b27c80eb4..bbdde6ba3b 100644 --- a/vm/src/vm/runners/builtin_runner/mod.rs +++ b/vm/src/vm/runners/builtin_runner/mod.rs @@ -421,7 +421,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) } } @@ -438,7 +442,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, @@ -572,6 +580,7 @@ mod tests { }, utils::test_utils::*, vm::vm_core::VirtualMachine, + vm::vm_memory::memory::MemoryCell, }; use assert_matches::assert_matches; @@ -1352,7 +1361,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 4ef0c23ade..61a7d5869f 100644 --- a/vm/src/vm/runners/builtin_runner/range_check.rs +++ b/vm/src/vm/runners/builtin_runner/range_check.rs @@ -130,8 +130,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 diff --git a/vm/src/vm/runners/cairo_runner.rs b/vm/src/vm/runners/cairo_runner.rs index b9507b4e75..1fe6b6feba 100644 --- a/vm/src/vm/runners/cairo_runner.rs +++ b/vm/src/vm/runners/cairo_runner.rs @@ -771,13 +771,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); } @@ -3905,11 +3905,11 @@ mod tests { vm.segments.memory.data = vec![ vec![ - Some(MemoryCell::new(Felt252::new(0x8000_8023_8012u64).into())), - Some(MemoryCell::new(Felt252::new(0xBFFF_8000_0620u64).into())), - Some(MemoryCell::new(Felt252::new(0x8FFF_8000_0750u64).into())), + MemoryCell::new(Felt252::new(0x8000_8023_8012u64).into()), + MemoryCell::new(Felt252::new(0xBFFF_8000_0620u64).into()), + MemoryCell::new(Felt252::new(0x8FFF_8000_0750u64).into()), ], - vec![Some(MemoryCell::new((0isize, 0usize).into())); 128 * 1024], + vec![MemoryCell::new((0isize, 0usize).into()); 128 * 1024], ]; cairo_runner @@ -3932,9 +3932,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), 5, true).into()]; assert_matches!( @@ -3968,9 +3968,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, ap: 0, @@ -3990,9 +3990,9 @@ mod tests { let cairo_runner = cairo_runner!(program); let mut vm = vm!(); vm.builtin_runners = vec![RangeCheckBuiltinRunner::new(Some(8), 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, ap: 0, @@ -4058,9 +4058,9 @@ mod tests { let cairo_runner = cairo_runner!(program); let mut vm = vm!(); vm.builtin_runners = vec![RangeCheckBuiltinRunner::new(Some(8), 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, ap: 0, @@ -4555,7 +4555,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); @@ -4585,8 +4585,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); @@ -4619,10 +4619,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 7ddbe29d1f..54c71ec55e 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 3bc833adad..0cd6319c46 100644 --- a/vm/src/vm/vm_core.rs +++ b/vm/src/vm/vm_core.rs @@ -623,12 +623,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, )))); } } @@ -2925,8 +2925,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] @@ -3015,15 +3015,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 @@ -4122,11 +4122,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 diff --git a/vm/src/vm/vm_memory/memory.rs b/vm/src/vm/vm_memory/memory.rs index c178d93cde..68f468ab66 100644 --- a/vm/src/vm/vm_memory/memory.rs +++ b/vm/src/vm/vm_memory/memory.rs @@ -16,28 +16,85 @@ 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()) } +} + +impl From for MemoryCell { + fn from(value: MaybeRelocatable) -> Self { + match value { + MaybeRelocatable::Int(x) => Self(x.raw()), + MaybeRelocatable::RelocatableValue(x) => Self([ + Self::RELOCATABLE_MASK, + 0, + u64::from_ne_bytes(x.segment_index.to_ne_bytes()), + x.offset as u64, + ]), + } + } +} - pub fn get_value_mut(&mut self) -> &mut MaybeRelocatable { - &mut self.0 +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(( + isize::from_ne_bytes(cell.0[2].to_ne_bytes()), + cell.0[3] as usize, + )), + _ => Self::Int(Felt252::from_raw(cell.0)), + } } } @@ -92,8 +149,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, @@ -104,8 +161,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), @@ -144,18 +201,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, )))); } @@ -177,7 +234,8 @@ impl Memory { &self.data }; let (i, j) = from_relocatable_to_indexes(relocatable); - Some(self.relocate_value(data.get(i)?.get(j)?.as_ref()?.get_value())) + let value = data.get(i)?.get(j)?.get_value()?; + Some(Cow::Owned(self.relocate_value(&value).into_owned())) } // Version of Memory.relocate_value() that doesn't require a self reference @@ -204,11 +262,16 @@ 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 +287,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)?; } addr = (addr + 1)?; } @@ -489,7 +552,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() } } @@ -502,13 +565,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(), ) } @@ -518,9 +575,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)) } } } @@ -532,17 +589,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}")?; } } @@ -640,9 +695,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(), @@ -660,7 +715,7 @@ mod memory_tests { memory.insert(key, &val).unwrap(); assert_eq!( memory.temp_data[0][3], - Some(MemoryCell::new(MaybeRelocatable::from(Felt252::new(8)))) + MemoryCell::new(MaybeRelocatable::from(Felt252::new(8))) ); } @@ -683,7 +738,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(( @@ -1512,9 +1570,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] @@ -1553,16 +1611,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 984b4c0074..2b0ceb6a30 100644 --- a/vm/src/vm/vm_memory/memory_segments.rs +++ b/vm/src/vm/vm_memory/memory_segments.rs @@ -516,9 +516,9 @@ mod tests { assert_eq!( segments.memory.data[1], vec![ - Some(MemoryCell::new(mayberelocatable!(11))), - Some(MemoryCell::new(mayberelocatable!(12))), - Some(MemoryCell::new(mayberelocatable!(1))), + MemoryCell::new(mayberelocatable!(11)), + MemoryCell::new(mayberelocatable!(12)), + MemoryCell::new(mayberelocatable!(1)), ] ); } @@ -543,9 +543,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))), ] ); }