-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[benchmark] Add option for package overrides #15841
[benchmark] Add option for package overrides #15841
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
731260e
to
c65a196
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.
Looks great and a very useful functionality. Left a few minor comments.
long, | ||
num_args = 1.., | ||
value_delimiter = ' ', | ||
help = "List of space-separated paths to compiled / built packages with Move code" |
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.
Another cool feature this brings (and I think we should be explicit about it in the readme) is that you can compile using different compiler versions/optimization levels/experimental compiler stuff and just point to the built packages, giving the ability to compare them.
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.
Yes yes yes! Exactly, but for that I need to modify CLI to pass flags to build option, or instead get already build packages from the path (ideally)
// 1. Override gas schedule, to track the costs of charging gas or tracking limits. | ||
// 2. BlockExecutorConfigFromOnchain to experiment with different block cutting based | ||
// on gas limits?. | ||
// 3. Build options for package overrides. |
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.
Ah, so this would allow pointing to just source. Perhaps mention that right now, the build option is only used for getting the bytecode version (and that this wont recompile the package with move-2).
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.
Right now compilation happens with hardcoded options, ideally one should be able to paramterize this, do you have specific things in mind I should add? e.g., how do I enable optimization levels, etc.
My original plan was to point to already build package (so that using compiler is external), but I did not find a way to retrieve the package from the filesystem (i.e., we don't have APIs for that, etc.) so instead just used this for now. You can probably suggest more here from compiler side?
103467a
to
a8fc584
Compare
a8fc584
to
5e3e076
Compare
package_address, err | ||
) | ||
}) | ||
.expect("Package registry for override must always exist"), |
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.
Is there an easy workflow to emulate publishing a new package and using that elsewhere? (but i guess for that purpose, can always include the new one inside the updated package)?
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.
Not that I know of, package system was built on top of the modules so it is quite hacky
let mut metadata_idx = None; | ||
for (idx, package_metadata) in package_registry.packages.iter().enumerate() { | ||
if package_metadata.name == metadata.name { | ||
metadata_idx = Some(idx); |
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 can assign it here and only handle None below?
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 not sure, you need to clone metadata then (Rust complains)? Even though it is clearly that metadata cannot be moved if we fallback to break.
5e3e076
to
c8171a9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
Description
This PR adds support for package overriding: one can specify
--override-packages P1 P2 P3
and replay historical transactions with new code (e.g., framework). This allows to check performance (benchmark
option) or gas usage (diff
option).Note: one needs to be careful overriding packages which add new package dependencies. For example, it is possible that stdlib adds a module, which is used in framework code. For override, one needs to provide both packages.
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist