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

[move-cm][closures] Refactor: Move type conversions out of Loader into a trait #15669

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

wrwg
Copy link
Contributor

@wrwg wrwg commented Jan 4, 2025

Description

[PR 3/n vm closures]

Type conversions from runtime types to MoveTypeLayout and TypeTag currently are associated with the Loader type. However, they are needed for the FunctionValueExtension trait which needs to be constructed in contexts where no loader but only ModuleStorage exists.

This PR moves the conversion functions into a new trait TypeConverter. The trait is then implemented two times, once based on ModuleStorage only and once based on the existing Loader, for downwards compatibility.

How Has This Been Tested?

Refactoring only, existing tests

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)

Copy link

trunk-io bot commented Jan 4, 2025

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

settingsfeedbackdocs ⋅ learn more about trunk.io

@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 34517a1 to 7862e77 Compare January 5, 2025 05:50
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from 4972a5c to 2fed681 Compare January 5, 2025 05:50
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 7862e77 to ee49138 Compare January 5, 2025 23:50
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from 2fed681 to 9c0814c Compare January 6, 2025 02:47
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from ee49138 to 9fbe74f Compare January 6, 2025 02:47
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch 2 times, most recently from 8b11451 to dcb6bc5 Compare January 6, 2025 03:25
@wrwg wrwg mentioned this pull request Jan 7, 2025
16 tasks
@wrwg wrwg marked this pull request as ready for review January 8, 2025 06:17
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 9fbe74f to a79fde9 Compare January 13, 2025 07:08
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from dcb6bc5 to bfc6f4c Compare January 13, 2025 07:08
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from a79fde9 to 719f728 Compare January 14, 2025 02:32
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from bfc6f4c to 355525d Compare January 14, 2025 02:32
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 8140006 to 40f54be Compare January 21, 2025 16:56
@wrwg wrwg requested a review from zekun000 as a code owner January 21, 2025 16:56
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from 1c9f941 to 5393a01 Compare January 21, 2025 16:56
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 40f54be to 691dc6e Compare January 22, 2025 03:19
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from 5393a01 to d4d5803 Compare January 22, 2025 03:19
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 691dc6e to 14e0465 Compare January 22, 2025 06:23
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from d4d5803 to 892bc51 Compare January 22, 2025 06:23
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 14e0465 to 75db034 Compare January 22, 2025 06:52
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from 892bc51 to bf07d31 Compare January 22, 2025 06:52
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 75db034 to 6dd70e7 Compare January 23, 2025 03:59
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from bf07d31 to 1513955 Compare January 23, 2025 03:59
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 6dd70e7 to cb81217 Compare January 23, 2025 04:06
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from 1513955 to 4022451 Compare January 23, 2025 04:06
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from cb81217 to 86223e7 Compare January 24, 2025 06:03
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from 4022451 to 6f6afe3 Compare January 24, 2025 06:03
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 86223e7 to 41d50fa Compare January 25, 2025 01:06
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from 6f6afe3 to f178823 Compare January 25, 2025 01:07
@wrwg wrwg force-pushed the wrwg/clos_ability_move branch from 41d50fa to 76701d8 Compare January 25, 2025 02:56
Base automatically changed from wrwg/clos_ability_move to main January 25, 2025 04:50
…nto a trait

Type conversions from runtime types to `MoveTypeLayout` and `TypeTag` currently are associated with the `Loader` type. However, they are needed for the `FunctionValueExtension` trait which needs to be constructed in contexts where no loader but only `ModuleStorage` exists.

This PR moves the conversion functions into a new trait `TypeConverter`. The trait is then implemented two times based on `ModuleStorage` only and based on the existing `Loader`, for downwards compatibility.
@wrwg wrwg force-pushed the wrwg/clos_type_conv branch from f178823 to 2707a22 Compare January 25, 2025 06:40
@wrwg wrwg enabled auto-merge (squash) January 25, 2025 06:40

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 2707a22233557a8ea1c12cc8b0340be109b16793

two traffics test: inner traffic : committed: 14659.29 txn/s, latency: 2701.63 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5573840
two traffics test : committed: 99.99 txn/s, latency: 1407.05 ms, (p50: 1400 ms, p70: 1500, p90: 1500 ms, p99: 1900 ms), latency samples: 1760
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.452, avg: 1.429", "ConsensusProposalToOrdered: max: 0.299, avg: 0.291", "ConsensusOrderedToCommit: max: 0.420, avg: 0.403", "ConsensusProposalToCommit: max: 0.710, avg: 0.694"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.65s no progress at version 18930 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.21s no progress at version 6315376 (avg 0.96s) [limit 16].
Test Ok

Copy link
Contributor

✅ Forge suite compat success on cb5d7c9615b937357dbede9439401be640fb7d17 ==> 2707a22233557a8ea1c12cc8b0340be109b16793

Compatibility test results for cb5d7c9615b937357dbede9439401be640fb7d17 ==> 2707a22233557a8ea1c12cc8b0340be109b16793 (PR)
1. Check liveness of validators at old version: cb5d7c9615b937357dbede9439401be640fb7d17
compatibility::simple-validator-upgrade::liveness-check : committed: 12293.83 txn/s, latency: 2597.29 ms, (p50: 2700 ms, p70: 2900, p90: 3200 ms, p99: 4100 ms), latency samples: 401080
2. Upgrading first Validator to new version: 2707a22233557a8ea1c12cc8b0340be109b16793
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 3956.43 txn/s, latency: 7853.74 ms, (p50: 8700 ms, p70: 9300, p90: 9600 ms, p99: 9700 ms), latency samples: 89600
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3997.58 txn/s, latency: 8549.51 ms, (p50: 9500 ms, p70: 9700, p90: 10000 ms, p99: 10100 ms), latency samples: 141120
3. Upgrading rest of first batch to new version: 2707a22233557a8ea1c12cc8b0340be109b16793
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 4581.65 txn/s, latency: 6785.29 ms, (p50: 7700 ms, p70: 8000, p90: 8200 ms, p99: 8300 ms), latency samples: 94500
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 4504.16 txn/s, latency: 7562.79 ms, (p50: 8500 ms, p70: 8600, p90: 8900 ms, p99: 9000 ms), latency samples: 158760
4. upgrading second batch to new version: 2707a22233557a8ea1c12cc8b0340be109b16793
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 7631.12 txn/s, latency: 3996.33 ms, (p50: 4600 ms, p70: 4800, p90: 5200 ms, p99: 5300 ms), latency samples: 141160
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 7592.40 txn/s, latency: 4478.12 ms, (p50: 4900 ms, p70: 5000, p90: 5200 ms, p99: 5400 ms), latency samples: 254480
5. check swarm health
Compatibility test for cb5d7c9615b937357dbede9439401be640fb7d17 ==> 2707a22233557a8ea1c12cc8b0340be109b16793 passed
Test Ok

@wrwg wrwg merged commit d01ae30 into main Jan 25, 2025
83 of 88 checks passed
@wrwg wrwg deleted the wrwg/clos_type_conv branch January 25, 2025 07:19
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.

3 participants