[loader-v2] Small cleanups & tests #15279

georgemitenkov · 2024-11-14T21:30:01Z

Description

This PR fixes few minor things:

Fixed some naming as requested by @gelash: valid entries in global module cache are now overridden. Had to invert conditions and add is_not_overridden APIs. Updated comments in a few places and removed forgotten _unsync in function names.
Added more counters, moved some old ones.
Added unit tests for TransactionSliceMetadata: it is good to have better coverage for these simple cases.

How Has This Been Tested?

Added new tests, + existing tests.

Type of Change

Refactoring
Tests

Which Components or Systems Does This Change Impact?

Move/Aptos Virtual Machine

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-11-14T21:30:05Z

⏱️ 6h 10m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	1h 3m	🟥 🟩 🟥 🟩
rust-cargo-deny	32m	🟩 ⬜ 🟩 🟩 🟩 (+15 more)
check-dynamic-deps	23m	🟩 🟩 🟩 🟩 🟩 (+15 more)
test-target-determinator	13m	🟩 🟩 🟩
rust-move-tests	13m	🟩
execution-performance / test-target-determinator	13m	🟩 🟩 🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-move-tests	11m	⬜

🚨 1 job on the last run was significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
execution-performance / single-node-performance	25m	17m

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

georgemitenkov · 2024-11-14T21:30:21Z

[loader-v2] Small cleanups & tests #15279 : 2 dependent PRs (#15280 , #15316 ) 👈 (View in Graphite)
[loader-v2] Fixing global cache reads & read-before-write on publish #15285
main

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

gelash · 2024-11-20T18:38:44Z

aptos-move/block-executor/src/code_cache_global.rs

-    ///   2. Valid modules should not be removed, and new modules should have unique ownership. If
-    ///      these constraints are violated, a panic error is returned.
-    pub fn insert_verified_unsync(
+    ///   2. Not overridden modules should not be removed, and new modules should have unique


where do we check unique ownership?

I think it's currently cloned for MVHashMap, so the comment will only be true once we remove the clone (there is a TODO)

- Fixed naming for global module cache. - Added more counters, moved some old ones. - Added unit tests for TransactionSliceMetadata + renaming. (cherry picked from commit 47f0bf3)

github-actions · 2024-11-20T22:28:02Z

💚 All backports created successfully

Status	Branch	Result
✅	aptos-release-v1.24

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

github-actions · 2024-11-20T22:54:19Z

✅ Forge suite `realistic_env_max_load` success on `ba3f9f53d15f969b9aba440406bd04ccbc58d5c9`

two traffics test: inner traffic : committed: 14439.62 txn/s, latency: 2753.85 ms, (p50: 2700 ms, p70: 2700, p90: 2900 ms, p99: 3000 ms), latency samples: 5490240
two traffics test : committed: 99.90 txn/s, latency: 1450.18 ms, (p50: 1400 ms, p70: 1400, p90: 1500 ms, p99: 6100 ms), latency samples: 1860
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.007, avg: 1.549", "ConsensusProposalToOrdered: max: 0.322, avg: 0.292", "ConsensusOrderedToCommit: max: 0.380, avg: 0.362", "ConsensusProposalToCommit: max: 0.668, avg: 0.655"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.13s no progress at version 2916695 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.52s no progress at version 2916693 (avg 8.52s) [limit 15].
Test Ok

github-actions · 2024-11-20T22:55:51Z

✅ Forge suite `compat` success on `df3d8058768fe607ead6d0e9f60e954977d7b0d4` ==> `ba3f9f53d15f969b9aba440406bd04ccbc58d5c9`

Compatibility test results for df3d8058768fe607ead6d0e9f60e954977d7b0d4 ==> ba3f9f53d15f969b9aba440406bd04ccbc58d5c9 (PR)
1. Check liveness of validators at old version: df3d8058768fe607ead6d0e9f60e954977d7b0d4
compatibility::simple-validator-upgrade::liveness-check : committed: 17219.12 txn/s, latency: 1970.46 ms, (p50: 1900 ms, p70: 2100, p90: 2300 ms, p99: 3000 ms), latency samples: 559420
2. Upgrading first Validator to new version: ba3f9f53d15f969b9aba440406bd04ccbc58d5c9
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7548.47 txn/s, latency: 3727.64 ms, (p50: 4200 ms, p70: 4500, p90: 4600 ms, p99: 4700 ms), latency samples: 137240
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6943.15 txn/s, latency: 4610.70 ms, (p50: 4600 ms, p70: 4700, p90: 6700 ms, p99: 6900 ms), latency samples: 233320
3. Upgrading rest of first batch to new version: ba3f9f53d15f969b9aba440406bd04ccbc58d5c9
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7836.44 txn/s, latency: 3626.51 ms, (p50: 4100 ms, p70: 4200, p90: 4300 ms, p99: 4400 ms), latency samples: 144680
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7948.46 txn/s, latency: 4055.31 ms, (p50: 4300 ms, p70: 4400, p90: 5600 ms, p99: 5900 ms), latency samples: 261660
4. upgrading second batch to new version: ba3f9f53d15f969b9aba440406bd04ccbc58d5c9
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11755.26 txn/s, latency: 2365.15 ms, (p50: 2500 ms, p70: 2600, p90: 3100 ms, p99: 3400 ms), latency samples: 203900
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10282.80 txn/s, latency: 3006.49 ms, (p50: 2600 ms, p70: 2800, p90: 5700 ms, p99: 7500 ms), latency samples: 343020
5. check swarm health
Compatibility test for df3d8058768fe607ead6d0e9f60e954977d7b0d4 ==> ba3f9f53d15f969b9aba440406bd04ccbc58d5c9 passed
Test Ok

github-actions · 2024-11-20T22:56:08Z

✅ Forge suite `framework_upgrade` success on `df3d8058768fe607ead6d0e9f60e954977d7b0d4` ==> `ba3f9f53d15f969b9aba440406bd04ccbc58d5c9`

Compatibility test results for df3d8058768fe607ead6d0e9f60e954977d7b0d4 ==> ba3f9f53d15f969b9aba440406bd04ccbc58d5c9 (PR)
Upgrade the nodes to version: ba3f9f53d15f969b9aba440406bd04ccbc58d5c9
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1421.38 txn/s, submitted: 1424.57 txn/s, failed submission: 3.19 txn/s, expired: 3.19 txn/s, latency: 2168.35 ms, (p50: 2000 ms, p70: 2100, p90: 3600 ms, p99: 5000 ms), latency samples: 124580
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1459.93 txn/s, submitted: 1463.74 txn/s, failed submission: 3.81 txn/s, expired: 3.81 txn/s, latency: 2089.22 ms, (p50: 1800 ms, p70: 2100, p90: 3000 ms, p99: 5200 ms), latency samples: 130400
5. check swarm health
Compatibility test for df3d8058768fe607ead6d0e9f60e954977d7b0d4 ==> ba3f9f53d15f969b9aba440406bd04ccbc58d5c9 passed
Upgrade the remaining nodes to version: ba3f9f53d15f969b9aba440406bd04ccbc58d5c9
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1372.09 txn/s, submitted: 1374.29 txn/s, failed submission: 2.20 txn/s, expired: 2.20 txn/s, latency: 2176.40 ms, (p50: 2100 ms, p70: 2400, p90: 3100 ms, p99: 4600 ms), latency samples: 124640
Test Ok

- Fixed naming for global module cache. - Added more counters, moved some old ones. - Added unit tests for TransactionSliceMetadata + renaming. (cherry picked from commit 47f0bf3) Co-authored-by: George Mitenkov <[email protected]>

georgemitenkov mentioned this pull request Nov 14, 2024

[loader-v2] Refactor how environment is stored in unsync storages and remove V1 from tests #15280

Closed

8 tasks

georgemitenkov changed the title ~~Renaming: - valid entries --> not overriden - updated comments - removed _unsync~~ [loader_v2]Re-use executor for replay, small cleanups & tests Nov 14, 2024

georgemitenkov force-pushed the george/loader-v2-todos branch 3 times, most recently from b0be60f to 9d0662e Compare November 14, 2024 21:53

georgemitenkov marked this pull request as ready for review November 14, 2024 21:55

georgemitenkov requested review from msmouse, lightmark, grao1991, gelash, zekun000, sasha8, danielxiangzl, davidiw, wrwg and vgao1996 as code owners November 14, 2024 21:56

georgemitenkov requested review from igor-aptos and ziaptos and removed request for davidiw, lightmark, msmouse, grao1991, sasha8, vgao1996, wrwg and danielxiangzl November 14, 2024 21:56

georgemitenkov changed the title ~~[loader_v2]Re-use executor for replay, small cleanups & tests~~ [loader_v2] Small cleanups & tests Nov 14, 2024

georgemitenkov changed the title ~~[loader_v2] Small cleanups & tests~~ [loader-v2] Small cleanups & tests Nov 14, 2024

address comments

ba3f9f5

georgemitenkov force-pushed the george/loader-v2-todos branch from 1926f11 to ba3f9f5 Compare November 20, 2024 18:06

georgemitenkov enabled auto-merge (squash) November 20, 2024 18:07

This comment has been minimized.

Sign in to view

gelash approved these changes Nov 20, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

georgemitenkov merged commit 47f0bf3 into main Nov 20, 2024
87 of 92 checks passed

georgemitenkov deleted the george/loader-v2-todos branch November 20, 2024 20:34

georgemitenkov added the 1.24 label Nov 20, 2024

This comment has been minimized.

Sign in to view

georgemitenkov added the v1.24 label Nov 20, 2024

github-actions bot mentioned this pull request Nov 20, 2024

[cp][aptos-release-v1.24] [loader-v2] Small cleanups & tests #15343

Merged

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[loader-v2] Small cleanups & tests #15279

[loader-v2] Small cleanups & tests #15279

georgemitenkov commented Nov 14, 2024 •

edited

Loading

trunk-io bot commented Nov 14, 2024 •

edited

Loading

georgemitenkov commented Nov 14, 2024 •

edited

Loading

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.

gelash Nov 20, 2024

georgemitenkov Nov 20, 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 Nov 20, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

[loader-v2] Small cleanups & tests #15279

[loader-v2] Small cleanups & tests #15279

Conversation

georgemitenkov commented Nov 14, 2024 • edited Loading

Description

How Has This Been Tested?

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Nov 14, 2024 • edited Loading

georgemitenkov commented Nov 14, 2024 • edited Loading

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.

gelash Nov 20, 2024

Choose a reason for hiding this comment

georgemitenkov Nov 20, 2024

Choose a reason for hiding this comment

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 Nov 20, 2024

💚 All backports created successfully

Questions ?

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 20, 2024

✅ Forge suite realistic_env_max_load success on ba3f9f53d15f969b9aba440406bd04ccbc58d5c9

github-actions bot commented Nov 20, 2024

✅ Forge suite compat success on df3d8058768fe607ead6d0e9f60e954977d7b0d4 ==> ba3f9f53d15f969b9aba440406bd04ccbc58d5c9

github-actions bot commented Nov 20, 2024

✅ Forge suite framework_upgrade success on df3d8058768fe607ead6d0e9f60e954977d7b0d4 ==> ba3f9f53d15f969b9aba440406bd04ccbc58d5c9

georgemitenkov commented Nov 14, 2024 •

edited

Loading

trunk-io bot commented Nov 14, 2024 •

edited

Loading

georgemitenkov commented Nov 14, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `ba3f9f53d15f969b9aba440406bd04ccbc58d5c9`

✅ Forge suite `compat` success on `df3d8058768fe607ead6d0e9f60e954977d7b0d4` ==> `ba3f9f53d15f969b9aba440406bd04ccbc58d5c9`

✅ Forge suite `framework_upgrade` success on `df3d8058768fe607ead6d0e9f60e954977d7b0d4` ==> `ba3f9f53d15f969b9aba440406bd04ccbc58d5c9`