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.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

georgemitenkov
Copy link
Contributor

@georgemitenkov georgemitenkov commented Jan 29, 2025

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

  • New feature

Which Components or Systems Does This Change Impact?

  • Other (specify)

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Jan 29, 2025

⏱️ 27m total CI duration on this PR
Job Cumulative Duration Recent Runs
check-dynamic-deps 14m 🟩🟩🟩🟩🟩
rust-cargo-deny 8m 🟩🟩🟩🟩
general-lints 2m 🟩🟩🟩🟩
semgrep/ci 2m 🟩🟩🟩🟩🟩
file_change_determinator 1m 🟩🟩🟩🟩🟩
permission-check 17s 🟩🟩🟩🟩🟩
permission-check 15s 🟩🟩🟩🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor Author

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

@georgemitenkov georgemitenkov marked this pull request as ready for review January 29, 2025 14:35
@georgemitenkov georgemitenkov force-pushed the george/replay-benchmark-package-override branch from 731260e to c65a196 Compare January 29, 2025 14:36
Copy link
Contributor

@vineethk vineethk left a 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.

aptos-move/replay-benchmark/src/overrides.rs Show resolved Hide resolved
aptos-move/replay-benchmark/src/overrides.rs Show resolved Hide resolved
aptos-move/replay-benchmark/src/overrides.rs Outdated Show resolved Hide resolved
aptos-move/replay-benchmark/src/overrides.rs Outdated Show resolved Hide resolved
aptos-move/replay-benchmark/src/overrides.rs Outdated Show resolved Hide resolved
aptos-move/replay-benchmark/src/commands/initialize.rs Outdated Show resolved Hide resolved
long,
num_args = 1..,
value_delimiter = ' ',
help = "List of space-separated paths to compiled / built packages with Move code"
Copy link
Contributor

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.

Copy link
Contributor Author

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.
Copy link
Contributor

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).

Copy link
Contributor Author

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?

@georgemitenkov georgemitenkov requested a review from gelash January 29, 2025 16:17
@georgemitenkov georgemitenkov force-pushed the george/replay-benchmark-package-override branch from c65a196 to 103467a Compare January 29, 2025 16:43
@georgemitenkov georgemitenkov force-pushed the george/replay-benchmark-package-override branch from 103467a to a8fc584 Compare January 29, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants