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

Change Payload.len() and Payload.empty() to indicate actual payload size instead of max_txns_to_execute #12696

Merged
merged 2 commits into from Apr 4, 2024
Merged

Change Payload.len() and Payload.empty() to indicate actual payload size instead of max_txns_to_execute #12696

merged 2 commits into from Apr 4, 2024

Conversation

manudhundi
Copy link
Contributor

@manudhundi manudhundi commented Mar 27, 2024 •
edited

Description

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
  • Other (specify)

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 Trunk.io
Copy link

trunk-io bot commented Mar 27, 2024 •
edited

⏱️ 1h 16m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-unit-tests 25m 🟩 
rust-move-tests 14m 🟩 ⬜ 
windows-build 12m 🟩 
run-tests-main-branch 9m 🟩 🟩 
rust-lints 7m 🟩 
check 4m 🟩 
check-dynamic-deps 3m 🟩 
general-lints 2m 🟩 
semgrep/ci 24s 🟩 
permission-check 15s 🟩 
file_change_determinator 10s 🟩 
file_change_determinator 9s 🟩 
permission-check 5s 🟩 🟩 
permission-check 4s 🟩 
permission-check 3s 🟩 

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

Job Duration vs 7d avg Delta
windows-build 12m 21m -42%

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

@manudhundi manudhundi self-assigned this Mar 27, 2024
@manudhundi manudhundi marked this pull request as ready for review March 27, 2024 00:41
sitalkedia
sitalkedia approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@sitalkedia sitalkedia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add some comments in the code explaining the reasoning behind the change and also the new semantic for len and empty

@manudhundi manudhundi force-pushed the manu/payload_len_fix branch 2 times, most recently from 5153442 to d8ec3c1 Compare April 3, 2024 23:54
@manudhundi manudhundi enabled auto-merge (squash) April 4, 2024 15:30
@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 Apr 4, 2024

✅ Forge suite compat success on aptos-node-v1.10.1 ==> e7fb998ccf3cbbd9227a177f67b5fc339447a79c

Compatibility test results for aptos-node-v1.10.1 ==> e7fb998ccf3cbbd9227a177f67b5fc339447a79c (PR)
1. Check liveness of validators at old version: aptos-node-v1.10.1
compatibility::simple-validator-upgrade::liveness-check : committed: 6185 txn/s, latency: 5018 ms, (p50: 5000 ms, p90: 6400 ms, p99: 8400 ms), latency samples: 235040
2. Upgrading first Validator to new version: e7fb998ccf3cbbd9227a177f67b5fc339447a79c
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1769 txn/s, latency: 15792 ms, (p50: 19300 ms, p90: 22400 ms, p99: 23100 ms), latency samples: 90220
3. Upgrading rest of first batch to new version: e7fb998ccf3cbbd9227a177f67b5fc339447a79c
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1669 txn/s, latency: 17311 ms, (p50: 19200 ms, p90: 23300 ms, p99: 23500 ms), latency samples: 88480
4. upgrading second batch to new version: e7fb998ccf3cbbd9227a177f67b5fc339447a79c
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 2986 txn/s, latency: 10438 ms, (p50: 11800 ms, p90: 12600 ms, p99: 13000 ms), latency samples: 119460
5. check swarm health
Compatibility test for aptos-node-v1.10.1 ==> e7fb998ccf3cbbd9227a177f67b5fc339447a79c passed
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 4, 2024

✅ Forge suite realistic_env_max_load success on e7fb998ccf3cbbd9227a177f67b5fc339447a79c

two traffics test: inner traffic : committed: 8067 txn/s, latency: 4868 ms, (p50: 4700 ms, p90: 5700 ms, p99: 9600 ms), latency samples: 3485200
two traffics test : committed: 100 txn/s, latency: 1829 ms, (p50: 1800 ms, p90: 2100 ms, p99: 2500 ms), latency samples: 1720
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.210, avg: 0.202", "QsPosToProposal: max: 0.260, avg: 0.237", "ConsensusProposalToOrdered: max: 0.442, avg: 0.404", "ConsensusOrderedToCommit: max: 0.324, avg: 0.308", "ConsensusProposalToCommit: max: 0.724, avg: 0.712"]
Max round gap was 1 [limit 4] at version 1349551. Max no progress secs was 4.504404 [limit 15] at version 1349551.
Test Ok

@manudhundi manudhundi merged commit ac71277 into main Apr 4, 2024
97 of 98 checks passed
@manudhundi manudhundi deleted the manu/payload_len_fix branch April 4, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vusirikala vusirikala vusirikala approved these changes

@sitalkedia sitalkedia sitalkedia approved these changes

@igor-aptos igor-aptos Awaiting requested review from igor-aptos

@bchocho bchocho Awaiting requested review from bchocho

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

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

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

Assignees

@manudhundi manudhundi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@manudhundi @vusirikala @sitalkedia