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

[State Sync] Adopt state value iterator usage. #13333

Closed
wants to merge 1 commit into from

Conversation

JoshLind
Copy link
Contributor

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

Copy link

trunk-io bot commented May 18, 2024

⏱️ 17h 26m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-smoke-tests 3h 15m 🟩🟩🟩🟥 (+2 more)
execution-performance / single-node-performance 2h 49m 🟩🟩🟩🟩🟩 (+2 more)
rust-targeted-unit-tests 2h 18m 🟥🟥🟥🟥🟥 (+2 more)
rust-images / rust-all 1h 34m 🟩🟩🟩🟩 (+3 more)
forge-e2e-test / forge 1h 24m 🟩🟩🟩🟩🟩 (+1 more)
forge-compat-test / forge 1h 18m 🟩🟩🟩🟩🟩 (+1 more)
rust-move-tests 1h 4m 🟩🟩🟩🟩🟩 (+3 more)
cli-e2e-tests / run-cli-tests 51m 🟩🟥🟥🟥🟥 (+1 more)
rust-lints 36m 🟩🟩🟩🟩🟩 (+2 more)
run-tests-main-branch 33m 🟩🟩🟩🟩🟩 (+3 more)
check 28m 🟩🟩🟩🟩🟩 (+2 more)
rust-build-cached-packages 25m 🟩🟩🟩🟩🟩 (+2 more)
general-lints 13m 🟩🟩🟩🟩🟩 (+2 more)
check-dynamic-deps 11m 🟩🟩🟩🟩🟩 (+3 more)
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose 11m 🟩🟩🟩🟩🟥 (+1 more)
node-api-compatibility-tests / node-api-compatibility-tests 5m 🟩🟩🟩🟩🟩 (+1 more)
semgrep/ci 3m 🟩🟩🟩🟩🟩 (+3 more)
file_change_determinator 2m 🟩🟩🟩🟩🟩 (+4 more)
file_change_determinator 1m 🟩🟩🟩🟩🟩 (+3 more)
file_change_determinator 1m 🟩🟩🟩🟩🟩 (+3 more)
execution-performance / file_change_determinator 1m 🟩🟩🟩🟩🟩 (+2 more)
permission-check 34s 🟩🟩🟩🟩🟩 (+4 more)
permission-check 24s 🟩🟩🟩🟩🟩 (+4 more)
permission-check 22s 🟩🟩🟩🟩🟩 (+3 more)
permission-check 20s 🟩🟩🟩🟩🟩 (+4 more)
permission-check 20s 🟩🟩🟩🟩🟩 (+4 more)
determine-docker-build-metadata 17s 🟩🟩🟩🟩🟩 (+3 more)

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

Job Duration vs 7d avg Delta
cli-e2e-tests / run-cli-tests 9m 6m +44%
rust-targeted-unit-tests 27m 19m +40%

settingsfeedbackdocs ⋅ learn more about trunk.io

@JoshLind JoshLind added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label May 18, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@JoshLind JoshLind force-pushed the use_storage_iter branch 2 times, most recently from 345a7ac to 3b46463 Compare May 18, 2024 09:45

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.

This comment has been minimized.

@msmouse msmouse changed the base branch from aptos-release-v1.12 to main May 19, 2024 07:26
@msmouse
Copy link
Contributor

msmouse commented May 19, 2024

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

Join @JoshLind and the rest of your teammates on Graphite Graphite

@msmouse msmouse mentioned this pull request May 19, 2024
21 tasks
/// Serializes the given data and returns the number of serialized bytes
fn get_num_serialized_bytes<T: ?Sized + Serialize>(
data: &T,
) -> aptos_storage_service_types::Result<u64, Error> {
let num_serialized_bytes = bcs::to_bytes(&data)
Copy link
Contributor

Choose a reason for hiding this comment

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

can probably use bcs::serialized_size

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@JoshLind JoshLind changed the base branch from main to aptos-release-v1.12 May 19, 2024 08:00

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on 01b24e7e3548382dd25440b39a0438a993387f12 ==> df638a1f18fbead95da1f33dbc8cea6619b70382

Compatibility test results for 01b24e7e3548382dd25440b39a0438a993387f12 ==> df638a1f18fbead95da1f33dbc8cea6619b70382 (PR)
1. Check liveness of validators at old version: 01b24e7e3548382dd25440b39a0438a993387f12
compatibility::simple-validator-upgrade::liveness-check : committed: 6213 txn/s, latency: 5014 ms, (p50: 4800 ms, p90: 8700 ms, p99: 10500 ms), latency samples: 242340
2. Upgrading first Validator to new version: df638a1f18fbead95da1f33dbc8cea6619b70382
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1746 txn/s, latency: 15613 ms, (p50: 18700 ms, p90: 21700 ms, p99: 22700 ms), latency samples: 90840
3. Upgrading rest of first batch to new version: df638a1f18fbead95da1f33dbc8cea6619b70382
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1717 txn/s, latency: 16906 ms, (p50: 19100 ms, p90: 22600 ms, p99: 22900 ms), latency samples: 89300
4. upgrading second batch to new version: df638a1f18fbead95da1f33dbc8cea6619b70382
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3534 txn/s, latency: 8870 ms, (p50: 9600 ms, p90: 12600 ms, p99: 13000 ms), latency samples: 144920
5. check swarm health
Compatibility test for 01b24e7e3548382dd25440b39a0438a993387f12 ==> df638a1f18fbead95da1f33dbc8cea6619b70382 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on df638a1f18fbead95da1f33dbc8cea6619b70382

two traffics test: inner traffic : committed: 7957 txn/s, latency: 4931 ms, (p50: 4800 ms, p90: 5700 ms, p99: 10200 ms), latency samples: 3437780
two traffics test : committed: 100 txn/s, latency: 1871 ms, (p50: 1900 ms, p90: 2100 ms, p99: 2300 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.208, avg: 0.203", "QsPosToProposal: max: 0.247, avg: 0.227", "ConsensusProposalToOrdered: max: 0.474, avg: 0.430", "ConsensusOrderedToCommit: max: 0.406, avg: 0.385", "ConsensusProposalToCommit: max: 0.830, avg: 0.815"]
Max round gap was 1 [limit 4] at version 1355245. Max no progress secs was 4.734199 [limit 15] at version 1355245.
Test Ok

@JoshLind JoshLind closed this Jun 1, 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.

None yet

2 participants