From dbcf4c4bbbcbc80326b57af504b4c72a51119ab0 Mon Sep 17 00:00:00 2001 From: Mario Rugiero Date: Thu, 15 Jun 2023 12:28:17 -0300 Subject: [PATCH] perf: reference management optimizations (#1214) * wip * test passing * more accurate bench * fix * perf: optimizations for reference management - Store directly the reference list rather than the whole `ReferenceManager` structures - Only process them once, when creating the `Program` - Move them to the `SharedProgramData` member - Convert to `Vec` and adapt the methods using it --------- Co-authored-by: Pedro Fontana --- CHANGELOG.md | 6 +++ bench/criterion_benchmark.rs | 4 +- bench/iai_benchmark.rs | 9 ++-- hint_accountant/src/main.rs | 2 +- .../cairo_1_hint_processor/hint_processor.rs | 2 +- .../hint_processor_definition.rs | 6 +-- vm/src/serde/deserialize_program.rs | 4 +- vm/src/types/program.rs | 43 ++++++++++++++----- vm/src/utils.rs | 26 +++++------ vm/src/vm/errors/vm_exception.rs | 15 +++---- vm/src/vm/runners/cairo_runner.rs | 42 +++--------------- 11 files changed, 78 insertions(+), 81 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b5a7b71e3..0a909d2036 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ * 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 +* perf: process reference list on `Program` creation only [#1214](https://github.com/lambdaclass/cairo-rs/pull/1214) + Also keep them in a `Vec<_>` instead of a `HashMap<_, _>` since it will be continuous anyway. + BREAKING: + * `HintProcessor::compile_hint` now receies a `&[HintReference]` rather than `&HashMap` + * Public `CairoRunner::get_reference_list` has been removed + #### [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/bench/criterion_benchmark.rs b/bench/criterion_benchmark.rs index 6ba40bf602..764bb42f70 100644 --- a/bench/criterion_benchmark.rs +++ b/bench/criterion_benchmark.rs @@ -17,7 +17,7 @@ fn parse_program(c: &mut Criterion) { //Picked the biggest one at the time of writing let program = include_bytes!("../cairo_programs/benchmarks/keccak_integration_benchmark.json"); c.bench_function("parse program", |b| { - b.iter(|| { + b.iter_with_large_drop(|| { _ = Program::from_bytes(black_box(program.as_slice()), black_box(Some("main"))) .unwrap(); }) @@ -29,7 +29,7 @@ fn build_many_runners(c: &mut Criterion) { let program = include_bytes!("../cairo_programs/benchmarks/keccak_integration_benchmark.json"); let program = Program::from_bytes(program.as_slice(), Some("main")).unwrap(); c.bench_function("build runner", |b| { - b.iter(|| { + b.iter_with_large_drop(|| { _ = black_box( CairoRunner::new( black_box(&program), diff --git a/bench/iai_benchmark.rs b/bench/iai_benchmark.rs index 9b9b55b8d0..b12c209514 100644 --- a/bench/iai_benchmark.rs +++ b/bench/iai_benchmark.rs @@ -1,4 +1,5 @@ -use iai_callgrind::{black_box, main}; +use core::hint::black_box; +use iai_callgrind::main; use cairo_vm::{ types::program::Program, @@ -18,7 +19,7 @@ fn parse_program() { let program = include_bytes!("../cairo_programs/benchmarks/keccak_integration_benchmark.json"); let program = Program::from_bytes(black_box(program.as_slice()), black_box(Some("main"))).unwrap(); - let _ = black_box(program); + core::mem::drop(black_box(program)); } #[export_name = "helper::parse_program"] @@ -33,7 +34,7 @@ fn parse_program_helper() -> Program { fn build_runner() { let program = parse_program_helper(); let runner = CairoRunner::new(black_box(&program), "starknet_with_keccak", false).unwrap(); - let _ = black_box(runner); + core::mem::drop(black_box(runner)); } #[export_name = "helper::build_runner"] @@ -54,6 +55,6 @@ fn load_program_data() { } main!( - callgrind_args = "toggle-collect=helper::*"; + callgrind_args = "toggle-collect=helper::*,core::mem::drop"; functions = parse_program, build_runner, load_program_data ); diff --git a/hint_accountant/src/main.rs b/hint_accountant/src/main.rs index 31100ad5c3..3a61fa6d54 100644 --- a/hint_accountant/src/main.rs +++ b/hint_accountant/src/main.rs @@ -50,7 +50,7 @@ fn run() { let (ap_tracking_data, reference_ids, references, mut exec_scopes, constants) = ( ApTracking::default(), HashMap::new(), - HashMap::new(), + Vec::new(), ExecutionScopes::new(), HashMap::new(), ); diff --git a/vm/src/hint_processor/cairo_1_hint_processor/hint_processor.rs b/vm/src/hint_processor/cairo_1_hint_processor/hint_processor.rs index f87e4d9731..17abefe0bb 100644 --- a/vm/src/hint_processor/cairo_1_hint_processor/hint_processor.rs +++ b/vm/src/hint_processor/cairo_1_hint_processor/hint_processor.rs @@ -1106,7 +1106,7 @@ impl HintProcessor for Cairo1HintProcessor { //(may contain other variables aside from those used by the hint) _reference_ids: &HashMap, //List of all references (key corresponds to element of the previous dictionary) - _references: &HashMap, + _references: &[HintReference], ) -> Result, VirtualMachineError> { let data = hint_code.parse().ok().and_then(|x: usize| self.hints.get(&x).cloned()) .ok_or(VirtualMachineError::CompileHintFail( diff --git a/vm/src/hint_processor/hint_processor_definition.rs b/vm/src/hint_processor/hint_processor_definition.rs index 294870dd4f..472f4479b2 100644 --- a/vm/src/hint_processor/hint_processor_definition.rs +++ b/vm/src/hint_processor/hint_processor_definition.rs @@ -40,7 +40,7 @@ pub trait HintProcessor { //(may contain other variables aside from those used by the hint) reference_ids: &HashMap, //List of all references (key corresponds to element of the previous dictionary) - references: &HashMap, + references: &[HintReference], ) -> Result, VirtualMachineError> { Ok(any_box!(HintProcessorData { code: hint_code.to_string(), @@ -52,7 +52,7 @@ pub trait HintProcessor { fn get_ids_data( reference_ids: &HashMap, - references: &HashMap, + references: &[HintReference], ) -> Result, VirtualMachineError> { let mut ids_data = HashMap::::new(); for (path, ref_id) in reference_ids { @@ -63,7 +63,7 @@ fn get_ids_data( ids_data.insert( name.to_string(), references - .get(ref_id) + .get(*ref_id) .ok_or(VirtualMachineError::Unexpected)? .clone(), ); diff --git a/vm/src/serde/deserialize_program.rs b/vm/src/serde/deserialize_program.rs index af03542197..67a5034bc9 100644 --- a/vm/src/serde/deserialize_program.rs +++ b/vm/src/serde/deserialize_program.rs @@ -196,7 +196,7 @@ fn deserialize_scientific_notation(n: Number) -> Option { } } -#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)] +#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)] pub struct ReferenceManager { pub references: Vec, } @@ -432,11 +432,11 @@ pub fn parse_program_json( .debug_info .map(|debug_info| debug_info.instruction_locations), identifiers: program_json.identifiers, + reference_manager: Program::get_reference_list(&program_json.reference_manager), }; Ok(Program { shared_program_data: Arc::new(shared_program_data), constants, - reference_manager: program_json.reference_manager, builtins: program_json.builtins, }) } diff --git a/vm/src/types/program.rs b/vm/src/types/program.rs index 8f774180f7..321983dc4d 100644 --- a/vm/src/types/program.rs +++ b/vm/src/types/program.rs @@ -3,11 +3,14 @@ use crate::stdlib::{collections::HashMap, prelude::*, sync::Arc}; #[cfg(feature = "cairo-1-hints")] use crate::serde::deserialize_program::{ApTracking, FlowTrackingData}; use crate::{ + hint_processor::hint_processor_definition::HintReference, serde::deserialize_program::{ deserialize_and_parse_program, Attribute, BuiltinName, HintParams, Identifier, - InstructionLocation, ReferenceManager, + InstructionLocation, OffsetValue, ReferenceManager, + }, + types::{ + errors::program_errors::ProgramError, instruction::Register, relocatable::MaybeRelocatable, }, - types::{errors::program_errors::ProgramError, relocatable::MaybeRelocatable}, }; #[cfg(feature = "cairo-1-hints")] use cairo_lang_starknet::casm_contract_class::CasmContractClass; @@ -48,6 +51,7 @@ pub(crate) struct SharedProgramData { pub(crate) error_message_attributes: Vec, pub(crate) instruction_locations: Option>, pub(crate) identifiers: HashMap, + pub(crate) reference_manager: Vec, } #[derive(Clone, Debug, PartialEq, Eq)] @@ -55,7 +59,6 @@ pub struct Program { pub(crate) shared_program_data: Arc, pub(crate) constants: HashMap, pub(crate) builtins: Vec, - pub(crate) reference_manager: ReferenceManager, } impl Program { @@ -89,11 +92,11 @@ impl Program { error_message_attributes, instruction_locations, identifiers, + reference_manager: Self::get_reference_list(&reference_manager), }; Ok(Self { shared_program_data: Arc::new(shared_program_data), constants, - reference_manager, builtins, }) } @@ -139,6 +142,29 @@ impl Program { .iter() .map(|(cairo_type, identifier)| (cairo_type.as_str(), identifier)) } + + pub(crate) fn get_reference_list(reference_manager: &ReferenceManager) -> Vec { + reference_manager + .references + .iter() + .map(|r| { + HintReference { + offset1: r.value_address.offset1.clone(), + offset2: r.value_address.offset2.clone(), + dereference: r.value_address.dereference, + // only store `ap` tracking data if the reference is referred to it + ap_tracking_data: match (&r.value_address.offset1, &r.value_address.offset2) { + (OffsetValue::Reference(Register::AP, _, _), _) + | (_, OffsetValue::Reference(Register::AP, _, _)) => { + Some(r.ap_tracking_data.clone()) + } + _ => None, + }, + cairo_type: Some(r.value_address.value_type.clone()), + } + }) + .collect() + } } impl Default for Program { @@ -146,9 +172,6 @@ impl Default for Program { Self { shared_program_data: Arc::new(SharedProgramData::default()), constants: HashMap::new(), - reference_manager: ReferenceManager { - references: Vec::new(), - }, builtins: Vec::new(), } } @@ -854,13 +877,13 @@ mod tests { error_message_attributes: Vec::new(), instruction_locations: None, identifiers: HashMap::new(), + reference_manager: Program::get_reference_list(&ReferenceManager { + references: Vec::new(), + }), }; let program = Program { shared_program_data: Arc::new(shared_program_data), constants: HashMap::new(), - reference_manager: ReferenceManager { - references: Vec::new(), - }, builtins: Vec::new(), }; diff --git a/vm/src/utils.rs b/vm/src/utils.rs index ef88dbfb97..80d9c71ae0 100644 --- a/vm/src/utils.rs +++ b/vm/src/utils.rs @@ -265,14 +265,14 @@ pub mod test_utils { error_message_attributes: crate::stdlib::vec::Vec::new(), instruction_locations: None, identifiers: crate::stdlib::collections::HashMap::new(), + reference_manager: Program::get_reference_list(&ReferenceManager { + references: crate::stdlib::vec::Vec::new(), + }), }; Program { shared_program_data: Arc::new(shared_program_data), constants: crate::stdlib::collections::HashMap::new(), builtins: vec![$( $builtin_name ),*], - reference_manager: ReferenceManager { - references: crate::stdlib::vec::Vec::new(), - }, } }}; ($($field:ident = $value:expr),* $(,)?) => {{ @@ -352,10 +352,10 @@ pub mod test_utils { error_message_attributes: val.error_message_attributes, instruction_locations: val.instruction_locations, identifiers: val.identifiers, + reference_manager: Program::get_reference_list(&val.reference_manager), }), constants: val.constants, builtins: val.builtins, - reference_manager: val.reference_manager, } } } @@ -926,13 +926,13 @@ mod test { error_message_attributes: Vec::new(), instruction_locations: None, identifiers: HashMap::new(), + reference_manager: Program::get_reference_list(&ReferenceManager { + references: Vec::new(), + }), }; let program = Program { shared_program_data: Arc::new(shared_data), constants: HashMap::new(), - reference_manager: ReferenceManager { - references: Vec::new(), - }, builtins: Vec::new(), }; assert_eq!(program, program!()) @@ -950,13 +950,13 @@ mod test { error_message_attributes: Vec::new(), instruction_locations: None, identifiers: HashMap::new(), + reference_manager: Program::get_reference_list(&ReferenceManager { + references: Vec::new(), + }), }; let program = Program { shared_program_data: Arc::new(shared_data), constants: HashMap::new(), - reference_manager: ReferenceManager { - references: Vec::new(), - }, builtins: vec![BuiltinName::range_check], }; @@ -975,13 +975,13 @@ mod test { error_message_attributes: Vec::new(), instruction_locations: None, identifiers: HashMap::new(), + reference_manager: Program::get_reference_list(&ReferenceManager { + references: Vec::new(), + }), }; let program = Program { shared_program_data: Arc::new(shared_data), constants: HashMap::new(), - reference_manager: ReferenceManager { - references: Vec::new(), - }, builtins: vec![BuiltinName::range_check], }; diff --git a/vm/src/vm/errors/vm_exception.rs b/vm/src/vm/errors/vm_exception.rs index 2a82fa717e..0d66dc5891 100644 --- a/vm/src/vm/errors/vm_exception.rs +++ b/vm/src/vm/errors/vm_exception.rs @@ -10,10 +10,7 @@ use thiserror::Error; use thiserror_no_std::Error; use crate::{ - hint_processor::{ - hint_processor_definition::HintReference, - hint_processor_utils::get_maybe_relocatable_from_reference, - }, + hint_processor::hint_processor_utils::get_maybe_relocatable_from_reference, serde::deserialize_program::{ApTracking, Attribute, Location, OffsetValue}, types::{instruction::Register, relocatable::MaybeRelocatable}, vm::{runners::cairo_runner::CairoRunner, vm_core::VirtualMachine}, @@ -177,13 +174,11 @@ fn get_value_from_simple_reference( runner: &CairoRunner, vm: &VirtualMachine, ) -> Option { - let reference: HintReference = runner + let reference = runner .program + .shared_program_data .reference_manager - .references - .get(ref_id)? - .clone() - .into(); + .get(ref_id)?; // Filter ap-based references match reference.offset1 { OffsetValue::Reference(Register::AP, _, _) => None, @@ -191,7 +186,7 @@ fn get_value_from_simple_reference( // Filer complex types (only felt/felt pointers) match reference.cairo_type { Some(ref cairo_type) if cairo_type.contains("felt") => Some( - get_maybe_relocatable_from_reference(vm, &reference, ap_tracking)?, + get_maybe_relocatable_from_reference(vm, reference, ap_tracking)?, ), _ => None, } diff --git a/vm/src/vm/runners/cairo_runner.rs b/vm/src/vm/runners/cairo_runner.rs index ff029df56f..baf0352980 100644 --- a/vm/src/vm/runners/cairo_runner.rs +++ b/vm/src/vm/runners/cairo_runner.rs @@ -12,7 +12,7 @@ use crate::{ use crate::{ hint_processor::hint_processor_definition::{HintProcessor, HintReference}, math_utils::safe_div_usize, - serde::deserialize_program::{BuiltinName, OffsetValue}, + serde::deserialize_program::BuiltinName, types::{ errors::{math_errors::MathError, program_errors::ProgramError}, exec_scope::ExecutionScopes, @@ -20,7 +20,6 @@ use crate::{ bitwise_instance_def::BitwiseInstanceDef, ec_op_instance_def::EcOpInstanceDef, ecdsa_instance_def::EcdsaInstanceDef, }, - instruction::Register, layout::CairoLayout, program::Program, relocatable::{relocate_address, relocate_value, MaybeRelocatable, Relocatable}, @@ -484,38 +483,10 @@ impl CairoRunner { self.initial_fp } - pub fn get_reference_list(&self) -> HashMap { - let mut references = HashMap::::new(); - - for (i, reference) in self.program.reference_manager.references.iter().enumerate() { - references.insert( - i, - HintReference { - offset1: reference.value_address.offset1.clone(), - offset2: reference.value_address.offset2.clone(), - dereference: reference.value_address.dereference, - // only store `ap` tracking data if the reference is referred to it - ap_tracking_data: match ( - &reference.value_address.offset1, - &reference.value_address.offset2, - ) { - (OffsetValue::Reference(Register::AP, _, _), _) - | (_, OffsetValue::Reference(Register::AP, _, _)) => { - Some(reference.ap_tracking_data.clone()) - } - _ => None, - }, - cairo_type: Some(reference.value_address.value_type.clone()), - }, - ); - } - references - } - /// Gets the data used by the HintProcessor to execute each hint pub fn get_hint_data_dictionary( &self, - references: &HashMap, + references: &[HintReference], hint_executor: &mut dyn HintProcessor, ) -> Result>>, VirtualMachineError> { let mut hint_data_dictionary = HashMap::>>::new(); @@ -561,8 +532,8 @@ impl CairoRunner { vm: &mut VirtualMachine, hint_processor: &mut dyn HintProcessor, ) -> Result<(), VirtualMachineError> { - let references = self.get_reference_list(); - let hint_data_dictionary = self.get_hint_data_dictionary(&references, hint_processor)?; + let references = &self.program.shared_program_data.reference_manager; + let hint_data_dictionary = self.get_hint_data_dictionary(references, hint_processor)?; #[cfg(feature = "hooks")] vm.execute_before_first_step(self, &hint_data_dictionary)?; @@ -593,8 +564,9 @@ impl CairoRunner { vm: &mut VirtualMachine, hint_processor: &mut dyn HintProcessor, ) -> Result<(), VirtualMachineError> { - let references = self.get_reference_list(); - let hint_data_dictionary = self.get_hint_data_dictionary(&references, hint_processor)?; + let references = &self.program.shared_program_data.reference_manager; + 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));