Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[move-vm][closures] Interpreter #15680

Open
wants to merge 1 commit into
base: wrwg/clos_values
Choose a base branch
from
Open

[move-vm][closures] Interpreter #15680

wants to merge 1 commit into from

Conversation

wrwg
Copy link
Contributor

@wrwg wrwg commented Jan 7, 2025

Description

[PR 5/n vm closures]

This closes the loop, putting the pieces together to implement the closure logic in the interpreter loop.

They are some paranoid checks still missing which are marked in the code.

How Has This Been Tested?

Testing postponed until e2e tests are possible

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

@trunk-io Trunk.io
Copy link

trunk-io bot commented Jan 7, 2025 •
edited
Loading

⏱️ 18m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-move-tests 8m 🟥 
rust-cargo-deny 5m 🟩 🟩 🟩 
check-dynamic-deps 2m 🟩 🟩 🟩 
general-lints 1m 🟩 🟩 🟩 
semgrep/ci 1m 🟩 🟩 🟩 
file_change_determinator 31s 🟩 🟩 🟩 
permission-check 9s 🟩 🟩 🟩 
permission-check 7s 🟩 🟩 🟩 
check-branch-prefix 1s 🟩 

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

This was referenced Jan 7, 2025
Merged
Merged
Merged
@wrwg Graphite App
Copy link
Contributor Author

wrwg commented Jan 7, 2025 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@wrwg wrwg mentioned this pull request Jan 7, 2025
Open
16 tasks
@wrwg wrwg marked this pull request as ready for review January 8, 2025 06:20
@wrwg wrwg force-pushed the wrwg/clos_values branch from 6eea07c to 06363e0 Compare January 13, 2025 07:08
@wrwg wrwg force-pushed the wrwg/clos_interp branch from 2ea7443 to 86af06c Compare January 13, 2025 07:08
@wrwg wrwg force-pushed the wrwg/clos_values branch from 06363e0 to 90f0c08 Compare January 14, 2025 02:32
@wrwg wrwg force-pushed the wrwg/clos_interp branch from 86af06c to 9848747 Compare January 14, 2025 02:32
@wrwg wrwg force-pushed the wrwg/clos_values branch from 90f0c08 to 4ccfa58 Compare January 17, 2025 06:11
@wrwg wrwg force-pushed the wrwg/clos_interp branch from 9848747 to 0547a60 Compare January 17, 2025 06:11
This was referenced Jan 21, 2025
Merged
Merged
@wrwg wrwg force-pushed the wrwg/clos_values branch from 4ccfa58 to c47a82f Compare January 21, 2025 05:09
@wrwg wrwg force-pushed the wrwg/clos_interp branch from 44d692c to 0ff52e9 Compare January 22, 2025 06:23
@wrwg wrwg force-pushed the wrwg/clos_values branch from 9359cfa to 129ffe7 Compare January 22, 2025 06:53
@wrwg wrwg force-pushed the wrwg/clos_interp branch from 0ff52e9 to a208441 Compare January 22, 2025 06:53
@wrwg wrwg force-pushed the wrwg/clos_values branch from 129ffe7 to 474593c Compare January 22, 2025 07:34
@wrwg wrwg force-pushed the wrwg/clos_interp branch 2 times, most recently from ba7ac48 to b4b6105 Compare January 22, 2025 07:44
georgemitenkov
georgemitenkov reviewed Jan 22, 2025
View reviewed changes
third_party/move/move-vm/runtime/src/interpreter.rs Show resolved Hide resolved
third_party/move/move-vm/runtime/src/interpreter.rs Show resolved Hide resolved
third_party/move/move-vm/runtime/src/interpreter.rs
)?;
},
ExitCode::CallClosure(_sig_idx) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we plan to charge gas for dependencies? In the current system, you should call check_dependencies_and_charge_gas for the function's module ID, to ensure that the modules loaded with closure are charged for. You probably should do it after popping the closure from thr stack, since you know function's name and module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, you should charge for ty tags (see my recent PR for that which you approved)

third_party/move/move-vm/runtime/src/interpreter.rs Show resolved Hide resolved
third_party/move/move-vm/runtime/src/interpreter.rs
// TODO(#15664): runtime checks. We should verify that the popped
// argument types match the function.
let function = resolver
.build_loaded_function_from_handle_and_ty_args(*fh_idx, vec![])?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will load the function if it is defined outside of this module, we should be charging gas for module loading here as well.

third_party/move/move-vm/runtime/src/interpreter.rs
@@ -593,21 +676,29 @@ impl InterpreterImpl {
loader: &Loader,
function: Rc<LoadedFunction>,
frame_cache: Rc<RefCell<FrameTypeCache>>,
mask: ClosureMask,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nicer to have frame setups for closures being completely separate, also no need to pass empty mask and empty captured args everywhere. Whatever is here inside can be factored out into helpers.

@wrwg wrwg force-pushed the wrwg/clos_values branch from 474593c to bb8390f Compare January 23, 2025 03:59
@wrwg wrwg force-pushed the wrwg/clos_interp branch from b4b6105 to 2a7face Compare January 23, 2025 03:59
@wrwg wrwg force-pushed the wrwg/clos_values branch from bb8390f to 24a5460 Compare January 23, 2025 04:06
@wrwg wrwg force-pushed the wrwg/clos_interp branch from 2a7face to 83a174e Compare January 23, 2025 04:06
@wrwg wrwg force-pushed the wrwg/clos_values branch from 24a5460 to e6560c5 Compare January 24, 2025 06:03
@wrwg wrwg force-pushed the wrwg/clos_interp branch from 83a174e to f48e6f6 Compare January 24, 2025 06:03
runtian-zhou
runtian-zhou reviewed Jan 24, 2025
View reviewed changes
third_party/move/move-vm/runtime/src/interpreter.rs
@@ -2198,6 +2310,42 @@ impl Frame {
.operand_stack
.push(reference.test_variant(info.variant)?)?;
},
Bytecode::PackClosure(fh_idx, mask) => {
// TODO(#15664): runtime checks. We should verify that the popped
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW I think @ziaptos refactored the runtime checks so that you wouldn't need to add the checks in the core interpretation loop.

runtian-zhou
runtian-zhou reviewed Jan 24, 2025
View reviewed changes
Copy link
Contributor

@runtian-zhou runtian-zhou left a comment

Choose a reason for hiding this comment

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

I think it would be easier for reviewers if you merge #15680 and #15670 as the logics here seem to be tightly related

runtian-zhou
runtian-zhou reviewed Jan 24, 2025
View reviewed changes
third_party/move/move-vm/runtime/src/interpreter.rs
self.operand_stack.pop()?,
loader.vm_config().check_invariant_in_swap_loc,
)?;
for i in (0..num_param_tys).rev() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably use the same helper function you had in ClosureMask to avoid the for loops here? Simialar to call_native

runtian-zhou
runtian-zhou reviewed Jan 24, 2025
View reviewed changes
Copy link
Contributor

@runtian-zhou runtian-zhou left a comment

Choose a reason for hiding this comment

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

Core VM logics seem pretty clean to me. I would want to get another eye from @vgao1995 on the gas semantics just so we are charging call instructions properly?

runtian-zhou
runtian-zhou reviewed Jan 24, 2025
View reviewed changes
Copy link
Contributor

@runtian-zhou runtian-zhou left a comment

Choose a reason for hiding this comment

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

third_party/move/move-vm/runtime/src/runtime_type_checks.rs is the location where paranoid mode is checked. Was wondering if we should make the checks to be nil op if you haven't implemented them yet and don't plan to implement it in this PR because the check should have behaved differently when you switch off the flag

@wrwg wrwg force-pushed the wrwg/clos_values branch from e6560c5 to b3492b5 Compare January 25, 2025 01:07
@wrwg wrwg force-pushed the wrwg/clos_interp branch from f48e6f6 to fd6aabd Compare January 25, 2025 01:07
@wrwg wrwg force-pushed the wrwg/clos_values branch from b3492b5 to c668c04 Compare January 27, 2025 04:12
@wrwg wrwg force-pushed the wrwg/clos_interp branch 3 times, most recently from ff41d73 to 9848bce Compare January 27, 2025 05:54
@wrwg wrwg force-pushed the wrwg/clos_values branch from 90f4c9a to 3864e40 Compare January 27, 2025 16:26
@wrwg wrwg force-pushed the wrwg/clos_interp branch from 9848bce to 2cce955 Compare January 27, 2025 16:27
@wrwg
[move-vm][closures] Interpreter
57afe25 
[PR 5/n vm closures]

This closes the loop, putting the pieces together to implement the closure logic in the interpreter loop.

They are some paranoid checks still missing which are marked in the code.
@wrwg wrwg force-pushed the wrwg/clos_values branch from 3864e40 to cfde21d Compare January 27, 2025 23:52
@wrwg wrwg force-pushed the wrwg/clos_interp branch from 2cce955 to 57afe25 Compare January 27, 2025 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

@georgemitenkov georgemitenkov georgemitenkov left review comments

@runtian-zhou runtian-zhou runtian-zhou left review comments

@vgao1996 vgao1996 Awaiting requested review from vgao1996

@vineethk vineethk Awaiting requested review from vineethk

@ziaptos ziaptos Awaiting requested review from ziaptos

@davidiw davidiw Awaiting requested review from davidiw

@movekevin movekevin Awaiting requested review from movekevin

@zekun000 zekun000 Awaiting requested review from zekun000

@banool banool Awaiting requested review from banool

@gregnazario gregnazario Awaiting requested review from gregnazario

@0xmaayan 0xmaayan Awaiting requested review from 0xmaayan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wrwg @runtian-zhou @georgemitenkov