Introduce code cache and loader V2 #14184

georgemitenkov · 2024-08-01T07:28:27Z

Description

This PR keeps track of all loader & code cache changes, and will be linked to an AIP.

Type of Change

Which Components or Systems Does This Change Impact?

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

trunk-io · 2024-08-01T07:28:29Z

⏱️ 23h 2m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
replay-testnet / replay-verify (17)	1h 41m	🟥
replay-mainnet / replay-verify (18)	1h 35m	🟥
replay-mainnet / replay-verify (2)	1h 34m	🟥
replay-mainnet / replay-verify (3)	1h 34m	🟥
replay-mainnet / replay-verify (7)	1h 33m	🟥
replay-mainnet / replay-verify (34)	1h 30m	🟥
replay-testnet / replay-verify (14)	1h 28m	🟥
replay-testnet / replay-verify (4)	1h 26m	🟥
replay-mainnet / replay-verify (14)	1h 22m	🟥
replay-testnet / replay-verify (7)	1h 19m	🟥
replay-mainnet / replay-verify (20)	1h 18m	🟥
replay-testnet / replay-verify (8)	1h 18m	🟩
replay-mainnet / replay-verify (30)	1h 18m	🟥
replay-mainnet / replay-verify (11)	1h 17m	🟥
replay-testnet / replay-verify (5)	1h 17m	🟥

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

georgemitenkov · 2024-08-01T07:28:41Z

Introduce code cache and loader V2 #14184 : 7 dependent PRs (#14075 , #14206 , #14341 and 4 others) 👈
[move] Change interpreter to use loaded func and improve scoping #14185
main

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

Join @georgemitenkov and the rest of your teammates on Graphite

codecov · 2024-08-01T11:49:04Z

Codecov Report

Attention: Patch coverage is 33.26643% with 2660 lines in your changes missing coverage. Please review.

Project coverage is 59.2%. Comparing base (0e4f5df) to head (4a31c6e).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
aptos-move/block-executor/src/code_cache.rs	0.0%	488 Missing ⚠️
aptos-move/aptos-vm/src/aptos_vm.rs	0.0%	421 Missing ⚠️
third_party/move/move-vm/runtime/src/loader/mod.rs	61.7%	188 Missing ⚠️
...ptos-move/block-executor/src/cross_block_caches.rs	0.0%	173 Missing ⚠️
aptos-move/block-executor/src/executor.rs	0.0%	150 Missing ⚠️
aptos-move/aptos-vm-environment/src/environment.rs	0.0%	149 Missing ⚠️
aptos-move/block-executor/src/captured_reads.rs	0.0%	122 Missing ⚠️
aptos-move/aptos-vm-environment/src/gas.rs	0.0%	69 Missing ⚠️
...e_vm_ext/session/user_transaction_sessions/user.rs	0.0%	67 Missing ⚠️
third_party/move/move-vm/runtime/src/session.rs	22.9%	67 Missing ⚠️
... and 51 more

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #14184     +/-   ##
=========================================
- Coverage    60.1%    59.2%   -0.9%     
=========================================
  Files         856      876     +20     
  Lines      211110   214751   +3641     
=========================================
+ Hits       126962   127230    +268     
- Misses      84148    87521   +3373

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

- Helper for struct name index map & type cache for Loader

ziaptos

No major issues found.

I'd just make sure to:

Document in a separate document all TODOs introduced by this PR (and there seems to be a lot of them). Then change the comments to refer to that document (or an issue no).
I'd still do the renaming of some variable and types, like ModuleStorageAdapter, to LegacyModuleStorageAdapter, etc....

ziaptos · 2024-10-29T02:07:40Z

third_party/move/move-vm/runtime/src/move_vm.rs

        Self {
            runtime: VMRuntime::new(natives, vm_config),
        }
    }

+    pub fn new_with_runtime_environment(runtime_environment: &RuntimeEnvironment) -> Self {
+        // Loader V2 does not store any natives, so we save the clone here.


A bit confusing comment. Seems like runtime_envoronment.natives() should be empty here? Maybe debug_assert?

This was confusing, fixed. Before, what I did is just avoided clone because V2 loader does not store natives (but they are stored in runtime environment). After addressing comments: removed MoveVM::new and MoveVM::new_with_config to ensure VM is always created based on the specified runtime environment (MoveVM::new_with_runtime_environment). Now there is less duplication, and the code is clear, and less clones in tests.

ziaptos · 2024-10-29T02:08:45Z

third_party/move/move-vm/runtime/src/move_vm.rs

+                Ok(module.as_ref().deref().clone())
+            },
+            Loader::V2(_) => Err(PartialVMError::new(
+                StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR,


This should also be unreachable? right? Do we need to handle it gracefully?

Gracefully as if ..? Here we use invariant violation so that we do not panic, in case something goes wrong

ziaptos · 2024-10-29T02:14:31Z

third_party/move/move-vm/runtime/src/native_functions.rs

+                    module_id.name(),
+                    function_name,
+                )
+                // Note: Keeping this consistent with Loader V1 implementation.


I like the note, but would add more context, as the V1 code above will be removed.

Added, also changed to TODO, because not remapping the error breaks some tests, but replay might be ok

ziaptos · 2024-10-29T02:15:58Z

third_party/move/move-vm/runtime/src/runtime.rs

-                vm_config,
-            ),
+            loader,
+            // Note(loader_v2): We still create this cache, but if V2 loader is used, it is not used.


nit: even though V2 is not using it. Maybe add a removal plan in the comment?

Done. Changed into a TODO so we can track this

ziaptos · 2024-10-29T02:20:03Z

third_party/move/move-vm/runtime/src/storage/code_storage.rs

+use std::sync::Arc;
+
+/// Returns the hash (SHA-3-256) of the bytes. Used for both modules and scripts.
+pub fn compute_code_hash(bytes: &[u8]) -> [u8; 32] {


Nit: This can be a generic utility function as well

Changed the name to sha3_256, and moved to move-vm-types crate so it serves as a generic utility.

third_party/move/move-vm/runtime/src/storage/publishing.rs

types/src/access_path.rs

ziaptos · 2024-10-29T02:33:00Z

third_party/move/move-vm/runtime/src/loader/modules.rs

@@ -311,17 +312,12 @@ impl Module {
                let module_handle = module.module_handle_at(struct_handle.module);
                let module_id = module.module_id_for_handle(module_handle);

-                if module_handle != module.self_handle() {


That seems right, We might be changing some behavior though by removing these. However, on the other side, if the reply is fine, we should be OK.

beer-1

LGTM, thanks for amazing improvement.

Current implementation is already very good for us than loader v1 a lot, but if I have to say something, it would be cool if we can use custom type_cache impl in the RuntimeEnvironment like ModuleStorage. In our case, we are using code checksum based cache, so want to implement checksum based type_cache impl if possible 👍

In addition to the comments, also force eviction of published modules from global cache. Enable V2 flag for some tests.

georgemitenkov · 2024-10-29T14:11:27Z

@beer-1 good point about the type cache! Let's address this in a separate PR, I added a loader TODO for this to track it.

georgemitenkov · 2024-10-29T14:20:42Z

@ziaptos thanks for review!

For TODOs, I use TODO(loader_v2): ... to make things searchable and do not lose any. I think creating issues is an overkill because half of them I will address as a part of the roll out. But we can create issues for things we do not want to fix short-term. Doc is a good idea to organise those👍 I already have one in notion
Renamed the types as well

- Renamed all types/traits to `Legacy..` - Added TODOs for removal and type cache handling

github-actions · 2024-10-29T18:25:40Z

✅ Forge suite `realistic_env_max_load` success on `bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4`

two traffics test: inner traffic : committed: 14451.46 txn/s, latency: 2750.22 ms, (p50: 2700 ms, p70: 2700, p90: 2900 ms, p99: 3000 ms), latency samples: 5495020
two traffics test : committed: 100.08 txn/s, latency: 1519.07 ms, (p50: 1300 ms, p70: 1400, p90: 1500 ms, p99: 7400 ms), latency samples: 1840
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.976, avg: 1.575", "ConsensusProposalToOrdered: max: 0.313, avg: 0.292", "ConsensusOrderedToCommit: max: 0.364, avg: 0.353", "ConsensusProposalToCommit: max: 0.657, avg: 0.645"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.90s no progress at version 2886964 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.48s no progress at version 2886962 (avg 8.48s) [limit 15].
Test Ok

github-actions · 2024-10-29T18:27:32Z

✅ Forge suite `framework_upgrade` success on `9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd` ==> `bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4`

Compatibility test results for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4 (PR)
Upgrade the nodes to version: bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1075.75 txn/s, submitted: 1077.59 txn/s, failed submission: 1.84 txn/s, expired: 1.84 txn/s, latency: 3137.38 ms, (p50: 2700 ms, p70: 3300, p90: 5400 ms, p99: 11300 ms), latency samples: 93700
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1312.33 txn/s, submitted: 1315.03 txn/s, failed submission: 2.71 txn/s, expired: 2.71 txn/s, latency: 2343.42 ms, (p50: 2100 ms, p70: 2400, p90: 3900 ms, p99: 5000 ms), latency samples: 116400
5. check swarm health
Compatibility test for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4 passed
Upgrade the remaining nodes to version: bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1171.07 txn/s, submitted: 1174.42 txn/s, failed submission: 3.35 txn/s, expired: 3.35 txn/s, latency: 2614.28 ms, (p50: 2700 ms, p70: 3000, p90: 3900 ms, p99: 5400 ms), latency samples: 104800
Test Ok

github-actions · 2024-10-29T18:31:02Z

✅ Forge suite `compat` success on `9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd` ==> `bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4`

Compatibility test results for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4 (PR)
1. Check liveness of validators at old version: 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd
compatibility::simple-validator-upgrade::liveness-check : committed: 13712.76 txn/s, latency: 2111.69 ms, (p50: 1600 ms, p70: 1800, p90: 2000 ms, p99: 23200 ms), latency samples: 545680
2. Upgrading first Validator to new version: bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 5864.22 txn/s, latency: 4720.99 ms, (p50: 4700 ms, p70: 5700, p90: 6500 ms, p99: 7000 ms), latency samples: 113360
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6002.51 txn/s, latency: 5361.79 ms, (p50: 5800 ms, p70: 6100, p90: 6500 ms, p99: 7400 ms), latency samples: 208280
3. Upgrading rest of first batch to new version: bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6190.31 txn/s, latency: 4313.16 ms, (p50: 4900 ms, p70: 5000, p90: 5200 ms, p99: 5300 ms), latency samples: 125060
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6628.49 txn/s, latency: 4862.90 ms, (p50: 5200 ms, p70: 5300, p90: 6300 ms, p99: 6700 ms), latency samples: 229820
4. upgrading second batch to new version: bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 8639.70 txn/s, latency: 3233.42 ms, (p50: 3600 ms, p70: 3800, p90: 4100 ms, p99: 4500 ms), latency samples: 157320
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 7230.43 txn/s, latency: 4359.66 ms, (p50: 3600 ms, p70: 3900, p90: 9800 ms, p99: 11100 ms), latency samples: 244320
5. check swarm health
Compatibility test for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4 passed
Test Ok

georgemitenkov changed the title ~~[loader] Add feature flag to be able to switch to V2 implementation~~ [DO NOT MERGE] Introduce code cache and loader V2 Aug 1, 2024

georgemitenkov marked this pull request as ready for review August 1, 2024 07:31

georgemitenkov force-pushed the george/main branch from ce4af1b to a13c2f0 Compare August 1, 2024 11:39

georgemitenkov changed the base branch from main to george/loaded-func August 1, 2024 11:39

georgemitenkov mentioned this pull request Aug 1, 2024

[move] Change interpreter to use loaded func and improve scoping #14185

Merged

21 tasks

georgemitenkov mentioned this pull request Aug 1, 2024

[move] Loader V2 API integration for MoveVM #14075

Merged

21 tasks

georgemitenkov requested review from davidiw, movekevin, wrwg, zekun000 and vgao1996 as code owners August 3, 2024 09:44

georgemitenkov removed request for davidiw, vgao1996, wrwg, zekun000 and movekevin August 3, 2024 12:29

Base automatically changed from george/loaded-func to main August 3, 2024 13:43

georgemitenkov force-pushed the george/main branch from fc53996 to 75195dc Compare August 3, 2024 19:29

georgemitenkov mentioned this pull request Aug 5, 2024

[move][aptos-vm] Integrate loader V2 into AptosVM & add test/adapter storages #14206

Merged

21 tasks

georgemitenkov force-pushed the george/main branch 2 times, most recently from 6753464 to 5a1517f Compare August 11, 2024 00:59

georgemitenkov force-pushed the george/main branch from 5a1517f to f49e8b1 Compare August 18, 2024 22:22

georgemitenkov mentioned this pull request Aug 19, 2024

[aptos-vm] init_module & state value metadata for V2 loader #14341

Merged

21 tasks

georgemitenkov force-pushed the george/main branch from f49e8b1 to 9647453 Compare August 22, 2024 10:09

This was referenced Aug 23, 2024

[move][aptos-vm] End-to-end module publishing #14395

Merged

[move] Fixing unit tests for loader V2 #14397

Merged

[move][aptos-vm] Addressing comments #14402

Merged

georgemitenkov added 3 commits October 28, 2024 15:31

[perf] Optimize index map cloning

dd9c43a

[nits] Multiple cleanups & improvements

378e898

- Helper for struct name index map & type cache for Loader

rebase

6f77fe7

georgemitenkov force-pushed the george/main branch from b6d9e84 to 6f77fe7 Compare October 28, 2024 15:32

This comment has been minimized.

Sign in to view

ziaptos approved these changes Oct 29, 2024

View reviewed changes

ziaptos requested a review from beer-1 October 29, 2024 02:37

beer-1 approved these changes Oct 29, 2024

View reviewed changes

[comments] Adressing Zi's comments, minor nits

410d58d

In addition to the comments, also force eviction of published modules from global cache. Enable V2 flag for some tests.

[nits] Multiple nits:

539b9a9

- Renamed all types/traits to `Legacy..` - Added TODOs for removal and type cache handling

This comment has been minimized.

Sign in to view

[loader] Enable V1 for merging

bf7b8fd

georgemitenkov removed the CICD:run-execution-performance-full-test Run execution performance test (full version) label Oct 29, 2024

georgemitenkov enabled auto-merge (squash) October 29, 2024 17:57

This comment has been minimized.

Sign in to view

georgemitenkov merged commit 751c9a3 into main Oct 29, 2024
77 of 97 checks passed

georgemitenkov deleted the george/main branch October 29, 2024 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce code cache and loader V2 #14184

Introduce code cache and loader V2 #14184

georgemitenkov commented Aug 1, 2024 •

edited

Loading

trunk-io bot commented Aug 1, 2024 •

edited

Loading

georgemitenkov commented Aug 1, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ziaptos left a comment

ziaptos Oct 29, 2024

georgemitenkov Oct 29, 2024

ziaptos Oct 29, 2024

georgemitenkov Oct 29, 2024

ziaptos Oct 29, 2024

georgemitenkov Oct 29, 2024

ziaptos Oct 29, 2024

georgemitenkov Oct 29, 2024

ziaptos Oct 29, 2024

georgemitenkov Oct 29, 2024

ziaptos Oct 29, 2024

beer-1 left a comment •

edited

Loading

georgemitenkov commented Oct 29, 2024

georgemitenkov commented Oct 29, 2024

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.

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

Introduce code cache and loader V2 #14184

Introduce code cache and loader V2 #14184

Conversation

georgemitenkov commented Aug 1, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Aug 1, 2024 • edited Loading

georgemitenkov commented Aug 1, 2024 • edited Loading

codecov bot commented Aug 1, 2024 • edited Loading

Codecov Report

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ziaptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beer-1 left a comment • edited Loading

Choose a reason for hiding this comment

georgemitenkov commented Oct 29, 2024

georgemitenkov commented Oct 29, 2024

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.

github-actions bot commented Oct 29, 2024

✅ Forge suite realistic_env_max_load success on bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4

github-actions bot commented Oct 29, 2024

✅ Forge suite framework_upgrade success on 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4

github-actions bot commented Oct 29, 2024

✅ Forge suite compat success on 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4

georgemitenkov commented Aug 1, 2024 •

edited

Loading

trunk-io bot commented Aug 1, 2024 •

edited

Loading

georgemitenkov commented Aug 1, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading

beer-1 left a comment •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4`

✅ Forge suite `framework_upgrade` success on `9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd` ==> `bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4`

✅ Forge suite `compat` success on `9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd` ==> `bf7b8fd8a36dd768f7fdeca5bcc8fa44a29f0fe4`