feat(new-execution): make vm segment executor generic #1582
Conversation
| VC: VmConfig<F>, | ||
| { | ||
| /// Guest memory type | ||
| type Mem: GuestMemory; |
There was a problem hiding this comment.
what's the reasoning to have these as associated types?
There was a problem hiding this comment.
we can make them generic too. the reasoning to make these associated types is because i figured that the struct implementing the trait will usually implement it with a specific Mem and Ctx so the trait would never have to be implemented more than once for a struct and doesn't need to generic
| .program_chip | ||
| .get_instruction(vm_state.pc)?; | ||
|
|
||
| let &Instruction { opcode, .. } = instruction; |
There was a problem hiding this comment.
I did not know you could do &Instruction!
| where | ||
| F: PrimeField32, | ||
| { | ||
| let memory = memory_controller.memory_image().clone(); |
There was a problem hiding this comment.
this is pretty bad to clone? is this temporary?
There was a problem hiding this comment.
yep, temporary. i need to figure out how to get memory here since it is owned by the memory controller atm
| segment.set_override_trace_heights(overridden_heights.clone()); | ||
| } | ||
| metrics_span("execute_time_ms", || segment.execute_from_pc(pc_start))?; | ||
| let mut vm_state = TracegenVmExecutionState::from_pc_and_memory_controller( |
There was a problem hiding this comment.
this flow seems a bit weird - ok if it's just temporary until we get rid of or refactor memory controller
There was a problem hiding this comment.
yeah, it's temporary until i figure out how to get rid of memory controller owning the memory and timestamp
| pub pc: u32, | ||
| pub is_terminated: bool, | ||
| pub struct TracegenCtx { | ||
| pub timestamp: u32, |
There was a problem hiding this comment.
fyi: timestamp seems like it will need to be inside TracingMemory later since that is what really manages it most of time, but I expect we move Streams inside Ctx eventually.
There was a problem hiding this comment.
okay. i was wondering if it was possible to keep all execution state here and make all the controllers etc. stateless, and take a reference to this vm state as argument
- make `VmSegmentExecutor` generic on `<Mem, Ctx, Ctrl>` where: - `Mem`: struct that implements `GuestMemory` - `Ctx`: struct that stores host context during execution - `Ctrl`: struct that implements pre/post segment execution hooks, termination condition and instruction execution logic - add `TracegenVmSegmentExecutor` that implements the current execution flow - move segmentation strategies to new file
2 participants
VmSegmentExecutorgeneric on<Mem, Ctx, Ctrl>where:Mem: struct that implementsGuestMemoryCtx: struct that stores host context during executionCtrl: struct that implements pre/post segment execution hooks, termination condition and instruction execution logicTracegenVmSegmentExecutorthat implements the current execution flow