Skip to content

Commit

Permalink
perf: keep min and max referenced offsets during run (#1080)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>
Co-authored-by: Tomá <[email protected]>
  • Loading branch information
@Oppen @pefontana @MegaRedHand
3 people authored Jun 13, 2023
1 parent 6d8d492 commit 4221723
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 226 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
131 changes: 32 additions & 99 deletions vm/src/vm/runners/cairo_runner.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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<Option<(isize, isize)>, 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::<Result<usize, MemoryError>>()
.map_err(Into::<VirtualMachineError>::into)?;

let unused_rc_units =
(self.layout.rc_units as usize - 3) * vm.current_step - rc_units_used_by_builtins;
Expand Down Expand Up @@ -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))
);
}

Expand All @@ -3818,19 +3756,14 @@ 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
)))]];
vm.builtin_runners = vec![RangeCheckBuiltinRunner::new(Some(12), 5, true).into()];

assert_matches!(
cairo_runner.get_perm_range_check_limits(&vm),
Ok(Some((0, 33023)))
Some((0, 33023))
);
}

Expand Down
126 changes: 14 additions & 112 deletions vm/src/vm/trace/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -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<Option<(isize, isize)>, 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,
}
}
15 changes: 0 additions & 15 deletions vm/src/vm/trace/trace_entry.rs
View file

This file was deleted.

Loading

1 comment on commit 4221723

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.30.

Benchmark suite Current: 4221723 Previous: 6d8d492 Ratio
add_u64_with_felt/0 1 ns/iter (± 0) 0 ns/iter (± 0) Infinity
add_u64_with_felt/1 4 ns/iter (± 0) 2 ns/iter (± 0) 2
add_u64_with_felt/2 4 ns/iter (± 0) 2 ns/iter (± 0) 2
add_u64_with_felt/3 2 ns/iter (± 0) 1 ns/iter (± 0) 2
add_u64_with_felt/4 2 ns/iter (± 0) 1 ns/iter (± 0) 2
add_u64_with_felt/5 2 ns/iter (± 0) 1 ns/iter (± 0) 2
add_u64_with_felt/6 4 ns/iter (± 0) 2 ns/iter (± 0) 2
add_u64_with_felt/7 4 ns/iter (± 0) 2 ns/iter (± 0) 2
add_u64_with_felt/8 3 ns/iter (± 0) 2 ns/iter (± 0) 1.50

This comment was automatically generated by workflow using github-action-benchmark.

CC: @unbalancedparentheses

Please sign in to comment.