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

[experimental] LayeredMap which views a sequence of MapLayers #12722

Merged
merged 1 commit into from Apr 15, 2024

Conversation

msmouse
Copy link
Contributor

@msmouse msmouse commented Mar 28, 2024

Description

A multi-versional mapping data structure much like the in memory sparse merkle tree but doesn't do the hashing on nodes.
This is useful for implementing the per-block speculative LRU cache / Active State coming up.

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) - not used yet

How Has This Been Tested?

proptest

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 Mar 28, 2024

⏱️ 4h 15m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-unit-tests 1h 43m 🟩🟥🟩🟩 (+1 more)
rust-lints 32m 🟥🟥🟩🟩🟩 (+1 more)
windows-build 31m 🟥🟩
run-tests-main-branch 27m 🟩🟩🟩🟩🟩 (+1 more)
rust-move-tests 23m 🟩🟩🟩🟩🟩 (+1 more)
general-lints 13m 🟩🟩🟩🟩🟩 (+1 more)
check-dynamic-deps 10m 🟩🟩🟩🟩🟩 (+1 more)
check 9m 🟩🟩
semgrep/ci 3m 🟩🟩🟩🟩🟩 (+1 more)
file_change_determinator 1m 🟩🟩🟩🟩🟩 (+1 more)
file_change_determinator 1m 🟩🟩🟩🟩🟩 (+1 more)
file_change_determinator 1m 🟩🟩🟩🟩🟩
permission-check 22s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 20s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 19s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 17s 🟩🟩🟩🟩🟩 (+1 more)

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

Job Duration vs 7d avg Delta
rust-move-tests 20m 2m +1135%
rust-lints 8m 7m +22%
rust-unit-tests 32m 26m +20%

settingsfeedbackdocs ⋅ learn more about trunk.io

@msmouse
Copy link
Contributor Author

msmouse commented Mar 28, 2024

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

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

@msmouse msmouse force-pushed the 0327-alden-layered-map branch 5 times, most recently from e43b2d5 to b941c9e Compare April 3, 2024 23:21
@msmouse msmouse requested review from grao1991, alinush, a team, areshand and zjma April 3, 2024 23:24
@msmouse msmouse marked this pull request as ready for review April 3, 2024 23:25
@msmouse msmouse requested a review from zekun000 April 3, 2024 23:26
use std::sync::{Arc, Weak};

#[derive(Debug)]
pub enum Ref<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe, when it's out of '/experimental'

#[derive(Debug)]
struct LayerInner<K: Key, V: Value> {
root: NodeRef<K, V>,
children: Mutex<Vec<Arc<LayerInner<K, V>>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

what is children used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same with SMT, there can be branches in the block tree.


#[derive(Clone, Debug)]
pub struct LayeredMap<K: Key, V: Value> {
bottom_layer: MapLayer<K, V>,
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need the bottom_layer? could we assume the LRU cache contains all the base key, value pairs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think "FrozenSparseMerkleTree" -- you need to make sure nodes created between bottom layer and top layer don't go away while you read, by holding a strong ref to the bottom layer root.

@msmouse msmouse changed the title LayeredMap which views a sequence of MapLayers [experimental] LayeredMap which views a sequence of MapLayers Apr 4, 2024
@msmouse msmouse enabled auto-merge (squash) April 15, 2024 22:38

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 aptos-node-v1.10.1 ==> 4a3f15941f1a0fc93f5e279bdaf69098b9ea7922

Compatibility test results for aptos-node-v1.10.1 ==> 4a3f15941f1a0fc93f5e279bdaf69098b9ea7922 (PR)
1. Check liveness of validators at old version: aptos-node-v1.10.1
compatibility::simple-validator-upgrade::liveness-check : committed: 6269 txn/s, latency: 5095 ms, (p50: 4800 ms, p90: 9000 ms, p99: 11100 ms), latency samples: 231980
2. Upgrading first Validator to new version: 4a3f15941f1a0fc93f5e279bdaf69098b9ea7922
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1700 txn/s, latency: 16272 ms, (p50: 19300 ms, p90: 23300 ms, p99: 25200 ms), latency samples: 91840
3. Upgrading rest of first batch to new version: 4a3f15941f1a0fc93f5e279bdaf69098b9ea7922
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1714 txn/s, latency: 16314 ms, (p50: 19300 ms, p90: 22900 ms, p99: 23800 ms), latency samples: 90860
4. upgrading second batch to new version: 4a3f15941f1a0fc93f5e279bdaf69098b9ea7922
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3380 txn/s, latency: 8949 ms, (p50: 9800 ms, p90: 12300 ms, p99: 13100 ms), latency samples: 145380
5. check swarm health
Compatibility test for aptos-node-v1.10.1 ==> 4a3f15941f1a0fc93f5e279bdaf69098b9ea7922 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 4a3f15941f1a0fc93f5e279bdaf69098b9ea7922

two traffics test: inner traffic : committed: 8232 txn/s, latency: 4769 ms, (p50: 4500 ms, p90: 5700 ms, p99: 10000 ms), latency samples: 3548060
two traffics test : committed: 100 txn/s, latency: 1820 ms, (p50: 1800 ms, p90: 2100 ms, p99: 4200 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.225, avg: 0.204", "QsPosToProposal: max: 0.249, avg: 0.235", "ConsensusProposalToOrdered: max: 0.445, avg: 0.410", "ConsensusOrderedToCommit: max: 0.336, avg: 0.323", "ConsensusProposalToCommit: max: 0.749, avg: 0.732"]
Max round gap was 1 [limit 4] at version 1766761. Max no progress secs was 4.8532352 [limit 15] at version 1766761.
Test Ok

@msmouse msmouse merged commit 3b659ae into main Apr 15, 2024
50 of 51 checks passed
@msmouse msmouse deleted the 0327-alden-layered-map branch April 15, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants