feat(new-execution): metered execution #1652
Conversation
shuklaayush
force-pushed
the
feat/new-exec-e2
branch
from
5466709 to
7194ad7
Compare
CodSpeed Walltime Performance ReportMerging #1652 will improve performances by 14.21%Comparing
|
| Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|
| ⚡ | benchmark_execute[bubblesort] |
505.5 ms | 451 ms | +12.09% |
| 🆕 | benchmark_execute[factorial_iterative_u256] |
N/A | 554 ms | N/A |
| ⚡ | benchmark_execute[fibonacci_iterative] |
514.5 ms | 450.5 ms | +14.21% |
| 🆕 | benchmark_execute[revm_snailtracer] |
N/A | 618.5 ms | N/A |
CodSpeed Instrumentation Performance ReportMerging #1652 will improve performances by 15.35%Comparing Summary
Benchmarks breakdown
|
|
|
||
| self.executors.get_mut(id).map(|executor| { | ||
| let insertion_id = self | ||
| .insertion_order |
There was a problem hiding this comment.
isn't this O(num_chips)
There was a problem hiding this comment.
yes, but i'm not sure how to avoid it atm. ideally this should be a constant vector and O(1). i added a todo and i'll fix this in a follow up
shuklaayush
changed the title
| @@ -7,8 +7,8 @@ use std::{ | |||
| use itertools::izip; | |||
| use openvm_circuit::{ | |||
| arch::{ | |||
| AdapterAirContext, AdapterExecutorE1, AdapterTraceStep, ExecutionBridge, ExecutionState, | |||
| VecHeapTwoReadsAdapterInterface, VmAdapterAir, | |||
| execution_mode::E1E2ExecutionCtx, AdapterAirContext, AdapterExecutorE1, AdapterTraceStep, | |||
There was a problem hiding this comment.
I'm also kinda unsure if we use this folder and wha its relation to rv32im/circuit/src/adapters is...
There was a problem hiding this comment.
i just put all the e1/e2 stuff in a new execution_mode folder and it contains this E1E2ExeuctionCtx trait. the trait contains the on_memory_operation callback. this is to make e1 and e2 use the same execute function
| .memory_dimensions | ||
| .label_to_index((address_space, block_id)); | ||
|
|
||
| if let Err(insert_idx) = self.leaf_indices.binary_search(&leaf_id) { |
There was a problem hiding this comment.
ok I didn't read this, but it seems too complicated for something that must be done during execution
There was a problem hiding this comment.
I can change it to add the height of the Merkle tree every time but that might be a huge overestimate
I think the better way is to not maintain a set here and make GuestMemory return a Boolean indicating a page fault. We can then add a fixed height on every page fault
Fixed height would be height of the merkle tree up to page size + size of the full page subtree
There was a problem hiding this comment.
@Golovanov399 wdyt?
There was a problem hiding this comment.
I thought for myself that it'll do for now, because if it's too slow, we'll surely notice
There was a problem hiding this comment.
Took a brief look, deferring to @Golovanov399 's review
I think we all have the sense that the structure / traits should be revisited, but I don't think we should block merge.
|
Yeah I'm all for merging |
execute_meteredfunction that keeps a track oftrace_heightsduring execution and suspends execution if the trace height, number of cells or number of interactions goes above a fixed thresholdResolves INT-3752