-
Notifications
You must be signed in to change notification settings - Fork 11.6k
[0/n][vm-rewrite][Move] Add runtime_id
to SerializedPackage
and remove old MoveResolver
trait
#21064
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
} | ||
|
||
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.
|
||
/// The `SerializedPackage` struct holds the serialized modules of a package, the storage ID of the | ||
/// package, and the linkage table that maps the runtime ID found within the package to their | ||
/// 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.
/// 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.
/// (this is not currently possible as ModuleId and StructTag | ||
/// are always structurally valid) | ||
/// - storage encounters internal error | ||
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
merged into the |
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.