Move DoStateCheckpoint off critical path #15411

msmouse · 2024-11-27T01:02:06Z

Description

Add struct State to represent the speculative state, utilizing the LayeredMap / MapLayer
Wrap the smt in StateSummary, to represent the speculative state tree. StateStorageUsage is removed from the smt and put in State.
The LayeredMap / MapLayer stack can read diffs between versions by following in mem links, hence we were able to remove the hashmaps to track the updates between versions.
Proof read is not removed from the execution stage so we save some CPU by only reading them for the writes, not the reads.

Todo: move it further to a sepate stage.

How Has This Been Tested?

existing coverage

Key Areas to Review

Type of Change

New feature

Which Components or Systems Does This Change Impact?

[x] Validator

trunk-io · 2024-11-27T01:02:10Z

⏱️ 2h 52m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
rust-cargo-deny	20m	🟩 ⬜ 🟩 🟩 🟩 (+7 more)
check-dynamic-deps	17m	🟩 🟩 🟩 🟩 🟥 (+7 more)
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	12m	🟩
rust-move-tests	9m	⬜
rust-move-tests	7m	⬜
general-lints	5m	🟩 ⬜ 🟩 🟩 🟩 (+7 more)
semgrep/ci	5m	🟩 🟩 🟩 🟩 🟩 (+7 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+7 more)

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

github-actions · 2024-12-19T02:22:11Z

✅ Forge suite `realistic_env_max_load` success on `f8d4befe5aed2b07953a6a791efdc2f086115494`

two traffics test: inner traffic : committed: 14526.70 txn/s, latency: 2732.14 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5523540
two traffics test : committed: 100.08 txn/s, latency: 1367.53 ms, (p50: 1300 ms, p70: 1400, p90: 1500 ms, p99: 1600 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.604, avg: 1.550", "ConsensusProposalToOrdered: max: 0.320, avg: 0.294", "ConsensusOrderedToCommit: max: 0.329, avg: 0.319", "ConsensusProposalToCommit: max: 0.621, avg: 0.613"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.56s no progress at version 43845 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.93s no progress at version 6242935 (avg 0.75s) [limit 16].
Test Ok

github-actions · 2024-12-19T02:22:45Z

✅ Forge suite `compat` success on `6593fb81261f25490ffddc2252a861c994234c2a` ==> `f8d4befe5aed2b07953a6a791efdc2f086115494`

Compatibility test results for 6593fb81261f25490ffddc2252a861c994234c2a ==> f8d4befe5aed2b07953a6a791efdc2f086115494 (PR)
1. Check liveness of validators at old version: 6593fb81261f25490ffddc2252a861c994234c2a
compatibility::simple-validator-upgrade::liveness-check : committed: 16034.06 txn/s, latency: 2108.44 ms, (p50: 2100 ms, p70: 2200, p90: 2500 ms, p99: 4200 ms), latency samples: 524480
2. Upgrading first Validator to new version: f8d4befe5aed2b07953a6a791efdc2f086115494
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 1298.04 txn/s, latency: 18917.18 ms, (p50: 19100 ms, p70: 25500, p90: 26400 ms, p99: 27300 ms), latency samples: 48580
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7070.13 txn/s, latency: 4717.28 ms, (p50: 5100 ms, p70: 5100, p90: 5400 ms, p99: 5500 ms), latency samples: 235580
3. Upgrading rest of first batch to new version: f8d4befe5aed2b07953a6a791efdc2f086115494
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6148.48 txn/s, latency: 4746.88 ms, (p50: 5500 ms, p70: 5700, p90: 5800 ms, p99: 5900 ms), latency samples: 111920
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6177.15 txn/s, latency: 5326.10 ms, (p50: 5800 ms, p70: 5900, p90: 6100 ms, p99: 6400 ms), latency samples: 215540
4. upgrading second batch to new version: f8d4befe5aed2b07953a6a791efdc2f086115494
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 10006.53 txn/s, latency: 2790.81 ms, (p50: 3100 ms, p70: 3300, p90: 3400 ms, p99: 3600 ms), latency samples: 180480
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10642.21 txn/s, latency: 2971.40 ms, (p50: 3100 ms, p70: 3300, p90: 3500 ms, p99: 3900 ms), latency samples: 348420
5. check swarm health
Compatibility test for 6593fb81261f25490ffddc2252a861c994234c2a ==> f8d4befe5aed2b07953a6a791efdc2f086115494 passed
Test Ok

areshand · 2024-12-19T00:24:54Z

storage/aptosdb/src/state_store/buffered_state.rs

+    /// the current state and the last checkpoint. shared with outside world.
+    current_state: Arc<Mutex<LedgerStateWithSummary>>,
+    /// The most recent checkpoint sent for persistence, not guaranteed to have committed already.
+    last_snapshot: StateWithSummary,


here, it is called snapshot, while it is called checkpoint in current_state. maybe rename this to last_persisting_checkpoint to be consistent?

areshand · 2024-12-19T04:00:08Z

storage/aptosdb/src/state_store/buffered_state.rs

        target_items: usize,
-        current_state: Arc<Mutex<CurrentState>>,
-        persisted_state: Arc<Mutex<PersistedState>>,
+        out_current_state: Arc<Mutex<LedgerStateWithSummary>>,


nit: what does "out_" mean?

areshand · 2024-12-19T04:09:02Z

storage/aptosdb/src/state_store/buffered_state.rs

+    state_commit_sender: SyncSender<CommitMessage<StateWithSummary>>,
+    /// Estimated number of items in the buffer.
+    estimated_items: usize,
+    /// The target number of items in the buffer between commits.
    target_items: usize,


could target_item be a const instead of a field of bufferedstate?

areshand · 2024-12-19T04:21:25Z

storage/storage-interface/src/lib.rs

@@ -354,7 +356,7 @@ pub trait DbReader: Send + Sync {
        /// Returns the proof of the given state key and version.
        fn get_state_proof_by_version_ext(
            &self,
-            state_key: &StateKey,
+            key: &HashValue,


name it as state_key to be explicit ?

areshand · 2024-12-19T04:52:38Z

storage/storage-interface/src/state_store/state_update_refs.rs

+
+pub struct StateUpdateRefs<'kv> {
+    pub per_version: PerVersionStateUpdateRefs<'kv>,
+    pub for_last_checkpoint: Option<BatchedStateUpdateRefs<'kv>>,


nit: "updates_till_last_checkpoint" could be more intuitive?

areshand · 2024-12-19T04:53:47Z

storage/storage-interface/src/state_store/state_update_refs.rs

+pub struct StateUpdateRefs<'kv> {
+    pub per_version: PerVersionStateUpdateRefs<'kv>,
+    pub for_last_checkpoint: Option<BatchedStateUpdateRefs<'kv>>,
+    pub for_latest: Option<BatchedStateUpdateRefs<'kv>>,


nit: updates_after_last_checkpoint?

areshand

parse through half of the changes. mainly naming suggestions. feel free to drop them

msmouse requested review from lightmark and grao1991 as code owners November 27, 2024 01:02

msmouse force-pushed the 1125-alden-state-summary branch from 0eb8d0e to ddaade9 Compare November 28, 2024 21:58

msmouse marked this pull request as draft November 28, 2024 22:11

aptos-labs deleted a comment from graphite-app bot Nov 28, 2024

msmouse force-pushed the 1125-alden-state-summary branch 8 times, most recently from 6c4828a to 6bc9a36 Compare December 3, 2024 03:31

msmouse force-pushed the 1125-alden-state-summary branch 3 times, most recently from ae365d6 to 9e9964b Compare December 8, 2024 21:51

msmouse added the CICD:run-execution-performance-test Run execution performance test label Dec 10, 2024

msmouse force-pushed the 1125-alden-state-summary branch 4 times, most recently from 550a066 to d59dead Compare December 10, 2024 10:40

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 Dec 10, 2024

This comment has been minimized.

Sign in to view

sample proof check

9a16478

msmouse changed the title ~~StateSummary~~ Move DoStateCheckpoint off critical path Dec 18, 2024

msmouse marked this pull request as ready for review December 18, 2024 05:10

msmouse requested review from alinush, zjma, mstraka100, rex1fernando, zekun000, sasha8 and ibalajiarun as code owners December 18, 2024 05:10

This comment has been minimized.

Sign in to view

msmouse mentioned this pull request Dec 18, 2024

InMemState, StateAuthenticator #15236

Closed

22 tasks

cheap estimate

95fe6da

This comment has been minimized.

Sign in to view

high concurrency proof read

f8d4bef

This comment has been minimized.

Sign in to view

areshand reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move DoStateCheckpoint off critical path #15411

Move DoStateCheckpoint off critical path #15411

msmouse commented Nov 27, 2024 •

edited

Loading

trunk-io bot commented Nov 27, 2024 •

edited

Loading

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.

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.

github-actions bot commented Dec 19, 2024

github-actions bot commented Dec 19, 2024

areshand Dec 19, 2024

areshand Dec 19, 2024

areshand Dec 19, 2024

areshand Dec 19, 2024

areshand Dec 19, 2024

areshand Dec 19, 2024

areshand left a comment •

edited

Loading

Move DoStateCheckpoint off critical path #15411

Are you sure you want to change the base?

Move DoStateCheckpoint off critical path #15411

Conversation

msmouse commented Nov 27, 2024 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

trunk-io bot commented Nov 27, 2024 • edited Loading

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.

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.

github-actions bot commented Dec 19, 2024

✅ Forge suite realistic_env_max_load success on f8d4befe5aed2b07953a6a791efdc2f086115494

github-actions bot commented Dec 19, 2024

✅ Forge suite compat success on 6593fb81261f25490ffddc2252a861c994234c2a ==> f8d4befe5aed2b07953a6a791efdc2f086115494

areshand Dec 19, 2024

Choose a reason for hiding this comment

areshand Dec 19, 2024

Choose a reason for hiding this comment

areshand Dec 19, 2024

Choose a reason for hiding this comment

areshand Dec 19, 2024

Choose a reason for hiding this comment

areshand Dec 19, 2024

Choose a reason for hiding this comment

areshand Dec 19, 2024

Choose a reason for hiding this comment

areshand left a comment • edited Loading

Choose a reason for hiding this comment

msmouse commented Nov 27, 2024 •

edited

Loading

trunk-io bot commented Nov 27, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `f8d4befe5aed2b07953a6a791efdc2f086115494`

✅ Forge suite `compat` success on `6593fb81261f25490ffddc2252a861c994234c2a` ==> `f8d4befe5aed2b07953a6a791efdc2f086115494`

areshand left a comment •

edited

Loading