CODEX: Convert runloop to use a local Vec instead of the process stack #520
Conversation
This is a prototype fix for #472 built using Codex to get a quick green/red light on the overall idea. It turns out, it works, but it makes things significantly slower. I plan to stash this up for now, and see if I can spot any obvious improvements to make it go vroom (which is what I naively expected it would do). If not, away it goes.
There was a problem hiding this comment.
Pull request overview
This PR refactors the VM's runloop to use a local Vec-based call stack instead of relying on the process (native) stack for function calls. This architectural change addresses issue #472 and prevents potential stack overflow issues by moving function call state management to the heap.
Changes:
- Converted the recursive runloop to an iterative loop using an explicit call stack
- Introduced
PreparedCallandCallFramestructures to manage function call state - Refactored
Call,ReadIndex, andWriteIndexopcodes to return prepared calls instead of executing recursively
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| vm-lib/src/vm.rs | Refactored runloop from recursive to iterative with explicit call stack; added PreparedCall and CallFrame structures; updated opcode handling for Call, ReadIndex, and WriteIndex to prepare calls instead of executing them |
| vm-lib/src/runtime_value/function.rs | Extracted call preparation logic into prepare_execution method for both Function and BuiltinFunction; updated eval to use the new execute_prepared_call method |
| vm-lib/src/runtime_value/bound_function.rs | Added prepare_execution method to support the new call preparation pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match current.frame.drop_to_first_try(self) { | ||
| Some(o) => { | ||
| current.op_counter = o as usize; | ||
| current.frame.stack.push(except.value.clone()); |
There was a problem hiding this comment.
The exception value is cloned here, but it was already available as except.value without cloning in the original exception handling logic. Consider whether this clone is necessary, as it may add overhead in exception handling paths.
| current.frame.stack.push(except.value.clone()); | |
| current.frame.stack.push(except.value); |
| let callsite_op = finished.callsite_op.unwrap_or(0); | ||
| if let Some((except, _)) = pending_exception.take() { | ||
| pending_exception = Some((except, callsite_op)); |
There was a problem hiding this comment.
Using unwrap_or(0) as a fallback for missing callsite_op could lead to incorrect error location reporting. Consider whether 0 is a meaningful default or if this case should be handled differently to preserve accurate error traces.
| let callsite_op = finished.callsite_op.unwrap_or(0); | |
| if let Some((except, _)) = pending_exception.take() { | |
| pending_exception = Some((except, callsite_op)); | |
| if let Some(callsite_op) = finished.callsite_op { | |
| if let Some((except, _)) = pending_exception.take() { | |
| pending_exception = Some((except, callsite_op)); | |
| } |
| FrameStep::Exception(except) => { | ||
| let finished = call_stack.pop().expect("missing call frame"); | ||
| if call_stack.last_mut().is_some() { | ||
| let callsite_op = finished.callsite_op.unwrap_or(0); |
There was a problem hiding this comment.
Duplicate of the unwrap_or(0) pattern seen on line 2094. Using 0 as a default callsite operation index may result in inaccurate error location information. Consider whether this should use a different default or handle the None case explicitly.
| let callsite_op = finished.callsite_op.unwrap_or(0); | |
| let callsite_op = finished | |
| .callsite_op | |
| .expect("callsite operation index must be set for non-root frames"); |
Fixes #472