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] Remove legacy type builder #14002

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

georgemitenkov
Copy link
Contributor

@georgemitenkov georgemitenkov commented Jul 15, 2024

Description

The feature to limit VM type sizes has been enabled, so this PR removes the legacy type builder implementation.

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
  • Other (specify)

How Has This Been Tested?

Key Areas to Review

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 Jul 15, 2024

⏱️ 7h 27m total CI duration on this PR
Job Cumulative Duration Recent Runs
test-fuzzers 3h 21m 🟩🟩🟩🟩 (+1 more)
execution-performance / single-node-performance 47m 🟩🟩
forge-e2e-test / forge 32m 🟥🟩
forge-framework-upgrade-test / forge 17m 🟩
rust-move-tests 15m 🟩
forge-compat-test / forge 14m 🟩
rust-move-unit-coverage 14m 🟩
rust-move-unit-coverage 13m 🟩
rust-move-tests 10m 🟩
rust-move-tests 9m 🟩
rust-move-tests 9m 🟩
general-lints 9m 🟩🟩🟩🟩
rust-cargo-deny 9m 🟩🟩🟩🟩
execution-performance / test-target-determinator 8m 🟩🟩
rust-move-unit-coverage 7m 🟩
cli-e2e-tests / run-cli-tests 7m 🟩
check-dynamic-deps 6m 🟩🟩🟩🟩🟩 (+1 more)
test-target-determinator 4m 🟩
check 4m 🟩
semgrep/ci 2m 🟩🟩🟩🟩🟩 (+1 more)
rust-move-tests 2m
rust-move-unit-coverage 2m
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose 2m 🟩
file_change_determinator 1m 🟩🟩🟩🟩 (+1 more)
file_change_determinator 57s 🟩🟩🟩🟩 (+1 more)
permission-check 18s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 16s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 15s 🟩🟩🟩🟩🟩 (+1 more)
file_change_determinator 14s 🟩
Backport PR 12s 🟥🟥
permission-check 11s 🟩🟩🟩🟩 (+1 more)
permission-check 5s 🟩🟩
permission-check 4s 🟩🟩
determine-docker-build-metadata 2s 🟩
rust-move-tests 1s

🚨 4 jobs on the last run were significantly faster/slower than expected

Job Duration vs 7d avg Delta
execution-performance / single-node-performance 25m 11m +121%
forge-framework-upgrade-test / forge 17m 9m +100%
test-fuzzers 45m 37m +23%
execution-performance / test-target-determinator 4m 6m -25%

settingsfeedbackdocs ⋅ learn more about trunk.io

@georgemitenkov georgemitenkov requested review from runtian-zhou and ziaptos and removed request for davidiw, wrwg and zekun000 July 15, 2024 08:38
@georgemitenkov georgemitenkov force-pushed the george/ty-builder-legacy-removal branch from e59980f to af175eb Compare July 15, 2024 08:40
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 82.40000% with 22 lines in your changes missing coverage. Please review.

Project coverage is 58.9%. Comparing base (34a404c) to head (4a2f7fe).
Report is 11 commits behind head on main.

Files Patch % Lines
aptos-move/aptos-vm-types/src/environment.rs 0.0% 9 Missing ⚠️
...ove/move-vm/types/src/loaded_data/runtime_types.rs 92.0% 8 Missing ⚠️
aptos-move/aptos-vm/src/move_vm_ext/vm.rs 0.0% 3 Missing ⚠️
...ptos-vm/src/verifier/transaction_arg_validation.rs 0.0% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (34a404c) and HEAD (4a2f7fe). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (34a404c) HEAD (4a2f7fe)
2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main   #14002       +/-   ##
===========================================
- Coverage    70.8%    58.9%    -11.9%     
===========================================
  Files        2322      824     -1498     
  Lines      459381   197978   -261403     
===========================================
- Hits       325331   116720   -208611     
+ Misses     134050    81258    -52792     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@georgemitenkov georgemitenkov force-pushed the george/ty-builder-legacy-removal branch from af175eb to 75c7b6c Compare July 15, 2024 21:06
Copy link
Contributor

@vgao1996 vgao1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we checked if it's safe to remove the old behavior through replay?

@georgemitenkov
Copy link
Contributor Author

@vgao1996 Yes when we selected the limits for the type builder, but there is a chance that some transaction before that and actual release can break things. I would prefer to land it and see if replay passes on main, to save some CI money, and if not, revert. Does this sound good?

Also, maybe we can select a range for the replay (only from the last tested txn, @perryjrandall do you know how I specify that in replay action?)

@georgemitenkov georgemitenkov force-pushed the george/ty-builder-legacy-removal branch from 75c7b6c to 4a2f7fe Compare July 16, 2024 08:18
Copy link
Contributor

@vgao1996 vgao1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if we didn't catch anything last time then I'd be fine taking a bet.

@perryjrandall
Copy link
Contributor

@vgao1996 Yes when we selected the limits for the type builder, but there is a chance that some transaction before that and actual release can break things. I would prefer to land it and see if replay passes on main, to save some CI money, and if not, revert. Does this sound good?

Also, maybe we can select a range for the replay (only from the last tested txn, @perryjrandall do you know how I specify that in replay action?)

Currently its not really easily possible @aluon it would be great to have a job which could just run a single range instead of the full range in case someone knows which txns they want to test

Copy link
Contributor

@runtian-zhou runtian-zhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thanks for cleaning it up!

@georgemitenkov georgemitenkov enabled auto-merge (squash) July 17, 2024 19:13
@georgemitenkov georgemitenkov added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 17, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c (PR)
1. Check liveness of validators at old version: 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5
compatibility::simple-validator-upgrade::liveness-check : committed: 7236.430046687059 txn/s, latency: 3952.0724307774226 ms, (p50: 3200 ms, p90: 3900 ms, p99: 27900 ms), latency samples: 300480
2. Upgrading first Validator to new version: 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 1850.970053919586 txn/s, latency: 11427.172476060192 ms, (p50: 11600 ms, p90: 25100 ms, p99: 26300 ms), latency samples: 73100
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1324.8329070154982 txn/s, latency: 20262.54439723845 ms, (p50: 24500 ms, p90: 28600 ms, p99: 29000 ms), latency samples: 75320
3. Upgrading rest of first batch to new version: 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7247.252426950779 txn/s, latency: 3718.7709114139693 ms, (p50: 4300 ms, p90: 4500 ms, p99: 4700 ms), latency samples: 140880
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6424.245699943597 txn/s, latency: 4771.507679331673 ms, (p50: 4600 ms, p90: 5500 ms, p99: 8500 ms), latency samples: 248980
4. upgrading second batch to new version: 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 9887.53567623546 txn/s, latency: 2686.22803605851 ms, (p50: 2500 ms, p90: 4800 ms, p99: 5500 ms), latency samples: 176380
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 9752.104266762051 txn/s, latency: 3332.50546652642 ms, (p50: 3100 ms, p90: 4500 ms, p99: 5900 ms), latency samples: 323240
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c passed
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c (PR)
Upgrade the nodes to version: 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1072.725211005885 txn/s, submitted: 1076.1531782332154 txn/s, failed submission: 3.4279672273302677 txn/s, expired: 3.4279672273302677 txn/s, latency: 2841.84147848317 ms, (p50: 2100 ms, p90: 5400 ms, p99: 8400 ms), latency samples: 93880
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1077.3476024896268 txn/s, submitted: 1079.8022055607505 txn/s, failed submission: 2.454603071123839 txn/s, expired: 2.454603071123839 txn/s, latency: 2892.3691487158244 ms, (p50: 2100 ms, p90: 5400 ms, p99: 9900 ms), latency samples: 96560
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c passed
Upgrade the remaining nodes to version: 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1161.95029604738 txn/s, submitted: 1165.262717986835 txn/s, failed submission: 3.3124219394549628 txn/s, expired: 3.3124219394549628 txn/s, latency: 2704.4880472408877 ms, (p50: 2100 ms, p90: 4800 ms, p99: 10200 ms), latency samples: 98220
Test Ok

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 4a2f7fec9acc4ac9f9f8cad1a5b58c45d78bb21c

two traffics test: inner traffic : committed: 9203.678458003542 txn/s, latency: 4330.81445614777 ms, (p50: 4100 ms, p90: 4900 ms, p99: 11100 ms), latency samples: 3499480
two traffics test : committed: 99.90075489191075 txn/s, latency: 2731.791176470588 ms, (p50: 2400 ms, p90: 3700 ms, p99: 8200 ms), latency samples: 1700
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.238, avg: 0.219", "QsPosToProposal: max: 1.716, avg: 1.396", "ConsensusProposalToOrdered: max: 0.315, avg: 0.293", "ConsensusOrderedToCommit: max: 0.448, avg: 0.405", "ConsensusProposalToCommit: max: 0.763, avg: 0.698"]
Max round gap was 1 [limit 4] at version 1307862. Max no progress secs was 5.810748 [limit 15] at version 1307862.
Test Ok

@georgemitenkov georgemitenkov merged commit 0f49e1c into main Jul 17, 2024
114 of 142 checks passed
@georgemitenkov georgemitenkov deleted the george/ty-builder-legacy-removal branch July 17, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants