-
Notifications
You must be signed in to change notification settings - Fork 11.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[vm rewrite] Swap to a more step-driven interpreter #19219
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
external-crates/move/crates/move-cli/tests/sandbox_tests/print_stack_trace/args.exp
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
if !control_flow_instruction(instruction) { | ||
state.current_frame.pc += 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on instead early returning from the control flow instructions?
Here we could then debug_assert that the state.current_frame.pc
hadn't changed yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The profiling code would make that quite a bit noisier, which seems more-likely to introduce errors.
let resolver = fun_ref.get_resolver(link_context, loader); | ||
|
||
let return_values = eval::call_native_with_args( | ||
None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense for this to be None? Especially since it holds VMRuntimeLimitsConfig
. Should we instead make an empty state of some sort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same initial reaction, though for me looking at the code and try to change that made it more clear why we are doing it.
But all of that (including our NativeContext
definition) made me think we really need to review native invocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could pass the limits separately. The only place this was used is in printing the stack trace.
external-crates/move/crates/move-vm-runtime/src/interpreter/state.rs
Outdated
Show resolved
Hide resolved
pub(crate) fn run( | ||
function: ArenaPointer<Function>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots to think about here for me. At some point in the far future, it would be very interesting to basically call step
, or at least "run" here to invoke a function, with a given state, instead of having the custom execution engine we have today for PTBs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!
Just some comments. I am going to finish the review later today.
I want to look at Tim's PR before the meeting so just leaving you a note right now
external-crates/move/crates/move-cli/tests/sandbox_tests/print_stack_trace/args.exp
Outdated
Show resolved
Hide resolved
let resolver = fun_ref.get_resolver(link_context, loader); | ||
|
||
let return_values = eval::call_native_with_args( | ||
None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same initial reaction, though for me looking at the code and try to change that made it more clear why we are doing it.
But all of that (including our NativeContext
definition) made me think we really need to review native invocation
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
let charge_result = gas_meter.charge_simple_instr(SimpleInstruction::Ret); | ||
profile_close_instr!(gas_meter, format!("{:?}", instruction)); | ||
|
||
partial_error_to_error(state, run_context, charge_result)?; | ||
let non_ref_vals = state | ||
.current_frame | ||
.locals | ||
.drop_all_values() | ||
.map(|(_idx, val)| val); | ||
|
||
// TODO: Check if the error location is set correctly. | ||
gas_meter | ||
.charge_drop_frame(non_ref_vals.into_iter()) | ||
.map_err(|e| state.set_location(e))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may very well be a "fix all things gas" issue, but I do not believe we charge anything for dropping a frame (lines 162-171) and yet we got calls and check and a TODO....
thedrop_all_values
part may actually do something useful (maybe not sure) but...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO was there in the old code -- not sure what to do about it. Let's revisit gas in the future.
79f8453
to
d49dc5d
Compare
d3170e0
to
1ca0e7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really like this and I am good with it.
Let's get this in!
Some of the CI failures should be easy to fix, not sure if this is supposed to be clean or not
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
1ca0e7a
to
7f1ad8c
Compare
Description
This splits up the interpreter to be more step-by-step than the previous loop with control flags approach. It also closes some of the circuit on ArenaPointer usage.
The
move-execution
changes are just due tocargo fmt --all
.Test plan
All tests still pass
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.