From 4221723f1d9d3db1cb8e3fd99cce5771543af399 Mon Sep 17 00:00:00 2001 From: Mario Rugiero Date: Tue, 13 Jun 2023 18:46:30 -0300 Subject: [PATCH] perf: keep min and max referenced offsets during run (#1080) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * WIP * WIP * WIP * WIP * WIP * WIP * WIP * WIP * WIP * WIP * WIP * WIP * WIP * WIP * Fix trace * WIP * Revert "WIP" This reverts commit eae76bb92a33de5dd5f488a08efe18b91846b2dd. * test: remove no longer relevant tests * Fix missing reassignment of cache * Cleanup and correctness * chore: changelog * refactor: don't return Result --------- Co-authored-by: Pedro Fontana Co-authored-by: Tomá <47506558+MegaRedHand@users.noreply.github.com> --- CHANGELOG.md | 3 + vm/src/vm/runners/cairo_runner.rs | 131 ++++++++---------------------- vm/src/vm/trace/mod.rs | 126 ++++------------------------ vm/src/vm/trace/trace_entry.rs | 15 ---- vm/src/vm/vm_core.rs | 22 +++++ 5 files changed, 71 insertions(+), 226 deletions(-) delete mode 100644 vm/src/vm/trace/trace_entry.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index e25570b1c1..6b5a7b71e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ #### Upcoming Changes +* perf: accumulate `min` and `max` instruction offsets during run to speed up range check [#1080](https://github.com/lambdaclass/cairo-rs/pull/) + BREAKING: `Cairo_runner::get_perm_range_check_limits` no longer returns an error when called without trace enabled, as it no longer depends on it + #### [0.5.2] - 2023-6-12 * BREAKING: Compute `ExecutionResources.n_steps` without requiring trace [#1222](https://github.com/lambdaclass/cairo-rs/pull/1222) diff --git a/vm/src/vm/runners/cairo_runner.rs b/vm/src/vm/runners/cairo_runner.rs index e989a9a269..ff029df56f 100644 --- a/vm/src/vm/runners/cairo_runner.rs +++ b/vm/src/vm/runners/cairo_runner.rs @@ -36,7 +36,6 @@ use crate::{ vm_exception::VmException, }, security::verify_secure_runner, - trace::get_perm_range_check_limits, { runners::builtin_runner::{ BitwiseBuiltinRunner, BuiltinRunner, EcOpBuiltinRunner, HashBuiltinRunner, @@ -596,7 +595,6 @@ impl CairoRunner { ) -> Result<(), VirtualMachineError> { let references = self.get_reference_list(); let hint_data_dictionary = self.get_hint_data_dictionary(&references, hint_processor)?; - for remaining_steps in (1..=steps).rev() { if self.final_pc.as_ref() == Some(&vm.run_context.pc) { return Err(VirtualMachineError::EndOfProgram(remaining_steps)); @@ -632,48 +630,29 @@ impl CairoRunner { self.run_until_steps(vm.current_step.next_power_of_two(), vm, hint_processor) } - pub fn get_perm_range_check_limits( - &self, - vm: &VirtualMachine, - ) -> Result, VirtualMachineError> { - let limits = get_perm_range_check_limits( - vm.trace.as_ref().ok_or(VirtualMachineError::TracerError( - TraceError::TraceNotEnabled, - ))?, - &vm.segments.memory, - )?; - - match limits { - Some((mut rc_min, mut rc_max)) => { - for runner in &vm.builtin_runners { - let (runner_min, runner_max) = - match runner.get_range_check_usage(&vm.segments.memory) { - Some(x) => x, - None => continue, - }; - - rc_min = rc_min.min(runner_min as isize); - rc_max = rc_max.max(runner_max as isize); - } - - Ok(Some((rc_min, rc_max))) - } - None => Ok(None), - } + pub fn get_perm_range_check_limits(&self, vm: &VirtualMachine) -> Option<(isize, isize)> { + let runner_usages = vm + .builtin_runners + .iter() + .filter_map(|runner| runner.get_range_check_usage(&vm.segments.memory)) + .map(|(rc_min, rc_max)| (rc_min as isize, rc_max as isize)); + let rc_bounds = vm.rc_limits.iter().copied().chain(runner_usages); + rc_bounds.reduce(|(min1, max1), (min2, max2)| (min1.min(min2), max1.max(max2))) } /// Checks that there are enough trace cells to fill the entire range check /// range. pub fn check_range_check_usage(&self, vm: &VirtualMachine) -> Result<(), VirtualMachineError> { - let (rc_min, rc_max) = match self.get_perm_range_check_limits(vm)? { - Some(x) => x, - None => return Ok(()), + let Some((rc_min, rc_max)) = self.get_perm_range_check_limits(vm) else { + return Ok(()); }; - let mut rc_units_used_by_builtins = 0; - for builtin_runner in &vm.builtin_runners { - rc_units_used_by_builtins += builtin_runner.get_used_perm_range_check_units(vm)?; - } + let rc_units_used_by_builtins: usize = vm + .builtin_runners + .iter() + .map(|runner| runner.get_used_perm_range_check_units(vm)) + .sum::>() + .map_err(Into::::into)?; let unused_rc_units = (self.layout.rc_units as usize - 3) * vm.current_step - rc_units_used_by_builtins; @@ -3737,74 +3716,33 @@ mod tests { ); } - /// Test that ensures get_perm_range_check_limits() returns an error when - /// trace is not enabled. - #[test] - #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] - fn get_perm_range_check_limits_trace_not_enabled() { - let program = program!(); - - let cairo_runner = cairo_runner!(program); - let vm = vm!(); - - assert_matches!( - cairo_runner.get_perm_range_check_limits(&vm), - Err(VirtualMachineError::TracerError( - TraceError::TraceNotEnabled - )) - ); - } - - /// Test that ensures get_perm_range_check_limits() returns None when the - /// trace is empty (get_perm_range_check_limits returns None). - #[test] - #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] - fn get_perm_range_check_limits_empty() { - let program = program!(); - - let cairo_runner = cairo_runner!(program); - let mut vm = vm!(); - vm.trace = Some(vec![]); - - assert_matches!(cairo_runner.get_perm_range_check_limits(&vm), Ok(None)); - } - /// Test that get_perm_range_check_limits() works correctly when there are /// no builtins. #[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn get_perm_range_check_limits_no_builtins() { let program = program!(); + let mut hint_processor = BuiltinHintProcessor::new(HashMap::new()); - let cairo_runner = cairo_runner!(program); + let mut cairo_runner = cairo_runner!(program); let mut vm = vm!(); - vm.trace = Some(vec![ - TraceEntry { - pc: 0, - ap: 0, - fp: 0, - }, - TraceEntry { - pc: 1, - ap: 0, - fp: 0, - }, - TraceEntry { - pc: 2, - ap: 0, - fp: 0, - }, - ]); - vm.segments.memory.data = vec![vec![ - Some(MemoryCell::new(Felt252::new(0x80FF_8000_0530u64).into())), - Some(MemoryCell::new(Felt252::new(0xBFFF_8000_0620u64).into())), - Some(MemoryCell::new(Felt252::new(0x8FFF_8000_0750u64).into())), - ]]; + 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())), + ], + vec![Some(MemoryCell::new((0isize, 0usize).into())); 128 * 1024], + ]; + + cairo_runner + .run_for_steps(1, &mut vm, &mut hint_processor) + .unwrap(); assert_matches!( cairo_runner.get_perm_range_check_limits(&vm), - Ok(Some((1328, 49151))) + Some((32768, 32803)) ); } @@ -3818,11 +3756,6 @@ mod tests { let cairo_runner = cairo_runner!(program); let mut vm = vm!(); - vm.trace = Some(vec![TraceEntry { - pc: 0, - ap: 0, - fp: 0, - }]); vm.segments.memory.data = vec![vec![Some(MemoryCell::new(mayberelocatable!( 0x80FF_8000_0530u64 )))]]; @@ -3830,7 +3763,7 @@ mod tests { assert_matches!( cairo_runner.get_perm_range_check_limits(&vm), - Ok(Some((0, 33023))) + Some((0, 33023)) ); } diff --git a/vm/src/vm/trace/mod.rs b/vm/src/vm/trace/mod.rs index 504da1938f..faab7ae879 100644 --- a/vm/src/vm/trace/mod.rs +++ b/vm/src/vm/trace/mod.rs @@ -1,114 +1,16 @@ -use core::ops::Shl; - -use self::trace_entry::TraceEntry; -use super::{ - decoding::decoder::decode_instruction, errors::vm_errors::VirtualMachineError, - vm_memory::memory::Memory, -}; -use num_traits::ToPrimitive; - -pub mod trace_entry; -const OFFSET_BITS: u32 = 16; -/// Return the minimum and maximum values in the perm_range_check component. -pub fn get_perm_range_check_limits( - trace: &[TraceEntry], - memory: &Memory, -) -> Result, VirtualMachineError> { - trace - .iter() - .try_fold(None, |offsets: Option<(isize, isize)>, trace| { - let instruction = memory.get_integer((0, trace.pc).into())?; - let instruction = instruction - .to_u64() - .ok_or(VirtualMachineError::InvalidInstructionEncoding)?; - - let decoded_instruction = decode_instruction(instruction)?; - let off0 = decoded_instruction.off0 + 1_isize.shl(OFFSET_BITS - 1); - let off1 = decoded_instruction.off1 + 1_isize.shl(OFFSET_BITS - 1); - let off2 = decoded_instruction.off2 + 1_isize.shl(OFFSET_BITS - 1); - - let min_value = off0.min(off1).min(off2); - let max_value = off0.max(off1).max(off2); - Ok( - offsets.map_or(Some((min_value, max_value)), |(min_offset, max_offset)| { - Some((min_offset.min(min_value), max_offset.max(max_value))) - }), - ) - }) -} - -#[cfg(test)] -mod test { - use super::*; - use crate::utils::test_utils::*; - use assert_matches::assert_matches; - - #[cfg(target_arch = "wasm32")] - use wasm_bindgen_test::*; - - /// Test that get_perm_range_check_limits() works as intended with an empty - /// trace. - #[test] - #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] - fn get_perm_range_check_limits_empty_trace() { - let trace = &[]; - let memory = Memory::new(); - - assert_matches!(get_perm_range_check_limits(trace, &memory), Ok(None)); - } - - /// Test that get_perm_range_check_limits() works as intended with a single - /// trace element. - #[test] - #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] - fn get_perm_range_check_limits_single_element() { - let trace = &[TraceEntry { - pc: 0, - ap: 0, - fp: 0, - }]; - - let memory = memory![((0, 0), 0xFFFF_8000_0000_u64)]; - // off0 -32768 - // off1 0 - // off2 32767 - assert_matches!( - get_perm_range_check_limits(trace, &memory), - Ok(Some((0, 65535))) - ); - } - - /// Test that get_perm_range_check_limits() works as intended with multiple - /// trace elements. - #[test] - #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] - fn get_perm_range_check_limits_multiple_elements() { - let trace = &[ - TraceEntry { - pc: 0, - ap: 0, - fp: 0, - }, - TraceEntry { - pc: 1, - ap: 0, - fp: 0, - }, - TraceEntry { - pc: 2, - ap: 0, - fp: 0, - }, - ]; - let memory = memory![ - ((0, 0), 0x80FF_8000_0530_u64), - ((0, 1), 0xBFFF_8000_0620u64), - ((0, 2), 0x8FFF_8000_0750u64) - ]; - - assert_matches!( - get_perm_range_check_limits(trace, &memory), - Ok(Some((1328, 49151))) - ); +pub mod trace_entry { + use serde::{Deserialize, Serialize}; + + ///A trace entry for every instruction that was executed. + ///Holds the register values before the instruction was executed. + /// Before relocation: + /// Register values are represented as their offsets, as their indexes will always be 0,1,1 respectively + /// The index of the last pc will not be equal to 0, but it is not appended to the trace + /// After relocation the value of each register will be a single integer + #[derive(Debug, PartialEq, Eq, Deserialize, Serialize)] + pub struct TraceEntry { + pub pc: usize, + pub ap: usize, + pub fp: usize, } } diff --git a/vm/src/vm/trace/trace_entry.rs b/vm/src/vm/trace/trace_entry.rs deleted file mode 100644 index 9586d1cf4e..0000000000 --- a/vm/src/vm/trace/trace_entry.rs +++ /dev/null @@ -1,15 +0,0 @@ -use crate::stdlib::prelude::*; -use serde::{Deserialize, Serialize}; - -///A trace entry for every instruction that was executed. -///Holds the register values before the instruction was executed. -/// Before relocation: -/// Register values are represented as their offsets, as their indexes will always be 0,1,1 respectively -/// The index of the last pc will not be equal to 0, but it is not appended to the trace -/// After relocation the value of each register will be a single integer -#[derive(Debug, PartialEq, Eq, Deserialize, Serialize)] -pub struct TraceEntry { - pub pc: usize, - pub ap: usize, - pub fp: usize, -} diff --git a/vm/src/vm/vm_core.rs b/vm/src/vm/vm_core.rs index b8b8691b79..a7e15820ac 100644 --- a/vm/src/vm/vm_core.rs +++ b/vm/src/vm/vm_core.rs @@ -78,6 +78,7 @@ pub struct VirtualMachine { pub(crate) segments: MemorySegmentManager, pub(crate) trace: Option>, pub(crate) current_step: usize, + pub(crate) rc_limits: Option<(isize, isize)>, trace_relocated: bool, skip_instruction_execution: bool, run_finished: bool, @@ -107,6 +108,7 @@ impl VirtualMachine { current_step: 0, skip_instruction_execution: false, segments: MemorySegmentManager::new(), + rc_limits: None, run_finished: false, trace_relocated: false, instruction_cache: Vec::new(), @@ -396,6 +398,24 @@ impl VirtualMachine { }); } + // Update range check limits + const OFFSET_BITS: u32 = 16; + let (off0, off1, off2) = ( + instruction.off0 + (1_isize << (OFFSET_BITS - 1)), + instruction.off1 + (1_isize << (OFFSET_BITS - 1)), + instruction.off2 + (1_isize << (OFFSET_BITS - 1)), + ); + self.rc_limits = Some(( + [self.rc_limits.unwrap_or((off0, off0)).0, off0, off1, off2] + .into_iter() + .min() + .unwrap(), + [self.rc_limits.unwrap_or((off0, off0)).1, off0, off1, off2] + .into_iter() + .max() + .unwrap(), + )); + self.segments .memory .mark_as_accessed(operands_addresses.dst_addr); @@ -408,6 +428,7 @@ impl VirtualMachine { self.update_registers(instruction, operands)?; self.current_step += 1; + Ok(()) } @@ -1083,6 +1104,7 @@ impl VirtualMachineBuilder { current_step: self.current_step, skip_instruction_execution: self.skip_instruction_execution, segments: self.segments, + rc_limits: None, run_finished: self.run_finished, trace_relocated: false, instruction_cache: Vec::new(),