-
Notifications
You must be signed in to change notification settings - Fork 11.4k
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
[0/n][vm-rewrite][Move] Add runtime_id
to SerializedPackage
and remove old MoveResolver
trait
#21064
base: cgswords/vm_pointer_exec
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
@@ -259,9 +262,10 @@ impl OnDiskStateView { | |||
fs::create_dir_all(&pkg_path)?; | |||
} | |||
|
|||
for m_bytes in package.modules { | |||
for (nm, m_bytes) in package.modules { |
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.
Nit: use a real name, I have no idea what nm
means here.
@@ -26,11 +26,17 @@ pub struct TypeOrigin { | |||
/// storage IDs. |
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.
Should this come live in move-vm-runtime
eventually?
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.
Possilby, or other stuff from the move-vm-runtime
will need to move here (or some in-between state). We're going to need to have an execution versioning discussion about the move-vm-runtime
crate and where types live as that was cropping up higher up as well for some other types.
@@ -26,11 +26,17 @@ pub struct TypeOrigin { | |||
/// storage IDs. | |||
#[derive(Debug, Clone)] | |||
pub struct SerializedPackage { | |||
pub modules: Vec<Vec<u8>>, |
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 am a little dubious about this change: we have previously discussed init
relying on this order (which is currently sorted based on dependencies), and we need to consider how init
and init_with_args
will look with these changes.
@@ -73,8 +89,6 @@ impl SerializedPackage { | |||
pub trait ModuleResolver { |
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.
Nit: this should be a PackageResolver
now
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.
Seriously concerned about losing module ordering, but otherwise LGTM.
Good point on the module ordering. I think switching over to an |
1417a14
to
53c073f
Compare
…emove old `MoveResolver` trait This adds the `runtime_id` to the `SerializedPackage` struct, changes the module bytes in the `SerializedPackage` to be a `BTreeMap` from the module name to the modules bytes (making module resolution faster and more efficient for certain operations), and removes the old `MoveResolver` trait as it was no longer needed for the new VM, The code in the PR may not be working as future PRs will build on top of this.
…` and remove old `MoveResolver` trait
ced69b5
to
ea01d5f
Compare
3418245
to
5e0a343
Compare
This adds the
runtime_id
to theSerializedPackage
struct, changes the module bytes in theSerializedPackage
to be aBTreeMap
from the module name to the modules bytes (making module resolution faster and more efficient for certain operations), and removes the oldMoveResolver
trait as it was no longer needed for the new VM,The code in the PR may not be working as future PRs will build on top of this.