perf: rework MemoryCell for cache efficiency (#1672)

* perf: rework `MemoryCell` for cache efficiency Store data and metadata inline as a single `[u64; 4]` with `32` bytes alignment, fitting a whole number of cells per cache line to reduce evictions and double sharing and to avoid split line access. Besides performance, an observable change is that now `Memory::get` always returns a `Cow::Owned` variant because the decoding process always creates a new value. * fmt --------- Co-authored-by: fmoletta <[email protected]> Co-authored-by: Pedro Fontana <[email protected]> Co-authored-by: Pedro Fontana <[email protected]> Co-authored-by: Federica <[email protected]>
lambdaclass · May 6, 2024 · bc5a14e · bc5a14e
1 parent f3161e3
commit bc5a14e
Show file tree

Hide file tree

Showing 9 changed files with 200 additions and 135 deletions.
diff --git 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
@@ -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::<Felt252>::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
@@ -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
@@ -122,8 +122,7 @@ impl<const N_PARTS: u64> RangeCheckBuiltinRunner<N_PARTS> {
         // 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<const N_PARTS: u64> RangeCheckBuiltinRunner<N_PARTS> {
     pub fn air_private_input(&self, memory: &Memory) -> Vec<PrivateInput> {
         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
@@ -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::<RC_N_PARTS_STANDARD>::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::<RC_N_PARTS_STANDARD>::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::<RC_N_PARTS_STANDARD>::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
@@ -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
@@ -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