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.

Sign up for GitHub

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

Jump to bottom

cherry-pick: remove usage of normalized types and improve module complexity check (main) #13935

Merged
merged 1 commit into from
Jul 8, 2024
Merged

cherry-pick: remove usage of normalized types and improve module complexity check (main) #13935

merged 1 commit into from
Jul 8, 2024

Conversation

vgao1996
Copy link
Contributor

@vgao1996 vgao1996 commented Jul 8, 2024

No description provided.

@vgao1996 @runtian-zhou @wrwg
cherry-pick: remove usage of normalized types and improve module comp…
34c2842 
…lexity check (aptos-labs#260)

* improve metadata check efficiency

* mark normalized types as deprecated

* improve module complexity check

* Update timed_features.rs

* Add activation time

* Update feature flag name

* [release-1.15] Deactivate broken package overrides (aptos-labs#13797)

Renaming the tomls for deactivating the tests

* Update timed_features.rs

---------

Co-authored-by: runtianz <runtian@aptoslabs.com>
Co-authored-by: Wolfgang Grieskamp <wg@aptoslabs.com>
@vgao1996 vgao1996 requested review from runtian-zhou and ziaptos July 8, 2024 18:49
@trunk-io Trunk.io
Copy link

trunk-io bot commented Jul 8, 2024 •
edited
Loading

⏱️ 3h 41m total CI duration on this PR
Job Cumulative Duration Recent Runs
test-fuzzers 1h 13m 🟩 🟩 
execution-performance / single-node-performance 30m 🟩 
forge-compat-test / forge 15m 🟩 
rust-move-tests 15m 🟩 
forge-framework-upgrade-test / forge 15m 🟩 
forge-e2e-test / forge 14m 🟩 
rust-move-unit-coverage 13m 🟩 
rust-images / rust-all 11m 🟩 
rust-move-tests 9m 🟩 
execution-performance / test-target-determinator 8m 🟩 
test-target-determinator 7m 🟩 
general-lints 4m 🟩 🟩 
check 3m 🟩 
check-dynamic-deps 2m 🟩 🟩 
semgrep/ci 54s 🟩 🟩 
file_change_determinator 23s 🟩 🟩 
file_change_determinator 20s 🟩 🟩 
permission-check 12s 🟩 🟩 
file_change_determinator 9s 🟩 
permission-check 4s 🟩 🟩 
determine-docker-build-metadata 4s 🟩 
permission-check 4s 🟩 🟩 
permission-check 4s 🟩 🟩 
permission-check 2s 🟩 

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

Job Duration vs 7d avg Delta
execution-performance / test-target-determinator 8m 5m +54%
test-target-determinator 7m 5m +47%
execution-performance / single-node-performance 30m 21m +43%

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

runtian-zhou
runtian-zhou reviewed Jul 8, 2024
View reviewed changes
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.

Was wondering if we can remove the timed feature flag in the main branch to make code a bit cleaner. I'm fine landing it as is but feels like a bit of extra clean up might be better.

@vgao1996 vgao1996 requested a review from sherry-x July 8, 2024 18:54
@vgao1996
Copy link
Contributor Author

vgao1996 commented Jul 8, 2024

@runtian-zhou was thinking about the same thing. IMO it's better to clean up the timed feature flag later since

  1. We'll need to run replay to confirm the removal is safe
  2. This is blocking the 1.16 release

@runtian-zhou
Copy link
Contributor

I'm suggesting for 1.16 cherry pick we just land as is. We should probably clean it up on main branch PR.

@vgao1996
Copy link
Contributor Author

vgao1996 commented Jul 8, 2024 •
edited
Loading

@runtian-zhou I think it's better we get all the cherry-picks landed at the same time for easy tracking and consistency.

The presence of the timed feature flag is a minor code cleanness issue and we can take time with it. The 1.17 branch cut may also happen soon. We don't want to make this a blocker.

@runtian-zhou
Copy link
Contributor

Would be concerned if we never get time to clean up those code but I agree that blocking release is not good either. I'll stamp it for now.

@vgao1996
Copy link
Contributor Author

vgao1996 commented Jul 8, 2024

I see your concern. I'll start a replay task today.

@vgao1996 vgao1996 enabled auto-merge (squash) July 8, 2024 21:39
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 8, 2024

✅ Forge suite realistic_env_max_load success on 34c28423aba4139505e4c760510229df26f1d06a

two traffics test: inner traffic : committed: 8372.659682654652 txn/s, latency: 4682.454892576342 ms, (p50: 4500 ms, p90: 6000 ms, p99: 10200 ms), latency samples: 3614660
two traffics test : committed: 100.022575854567 txn/s, latency: 2057.765 ms, (p50: 2000 ms, p90: 2300 ms, p99: 3300 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.223, avg: 0.208", "QsPosToProposal: max: 0.111, avg: 0.107", "ConsensusProposalToOrdered: max: 0.312, avg: 0.288", "ConsensusOrderedToCommit: max: 0.390, avg: 0.371", "ConsensusProposalToCommit: max: 0.674, avg: 0.659"]
Max round gap was 1 [limit 4] at version 1781786. Max no progress secs was 4.731902 [limit 15] at version 1781786.
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 8, 2024

✅ Forge suite framework_upgrade success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 34c28423aba4139505e4c760510229df26f1d06a

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 34c28423aba4139505e4c760510229df26f1d06a (PR)
Upgrade the nodes to version: 34c28423aba4139505e4c760510229df26f1d06a
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1106.2424564378707 txn/s, submitted: 1108.8789826498378 txn/s, failed submission: 2.63652621196712 txn/s, expired: 2.63652621196712 txn/s, latency: 2840.442075471698 ms, (p50: 2100 ms, p90: 5100 ms, p99: 10500 ms), latency samples: 100700
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1070.5554412889596 txn/s, submitted: 1073.2340620761713 txn/s, failed submission: 2.678620787211742 txn/s, expired: 2.678620787211742 txn/s, latency: 2734.658559216013 ms, (p50: 1800 ms, p90: 4800 ms, p99: 12000 ms), latency samples: 95920
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 34c28423aba4139505e4c760510229df26f1d06a passed
Upgrade the remaining nodes to version: 34c28423aba4139505e4c760510229df26f1d06a
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1052.6740981977584 txn/s, submitted: 1054.345767591448 txn/s, failed submission: 1.6716693936897251 txn/s, expired: 1.6716693936897251 txn/s, latency: 3046.39681261343 ms, (p50: 2100 ms, p90: 5400 ms, p99: 12400 ms), latency samples: 88160
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 8, 2024

✅ Forge suite compat success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 34c28423aba4139505e4c760510229df26f1d06a

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 34c28423aba4139505e4c760510229df26f1d06a (PR)
1. Check liveness of validators at old version: 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5
compatibility::simple-validator-upgrade::liveness-check : committed: 7905.147342714089 txn/s, latency: 3909.9351222610353 ms, (p50: 3000 ms, p90: 4200 ms, p99: 28700 ms), latency samples: 314900
2. Upgrading first Validator to new version: 34c28423aba4139505e4c760510229df26f1d06a
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 3132.9144775638983 txn/s, latency: 8399.980379746836 ms, (p50: 9300 ms, p90: 9900 ms, p99: 10300 ms), latency samples: 75840
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3182.8033681587563 txn/s, latency: 9774.564016191991 ms, (p50: 9700 ms, p90: 14800 ms, p99: 15100 ms), latency samples: 138340
3. Upgrading rest of first batch to new version: 34c28423aba4139505e4c760510229df26f1d06a
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 1409.0898129742577 txn/s, latency: 20709.642419928827 ms, (p50: 26500 ms, p90: 29700 ms, p99: 30500 ms), latency samples: 56200
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3380.4593274264744 txn/s, latency: 9103.597147302904 ms, (p50: 10000 ms, p90: 13600 ms, p99: 14200 ms), latency samples: 134960
4. upgrading second batch to new version: 34c28423aba4139505e4c760510229df26f1d06a
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 6615.501517721884 txn/s, latency: 4218.70740491165 ms, (p50: 4800 ms, p90: 5200 ms, p99: 5500 ms), latency samples: 133560
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5726.763545438303 txn/s, latency: 5336.5873824382525 ms, (p50: 5100 ms, p90: 6500 ms, p99: 9600 ms), latency samples: 227540
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 34c28423aba4139505e4c760510229df26f1d06a passed
Test Ok

@vgao1996 vgao1996 merged commit 20b7f3a into aptos-labs:main Jul 8, 2024
89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ziaptos ziaptos ziaptos approved these changes

@runtian-zhou runtian-zhou runtian-zhou approved these changes

@davidiw davidiw Awaiting requested review from davidiw davidiw is a code owner

@movekevin movekevin Awaiting requested review from movekevin movekevin is a code owner

@wrwg wrwg Awaiting requested review from wrwg wrwg is a code owner

@zekun000 zekun000 Awaiting requested review from zekun000 zekun000 is a code owner

@georgemitenkov georgemitenkov Awaiting requested review from georgemitenkov georgemitenkov is a code owner

@sherry-x sherry-x Awaiting requested review from sherry-x

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vgao1996 @runtian-zhou @ziaptos