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

WriteOp carries StateValue #15202

Merged
merged 3 commits into from
Nov 8, 2024
Merged

WriteOp carries StateValue #15202

merged 3 commits into from
Nov 8, 2024

Conversation

msmouse
Copy link
Contributor

@msmouse msmouse commented Nov 6, 2024

Description

WrietOp carries StataValue so that in many cases (that I'm gonna introduce) we can use refs to StateValue directly out of the transaction outputs without constructing them.

Removed the cached hashvalue field on StateValue otherwise there's a slight performance regression (now it's a slight improvement, it looks like). Also, now that we have introduced setters to the fields, it's safer not to cache the hash.
We only need the hashes when we update the trees and if necessary I will explicitly cache them.

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

Copy link

trunk-io bot commented Nov 6, 2024

⏱️ 8h 10m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 3h 48m 🟩🟩🟩🟩🟩 (+5 more)
execution-performance / test-target-determinator 39m 🟩🟩🟩🟩🟩 (+4 more)
test-target-determinator 31m 🟩🟩🟩🟩🟩 (+3 more)
check 27m 🟩🟩🟩🟩🟩 (+4 more)
fetch-last-released-docker-image-tag 17m 🟩🟩🟩🟩🟩 (+3 more)
rust-cargo-deny 15m 🟩🟩🟩🟩🟩 (+5 more)
check-dynamic-deps 14m 🟩🟩🟩🟩🟩 (+7 more)
rust-move-tests 9m 🟩
rust-move-tests 9m 🟩
rust-move-tests 9m 🟩
rust-move-tests 9m 🟩
rust-move-tests 9m 🟩
rust-move-tests 9m 🟩
rust-move-tests 8m 🟩
rust-move-tests 5m

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

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

settingsfeedbackdocs ⋅ learn more about trunk.io

@msmouse msmouse force-pushed the 1105-alden-write-op-value branch 2 times, most recently from bdfb2f3 to d77d66d Compare November 6, 2024 00:24
@msmouse msmouse added CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR CICD:run-execution-performance-test Run execution performance test labels Nov 6, 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.

This comment has been minimized.

This comment has been minimized.

@msmouse msmouse changed the title remove Hash impl for WriteSet family WriteOp carries StateValue Nov 6, 2024
@msmouse msmouse requested a review from a team November 6, 2024 23:59
@msmouse msmouse marked this pull request as ready for review November 6, 2024 23:59
Copy link
Contributor

@georgemitenkov georgemitenkov left a comment

Choose a reason for hiding this comment

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

Like this change, only one question about the hash

types/src/write_set.rs Outdated Show resolved Hide resolved
types/src/write_set.rs Outdated Show resolved Hide resolved
types/src/write_set.rs Outdated Show resolved Hide resolved
types/src/state_store/state_value.rs Show resolved Hide resolved
types/src/write_set.rs Show resolved Hide resolved

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 enabled auto-merge (squash) November 7, 2024 22:52

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 disabled auto-merge November 7, 2024 23:44
@msmouse msmouse enabled auto-merge (rebase) November 7, 2024 23:44

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Nov 8, 2024

✅ Forge suite realistic_env_max_load success on a4c0e86e92c512dba93b872b4e3deec8f63a4c6c

two traffics test: inner traffic : committed: 14309.06 txn/s, latency: 2776.65 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3100 ms), latency samples: 5440680
two traffics test : committed: 99.90 txn/s, latency: 1497.72 ms, (p50: 1400 ms, p70: 1400, p90: 1500 ms, p99: 7200 ms), latency samples: 1740
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.113, avg: 1.599", "ConsensusProposalToOrdered: max: 0.325, avg: 0.295", "ConsensusOrderedToCommit: max: 0.371, avg: 0.358", "ConsensusProposalToCommit: max: 0.663, avg: 0.653"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.11s no progress at version 2170268 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.41s no progress at version 2170266 (avg 8.41s) [limit 15].
Test Ok

Copy link
Contributor

github-actions bot commented Nov 8, 2024

✅ Forge suite framework_upgrade success on fcd2dedf6ca61a23f4979e944c629dcdbdae5dca ==> a4c0e86e92c512dba93b872b4e3deec8f63a4c6c

Compatibility test results for fcd2dedf6ca61a23f4979e944c629dcdbdae5dca ==> a4c0e86e92c512dba93b872b4e3deec8f63a4c6c (PR)
Upgrade the nodes to version: a4c0e86e92c512dba93b872b4e3deec8f63a4c6c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1479.84 txn/s, submitted: 1482.38 txn/s, failed submission: 2.54 txn/s, expired: 2.54 txn/s, latency: 2191.01 ms, (p50: 1900 ms, p70: 2100, p90: 3600 ms, p99: 5400 ms), latency samples: 128240
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1328.28 txn/s, submitted: 1331.59 txn/s, failed submission: 3.30 txn/s, expired: 3.30 txn/s, latency: 2254.20 ms, (p50: 2100 ms, p70: 2400, p90: 3000 ms, p99: 5100 ms), latency samples: 120600
5. check swarm health
Compatibility test for fcd2dedf6ca61a23f4979e944c629dcdbdae5dca ==> a4c0e86e92c512dba93b872b4e3deec8f63a4c6c passed
Upgrade the remaining nodes to version: a4c0e86e92c512dba93b872b4e3deec8f63a4c6c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1376.54 txn/s, submitted: 1378.62 txn/s, failed submission: 2.08 txn/s, expired: 2.08 txn/s, latency: 2278.64 ms, (p50: 2100 ms, p70: 2400, p90: 3300 ms, p99: 5000 ms), latency samples: 119100
Test Ok

Copy link
Contributor

github-actions bot commented Nov 8, 2024

✅ Forge suite compat success on fcd2dedf6ca61a23f4979e944c629dcdbdae5dca ==> a4c0e86e92c512dba93b872b4e3deec8f63a4c6c

Compatibility test results for fcd2dedf6ca61a23f4979e944c629dcdbdae5dca ==> a4c0e86e92c512dba93b872b4e3deec8f63a4c6c (PR)
1. Check liveness of validators at old version: fcd2dedf6ca61a23f4979e944c629dcdbdae5dca
compatibility::simple-validator-upgrade::liveness-check : committed: 15082.66 txn/s, latency: 2067.17 ms, (p50: 2000 ms, p70: 2100, p90: 2200 ms, p99: 7500 ms), latency samples: 570480
2. Upgrading first Validator to new version: a4c0e86e92c512dba93b872b4e3deec8f63a4c6c
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6569.04 txn/s, latency: 4193.38 ms, (p50: 4600 ms, p70: 4900, p90: 5500 ms, p99: 5600 ms), latency samples: 116480
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6991.64 txn/s, latency: 4583.89 ms, (p50: 4800 ms, p70: 5000, p90: 6300 ms, p99: 6700 ms), latency samples: 234260
3. Upgrading rest of first batch to new version: a4c0e86e92c512dba93b872b4e3deec8f63a4c6c
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6368.84 txn/s, latency: 4378.58 ms, (p50: 4900 ms, p70: 5200, p90: 5700 ms, p99: 5900 ms), latency samples: 117920
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6382.02 txn/s, latency: 4999.48 ms, (p50: 5400 ms, p70: 5600, p90: 6500 ms, p99: 7100 ms), latency samples: 217280
4. upgrading second batch to new version: a4c0e86e92c512dba93b872b4e3deec8f63a4c6c
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 8445.61 txn/s, latency: 3355.90 ms, (p50: 3800 ms, p70: 4000, p90: 4400 ms, p99: 4500 ms), latency samples: 150600
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 8035.51 txn/s, latency: 3927.50 ms, (p50: 4100 ms, p70: 4500, p90: 5500 ms, p99: 5800 ms), latency samples: 266680
5. check swarm health
Compatibility test for fcd2dedf6ca61a23f4979e944c629dcdbdae5dca ==> a4c0e86e92c512dba93b872b4e3deec8f63a4c6c passed
Test Ok

@msmouse msmouse merged commit 7df77b8 into main Nov 8, 2024
132 of 138 checks passed
@msmouse msmouse deleted the 1105-alden-write-op-value branch November 8, 2024 00:17
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 CICD:run-execution-performance-test Run execution performance test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants