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

avoid TransactionToCommit #14999

Merged
merged 1 commit into from
Oct 18, 2024
Merged

avoid TransactionToCommit #14999

merged 1 commit into from
Oct 18, 2024

Conversation

msmouse
Copy link
Contributor

@msmouse msmouse commented Oct 17, 2024

Description

  1. avoid unnecessary packing and unpacking, it's desirable because I'm
    gonna make the PartialStateComputeResult consist of multiple
    immutable stage outputs.
  2. extract DoLedgerUpdate algorithm

How Has This Been Tested?

existing coverage

Key Areas to Review

Type of Change

  • Refactoring

Which Components or Systems Does This Change Impact?

  • Validator Node

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 Oct 17, 2024

⏱️ 4h 30m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 1h 9m 🟩🟩🟩
rust-targeted-unit-tests 16m 🟩
forge-e2e-test / forge 15m 🟥
rust-cargo-deny 14m 🟩🟩🟩🟩🟩 (+3 more)
check-dynamic-deps 14m 🟩🟩🟩🟩🟩 (+4 more)
test-target-determinator 13m 🟩🟩🟩
execution-performance / test-target-determinator 13m 🟩🟩🟩
check 11m 🟩🟩🟩
rust-move-tests 10m
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 9m 🟩
rust-move-tests 9m 🟩
rust-move-tests 9m 🟩

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

Job Duration vs 7d avg Delta
execution-performance / single-node-performance 23m 18m +31%

settingsfeedbackdocs ⋅ learn more about trunk.io

@msmouse msmouse force-pushed the 1017-alden-remove-txn-to-commit branch from e5e79fc to 247545f Compare October 17, 2024 22:53
Base automatically changed from 1016-alden-chunk-to-commit to main October 17, 2024 23:09
@msmouse msmouse force-pushed the 1017-alden-remove-txn-to-commit branch 2 times, most recently from 2d7e0f0 to 6c22139 Compare October 17, 2024 23:42
@msmouse msmouse changed the title remove TransactionToCommit avoid TransactionToCommit Oct 17, 2024
@msmouse msmouse marked this pull request as ready for review October 17, 2024 23:43
@msmouse msmouse force-pushed the 1017-alden-remove-txn-to-commit branch 2 times, most recently from c125ffe to 939bf46 Compare October 18, 2024 00:11
@msmouse msmouse added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Oct 18, 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.

1. avoid unnecessary packing and unpacking, it's desirable because I'm
   gonna make the PartialStateComputeResult consist of multiple
   immutable stage outputs.
2. extract DoLedgerUpdate algorithm
@msmouse msmouse force-pushed the 1017-alden-remove-txn-to-commit branch from 939bf46 to 20f2fde Compare October 18, 2024 18:55

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@msmouse msmouse enabled auto-merge (squash) October 18, 2024 19:42

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 20f2fdecee25d002bd415b4435cff0a1aeacdf35

two traffics test: inner traffic : committed: 11804.59 txn/s, latency: 3363.15 ms, (p50: 2700 ms, p70: 2900, p90: 3200 ms, p99: 14400 ms), latency samples: 4488420
two traffics test : committed: 99.99 txn/s, latency: 1783.09 ms, (p50: 1600 ms, p70: 1700, p90: 1900 ms, p99: 8400 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.252, avg: 0.225", "QsPosToProposal: max: 1.048, avg: 1.012", "ConsensusProposalToOrdered: max: 0.347, avg: 0.332", "ConsensusOrderedToCommit: max: 0.598, avg: 0.530", "ConsensusProposalToCommit: max: 0.924, avg: 0.862"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.29s no progress at version 4489415 (avg 0.24s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 7.59s no progress at version 848911 (avg 7.36s) [limit 15].
Test Ok

Copy link
Contributor

✅ Forge suite compat success on b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> 20f2fdecee25d002bd415b4435cff0a1aeacdf35

Compatibility test results for b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> 20f2fdecee25d002bd415b4435cff0a1aeacdf35 (PR)
1. Check liveness of validators at old version: b29f09f57e898d8d211c8bc3e303f6e50bba2266
compatibility::simple-validator-upgrade::liveness-check : committed: 12955.63 txn/s, latency: 2255.24 ms, (p50: 1800 ms, p70: 2100, p90: 2400 ms, p99: 20200 ms), latency samples: 509000
2. Upgrading first Validator to new version: 20f2fdecee25d002bd415b4435cff0a1aeacdf35
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 5517.37 txn/s, latency: 5196.64 ms, (p50: 5700 ms, p70: 6200, p90: 6700 ms, p99: 6900 ms), latency samples: 112300
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 5490.87 txn/s, latency: 5933.25 ms, (p50: 6500 ms, p70: 6700, p90: 7100 ms, p99: 7900 ms), latency samples: 191840
3. Upgrading rest of first batch to new version: 20f2fdecee25d002bd415b4435cff0a1aeacdf35
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 5124.06 txn/s, latency: 5584.92 ms, (p50: 5900 ms, p70: 6800, p90: 6900 ms, p99: 7100 ms), latency samples: 99000
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 5037.55 txn/s, latency: 6411.50 ms, (p50: 7100 ms, p70: 7300, p90: 8300 ms, p99: 8900 ms), latency samples: 168820
4. upgrading second batch to new version: 20f2fdecee25d002bd415b4435cff0a1aeacdf35
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 7412.87 txn/s, latency: 3660.36 ms, (p50: 4000 ms, p70: 4100, p90: 4500 ms, p99: 4800 ms), latency samples: 138560
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 7324.48 txn/s, latency: 3858.70 ms, (p50: 3900 ms, p70: 4100, p90: 4400 ms, p99: 6400 ms), latency samples: 277480
5. check swarm health
Compatibility test for b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> 20f2fdecee25d002bd415b4435cff0a1aeacdf35 passed
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> 20f2fdecee25d002bd415b4435cff0a1aeacdf35

Compatibility test results for b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> 20f2fdecee25d002bd415b4435cff0a1aeacdf35 (PR)
Upgrade the nodes to version: 20f2fdecee25d002bd415b4435cff0a1aeacdf35
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1205.86 txn/s, submitted: 1207.69 txn/s, failed submission: 1.82 txn/s, expired: 1.82 txn/s, latency: 2746.01 ms, (p50: 2400 ms, p70: 2700, p90: 4200 ms, p99: 5700 ms), latency samples: 105840
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1266.57 txn/s, submitted: 1268.70 txn/s, failed submission: 2.13 txn/s, expired: 2.13 txn/s, latency: 2521.71 ms, (p50: 2400 ms, p70: 2700, p90: 3600 ms, p99: 6200 ms), latency samples: 106940
5. check swarm health
Compatibility test for b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> 20f2fdecee25d002bd415b4435cff0a1aeacdf35 passed
Upgrade the remaining nodes to version: 20f2fdecee25d002bd415b4435cff0a1aeacdf35
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1197.34 txn/s, submitted: 1199.99 txn/s, failed submission: 2.66 txn/s, expired: 2.66 txn/s, latency: 2545.02 ms, (p50: 2400 ms, p70: 2700, p90: 4100 ms, p99: 5600 ms), latency samples: 108200
Test Ok

@msmouse msmouse merged commit 79d5257 into main Oct 18, 2024
93 checks passed
@msmouse msmouse deleted the 1017-alden-remove-txn-to-commit branch October 18, 2024 20:16
msmouse added a commit that referenced this pull request Oct 18, 2024
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.

3 participants