implements copy-on-write for vote-accounts by behzadnouri · Pull Request #19362 · solana-labs/solana

behzadnouri · 2021-08-22T23:14:05Z

Problem

Bank::vote_accounts redundantly clones vote-accounts HashMap even though
an immutable reference will suffice:
https://github.com/solana-labs/solana/blob/95c998a19/runtime/src/bank.rs#L5174-L5186

Summary of Changes

This commit implements copy-on-write semantics for vote-accounts by
wrapping the underlying HashMap in Arc<...>.

codecov · 2021-08-23T12:29:17Z

Codecov Report

Merging #19362 (84db04c) into master (af33012) will increase coverage by 0.0%.
The diff coverage is 86.2%.

❗ Current head 84db04c differs from pull request most recent head 818fd84. Consider uploading reports for the commit 818fd84 to get more accurate results

@@           Coverage Diff            @@
##           master   #19362    +/-   ##
========================================
  Coverage    82.7%    82.7%            
========================================
  Files         459      461     +2     
  Lines      130921   131077   +156     
========================================
+ Hits       108321   108457   +136     
- Misses      22600    22620    +20

Bank::vote_accounts redundantly clones vote-accounts HashMap even though an immutable reference will suffice: https://github.com/solana-labs/solana/blob/95c998a19/runtime/src/bank.rs#L5174-L5186 This commit implements copy-on-write semantics for vote-accounts by wrapping the underlying HashMap in Arc<...>.

carllin · 2021-08-29T07:35:43Z

+    pub(crate) fn insert(&mut self, pubkey: Pubkey, (stake, vote_account): (u64, VoteAccount)) {
        self.add_node_stake(stake, &vote_account);
-        if let Some((stake, vote_account)) =
-            self.vote_accounts.insert(pubkey, (stake, vote_account))
-        {
+        let vote_accounts = Arc::make_mut(&mut self.vote_accounts);
+        if let Some((stake, vote_account)) = vote_accounts.insert(pubkey, (stake, vote_account)) {
            self.sub_node_stake(stake, &vote_account);
        }
    }

-    pub fn remove(&mut self, pubkey: &Pubkey) -> Option<(u64, VoteAccount)> {
-        let value = self.vote_accounts.remove(pubkey);
-        if let Some((stake, ref vote_account)) = value {
+    pub(crate) fn remove(&mut self, pubkey: &Pubkey) -> Option<(u64, VoteAccount)> {
+        let vote_accounts = Arc::make_mut(&mut self.vote_accounts);
+        let entry = vote_accounts.remove(pubkey);
+        if let Some((stake, ref vote_account)) = entry {
            self.sub_node_stake(stake, vote_account);
        }
-        value
+        entry
    }

-    pub fn add_stake(&mut self, pubkey: &Pubkey, delta: u64) {
-        if let Some((stake, vote_account)) = self.vote_accounts.get_mut(pubkey) {
+    pub(crate) fn add_stake(&mut self, pubkey: &Pubkey, delta: u64) {
+        let vote_accounts = Arc::make_mut(&mut self.vote_accounts);
+        if let Some((stake, vote_account)) = vote_accounts.get_mut(pubkey) {
            *stake += delta;
            let vote_account = vote_account.clone();
            self.add_node_stake(delta, &vote_account);
        }
    }

-    pub fn sub_stake(&mut self, pubkey: &Pubkey, delta: u64) {
-        if let Some((stake, vote_account)) = self.vote_accounts.get_mut(pubkey) {
+    pub(crate) fn sub_stake(&mut self, pubkey: &Pubkey, delta: u64) {
+        let vote_accounts = Arc::make_mut(&mut self.vote_accounts);
+        if let Some((stake, vote_account)) = vote_accounts.get_mut(pubkey) {


Nice! I think this works

Quick sanity check, this should be safe even if we have interleaving updates between Replay and BankingStage.

For example, imagine we have:

/--------5 (Banking Stage) 0 \-----------4 (ReplayStage)

So the sequence of events:

We start a leader block for our own slot 5 , inheriting the shared stakes object from the slot 0:

solana/runtime/src/bank.rs

Line 1344 in 84db04c

stakes: RwLock::new(parent.stakes.read().unwrap().clone()),

.

At some later point in time Replay concurrently creates and starts replaying block 4, also inheriting the shared stakes object from the slot 0.

The situation we want to avoid is either of the slot-specific updates to stakes via update_cached_accounts():

solana/runtime/src/bank.rs

Line 5152 in 84db04c

fn update_cached_accounts(

from being visible to the other child.

Now we know this is safe because if we do an update in slot 5 before or after 4 is created, we clone the vote accounts for 5 via make_mut, so these updates are hidden from 4.

Yes, it will be safe because as soon as any mutations are about to be made (and there is still another Arc reference to the same hash-map), a clone is made and so the mutations are not leaked to other Arc references.
The added test_staked_nodes_cow test is testing one such scenario.

Bank::vote_accounts redundantly clones vote-accounts HashMap even though an immutable reference will suffice: https://github.com/solana-labs/solana/blob/95c998a19/runtime/src/bank.rs#L5174-L5186 This commit implements copy-on-write semantics for vote-accounts by wrapping the underlying HashMap in Arc<...>.

This reverts commit cb0c470.

Bank::vote_accounts redundantly clones vote-accounts HashMap even though an immutable reference will suffice: https://github.com/solana-labs/solana/blob/95c998a19/runtime/src/bank.rs#L5174-L5186 This commit implements copy-on-write semantics for vote-accounts by wrapping the underlying HashMap in Arc<...>. (cherry picked from commit 8ad52fa) # Conflicts: # core/src/cluster_info_vote_listener.rs # core/src/consensus.rs # ledger/src/blockstore_processor.rs # runtime/src/bank.rs # runtime/src/stakes.rs # runtime/src/vote_account.rs

Bank::vote_accounts redundantly clones vote-accounts HashMap even though an immutable reference will suffice: https://github.com/solana-labs/solana/blob/95c998a19/runtime/src/bank.rs#L5174-L5186 This commit implements copy-on-write semantics for vote-accounts by wrapping the underlying HashMap in Arc<...>.

…22139) Bank::vote_accounts redundantly clones vote-accounts HashMap even though an immutable reference will suffice: https://github.com/solana-labs/solana/blob/95c998a19/runtime/src/bank.rs#L5174-L5186 This commit implements copy-on-write semantics for vote-accounts by wrapping the underlying HashMap in Arc<...>. Co-authored-by: behzad nouri <behzadnouri@gmail.com>

behzadnouri force-pushed the arc-vote-accounts-lite branch 3 times, most recently from 66af32d to 24cbc2c Compare August 23, 2021 02:16

behzadnouri requested review from carllin and sakridge August 23, 2021 12:31

behzadnouri force-pushed the arc-vote-accounts-lite branch from 24cbc2c to 818fd84 Compare August 26, 2021 17:13

carllin reviewed Aug 29, 2021

View reviewed changes

behzadnouri requested a review from carllin August 29, 2021 14:44

carllin approved these changes Aug 29, 2021

View reviewed changes

behzadnouri merged commit 8ad52fa into solana-labs:master Aug 30, 2021

behzadnouri deleted the arc-vote-accounts-lite branch August 30, 2021 15:54

frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021

Revert "implements copy-on-write for vote-accounts (solana-labs#19362)"

844c2f6

This reverts commit cb0c470.

carllin added the v1.8 label Dec 28, 2021

mergify Bot mentioned this pull request Dec 28, 2021

implements copy-on-write for vote-accounts (backport #19362) #22139

Merged

behzadnouri mentioned this pull request Mar 1, 2022

Validator runs OOM if it cannot make roots #23061

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implements copy-on-write for vote-accounts#19362

implements copy-on-write for vote-accounts#19362
behzadnouri merged 1 commit intosolana-labs:masterfrom
behzadnouri:arc-vote-accounts-lite

behzadnouri commented Aug 22, 2021

Uh oh!

codecov Bot commented Aug 23, 2021 •

edited

Loading

Uh oh!

carllin Aug 29, 2021 •

edited

Loading

Uh oh!

behzadnouri Aug 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

behzadnouri commented Aug 22, 2021

Problem

Summary of Changes

Uh oh!

codecov Bot commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

carllin Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behzadnouri Aug 29, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Aug 23, 2021 •

edited

Loading

carllin Aug 29, 2021 •

edited

Loading