From eecdacac42610df3ba157f7ac67fd6c8ec67378f Mon Sep 17 00:00:00 2001
From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com>
Date: Wed, 28 Oct 2020 11:36:28 +0000
Subject: [PATCH] Don't hold dashmap write lock in store create (#13007)
 (#13230)

Co-authored-by: Carl Lin <carl@solana.com>
(cherry picked from commit c8fc0a6ba1ffe5dbf5f6dc7271108b1e9cd97db1)

Co-authored-by: carllin <wumu727@gmail.com>
---
 runtime/benches/accounts.rs | 58 ++++++++++++++++++++++++++++++++++++-
 runtime/src/accounts_db.rs  | 24 +++++++++------
 2 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/runtime/benches/accounts.rs b/runtime/benches/accounts.rs
index 47f8d8072f..50e73f9dbf 100644
--- a/runtime/benches/accounts.rs
+++ b/runtime/benches/accounts.rs
@@ -2,6 +2,7 @@
 
 extern crate test;
 
+use dashmap::DashMap;
 use rand::Rng;
 use solana_runtime::{
     accounts::{create_test_accounts, Accounts},
@@ -12,7 +13,12 @@ use solana_sdk::{
     genesis_config::{create_genesis_config, ClusterType},
     pubkey::Pubkey,
 };
-use std::{collections::HashMap, path::PathBuf, sync::Arc, thread::Builder};
+use std::{
+    collections::HashMap,
+    path::PathBuf,
+    sync::{Arc, RwLock},
+    thread::Builder,
+};
 use test::Bencher;
 
 fn deposit_many(bank: &Bank, pubkeys: &mut Vec<Pubkey>, num: usize) {
@@ -195,3 +201,53 @@ fn bench_concurrent_read_write(bencher: &mut Bencher) {
         }
     })
 }
+
+#[bench]
+#[ignore]
+fn bench_dashmap_single_reader_with_n_writers(bencher: &mut Bencher) {
+    let num_readers = 5;
+    let num_keys = 10000;
+    let map = Arc::new(DashMap::new());
+    for i in 0..num_keys {
+        map.insert(i, i);
+    }
+    for _ in 0..num_readers {
+        let map = map.clone();
+        Builder::new()
+            .name("readers".to_string())
+            .spawn(move || loop {
+                test::black_box(map.entry(5).or_insert(2));
+            })
+            .unwrap();
+    }
+    bencher.iter(|| {
+        for _ in 0..num_keys {
+            test::black_box(map.get(&5).unwrap().value());
+        }
+    })
+}
+
+#[bench]
+#[ignore]
+fn bench_rwlock_hashmap_single_reader_with_n_writers(bencher: &mut Bencher) {
+    let num_readers = 5;
+    let num_keys = 10000;
+    let map = Arc::new(RwLock::new(HashMap::new()));
+    for i in 0..num_keys {
+        map.write().unwrap().insert(i, i);
+    }
+    for _ in 0..num_readers {
+        let map = map.clone();
+        Builder::new()
+            .name("readers".to_string())
+            .spawn(move || loop {
+                test::black_box(map.write().unwrap().get(&5));
+            })
+            .unwrap();
+    }
+    bencher.iter(|| {
+        for _ in 0..num_keys {
+            test::black_box(map.read().unwrap().get(&5));
+        }
+    })
+}
diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs
index 0ebf47ad7a..db24a92930 100644
--- a/runtime/src/accounts_db.rs
+++ b/runtime/src/accounts_db.rs
@@ -130,9 +130,8 @@ impl AccountStorage {
         slot: Slot,
         store_id: AppendVecId,
     ) -> Option<Arc<AccountStorageEntry>> {
-        self.0
-            .get(&slot)
-            .and_then(|storage_map| storage_map.value().read().unwrap().get(&store_id).cloned())
+        self.get_slot_stores(slot)
+            .and_then(|storage_map| storage_map.read().unwrap().get(&store_id).cloned())
     }
 
     fn get_slot_stores(&self, slot: Slot) -> Option<SlotStores> {
@@ -1402,12 +1401,19 @@ impl AccountsDB {
         let store =
             Arc::new(self.new_storage_entry(slot, &Path::new(&self.paths[path_index]), size));
         let store_for_index = store.clone();
-        let slot_storage = self
-            .storage
-            .0
-            .entry(slot)
-            .or_insert(Arc::new(RwLock::new(HashMap::new())));
-        slot_storage
+
+        let slot_storages: SlotStores = self.storage.get_slot_stores(slot).unwrap_or_else(||
+            // DashMap entry.or_insert() returns a RefMut, essentially a write lock,
+            // which is dropped after this block ends, minimizing time held by the lock.
+            // However, we still want to persist the reference to the `SlotStores` behind
+            // the lock, hence we clone it out, (`SlotStores` is an Arc so is cheap to clone).
+            self.storage
+                .0
+                .entry(slot)
+                .or_insert(Arc::new(RwLock::new(HashMap::new())))
+                .clone());
+
+        slot_storages
             .write()
             .unwrap()
             .insert(store.id, store_for_index);