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] Refactor type caches in loader #15818

Merged
merged 2 commits into from
Feb 1, 2025
Merged

Conversation

georgemitenkov
Copy link
Contributor

Description

How Has This Been Tested?

Key Areas to Review

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)

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 27, 2025

⏱️ 12h 57m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
single-node-performance 8h 31m 🟥🟥🟥🟥🟥 (+15 more)
test-target-determinator 1h 37m 🟩🟩🟩🟩🟩 (+15 more)
execution-performance / single-node-performance 47m 🟩🟩
check-dynamic-deps 33m 🟩🟩🟩🟩🟩 (+8 more)
rust-targeted-unit-tests 25m 🟩
rust-cargo-deny 15m 🟩🟩🟩🟩🟩 (+5 more)
execution-performance / test-target-determinator 9m 🟩🟩
test-target-determinator 9m 🟩🟩
rust-doc-tests 7m 🟩
rust-doc-tests 7m 🟩
semgrep/ci 5m 🟩🟩🟩🟩🟩 (+8 more)
general-lints 4m 🟩🟩🟩🟩🟩 (+5 more)
fetch-last-released-docker-image-tag 4m 🟩🟩
file_change_determinator 2m 🟩🟩🟩🟩🟩 (+7 more)
permission-check 44s 🟩🟩🟩🟩🟩 (+8 more)

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

Job Duration vs 7d avg Delta
test-target-determinator 4m 5m -23%

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor Author

georgemitenkov commented Jan 27, 2025

@georgemitenkov georgemitenkov force-pushed the george/single-key-for-tag-cache branch from 4f0eae6 to cfa2edd Compare January 27, 2025 12:12
@georgemitenkov georgemitenkov force-pushed the george/refactor-ty-caches branch from aadca0d to a9d76f5 Compare January 27, 2025 12:12
@georgemitenkov georgemitenkov force-pushed the george/single-key-for-tag-cache branch from cfa2edd to c0ff83a Compare January 27, 2025 13:42
@georgemitenkov georgemitenkov force-pushed the george/refactor-ty-caches branch 2 times, most recently from b809fcc to bcdfaeb Compare January 29, 2025 14:51
@georgemitenkov georgemitenkov force-pushed the george/single-key-for-tag-cache branch from c0ff83a to 6b96cb5 Compare January 29, 2025 14:51
@georgemitenkov georgemitenkov marked this pull request as ready for review January 29, 2025 17:06
@georgemitenkov georgemitenkov force-pushed the george/refactor-ty-caches branch from bcdfaeb to 7c62f72 Compare January 29, 2025 17:21
Base automatically changed from george/single-key-for-tag-cache to main January 29, 2025 17:59
@georgemitenkov georgemitenkov force-pushed the george/refactor-ty-caches branch from 7c62f72 to 542fbdf Compare January 30, 2025 17:26
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.

Seems that this results in a performance regression? Do we have plans for mitigating that?

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.

Changes looks good to me. Was mostly wondering on how we are planning to work on the perf issues.

@georgemitenkov georgemitenkov enabled auto-merge (squash) February 1, 2025 00:13

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.

@georgemitenkov georgemitenkov enabled auto-merge (squash) February 1, 2025 01:11

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Feb 1, 2025

✅ Forge suite realistic_env_max_load success on 38246b8d21a43830a7cc9b4fc3920f262714fff5

two traffics test: inner traffic : committed: 12048.50 txn/s, submitted: 12050.67 txn/s, expired: 2.18 txn/s, latency: 3276.56 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 27700 ms), latency samples: 4581132
two traffics test : committed: 99.96 txn/s, latency: 1561.12 ms, (p50: 1500 ms, p70: 1600, p90: 1700 ms, p99: 2000 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.988, avg: 1.752", "ConsensusProposalToOrdered: max: 0.345, avg: 0.330", "ConsensusOrderedToCommit: max: 0.545, avg: 0.515", "ConsensusProposalToCommit: max: 0.876, avg: 0.845"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.31s no progress at version 19421 (avg 0.23s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.64s no progress at version 1659989 (avg 0.60s) [limit 16].
Test Ok

Copy link
Contributor

github-actions bot commented Feb 1, 2025

✅ Forge suite compat success on 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 38246b8d21a43830a7cc9b4fc3920f262714fff5

Compatibility test results for 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 38246b8d21a43830a7cc9b4fc3920f262714fff5 (PR)
1. Check liveness of validators at old version: 60f7ca8827f5d64a148c3b163dc4126b0879279b
compatibility::simple-validator-upgrade::liveness-check : committed: 9815.78 txn/s, latency: 3290.37 ms, (p50: 2700 ms, p70: 3500, p90: 5400 ms, p99: 6300 ms), latency samples: 334380
2. Upgrading first Validator to new version: 38246b8d21a43830a7cc9b4fc3920f262714fff5
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 3214.45 txn/s, latency: 9051.43 ms, (p50: 9700 ms, p70: 10900, p90: 11900 ms, p99: 12000 ms), latency samples: 70000
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3201.06 txn/s, latency: 10759.63 ms, (p50: 11600 ms, p70: 12400, p90: 12400 ms, p99: 12500 ms), latency samples: 125880
3. Upgrading rest of first batch to new version: 38246b8d21a43830a7cc9b4fc3920f262714fff5
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 1053.78 txn/s, submitted: 1053.96 txn/s, expired: 0.18 txn/s, latency: 9947.06 ms, (p50: 10700 ms, p70: 12200, p90: 12800 ms, p99: 13000 ms), latency samples: 64629
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 2980.17 txn/s, latency: 11247.77 ms, (p50: 12400 ms, p70: 13300, p90: 13400 ms, p99: 13400 ms), latency samples: 112640
4. upgrading second batch to new version: 38246b8d21a43830a7cc9b4fc3920f262714fff5
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 5263.73 txn/s, latency: 5697.49 ms, (p50: 6400 ms, p70: 6600, p90: 7800 ms, p99: 8000 ms), latency samples: 104500
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5148.89 txn/s, latency: 6540.37 ms, (p50: 6500 ms, p70: 7600, p90: 8000 ms, p99: 8300 ms), latency samples: 186160
5. check swarm health
Compatibility test for 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 38246b8d21a43830a7cc9b4fc3920f262714fff5 passed
Test Ok

Copy link
Contributor

github-actions bot commented Feb 1, 2025

✅ Forge suite framework_upgrade success on 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 38246b8d21a43830a7cc9b4fc3920f262714fff5

Compatibility test results for 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 38246b8d21a43830a7cc9b4fc3920f262714fff5 (PR)
Upgrade the nodes to version: 38246b8d21a43830a7cc9b4fc3920f262714fff5
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1500.17 txn/s, submitted: 1506.16 txn/s, failed submission: 5.99 txn/s, expired: 5.99 txn/s, latency: 2031.35 ms, (p50: 1800 ms, p70: 2200, p90: 3300 ms, p99: 4400 ms), latency samples: 135302
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1442.32 txn/s, submitted: 1448.07 txn/s, failed submission: 5.75 txn/s, expired: 5.75 txn/s, latency: 2516.68 ms, (p50: 1800 ms, p70: 2100, p90: 3600 ms, p99: 12600 ms), latency samples: 125400
5. check swarm health
Compatibility test for 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 38246b8d21a43830a7cc9b4fc3920f262714fff5 passed
Upgrade the remaining nodes to version: 38246b8d21a43830a7cc9b4fc3920f262714fff5
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1502.64 txn/s, submitted: 1505.81 txn/s, failed submission: 3.17 txn/s, expired: 3.17 txn/s, latency: 1933.48 ms, (p50: 1700 ms, p70: 2100, p90: 3100 ms, p99: 4300 ms), latency samples: 132540
Test Ok

@georgemitenkov georgemitenkov merged commit 20a11aa into main Feb 1, 2025
130 of 131 checks passed
@georgemitenkov georgemitenkov deleted the george/refactor-ty-caches branch February 1, 2025 01:49
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.

5 participants