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_value` always returns a `Cow::Owned` variant because the decoding process always creates a new value.
lambdaclass · Mar 15, 2024 · cfcab8c · cfcab8c
1 parent 8ab4471
commit cfcab8c
Show file tree

Hide file tree

Showing 14 changed files with 248 additions and 207 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,9 +2,14 @@
 
 #### Upcoming Changes
 
+* perf: use a more compact representation for `MemoryCell` [#1669](https://github.com/lambdaclass/cairo-vm/pull/1669)
+    * 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
diff --git 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
@@ -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
@@ -105,26 +105,48 @@ from_num!(i32, i64);
 // TODO: move to upstream?
 impl From<i64> 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 }
     }
 }
 
 // TODO: move to upstream?
 impl From<i128> 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<Item = u64> {
         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
@@ -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
@@ -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::<Felt252>::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
@@ -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(()));
     }