From 0ff2bfdd0c49bc88e64ec562af87114bbc77665c Mon Sep 17 00:00:00 2001 From: anatoly yakovenko Date: Tue, 16 Apr 2019 08:50:05 -0700 Subject: [PATCH] Fewer unsafe hacks for AppendVec (#3801) * storage account changes * cleanup * checks * comments * clippy * tests * woot! * comments * benches --- runtime/benches/append_vec.rs | 40 ++++---- runtime/src/accounts.rs | 51 ++++++---- runtime/src/append_vec.rs | 187 +++++++++++++++++++--------------- runtime/src/lib.rs | 3 + 4 files changed, 160 insertions(+), 121 deletions(-) diff --git a/runtime/benches/append_vec.rs b/runtime/benches/append_vec.rs index ccd5017e1..e55d8d9f7 100644 --- a/runtime/benches/append_vec.rs +++ b/runtime/benches/append_vec.rs @@ -15,8 +15,8 @@ fn append_vec_append(bencher: &mut Bencher) { let path = get_append_vec_path("bench_append"); let vec = AppendVec::new(&path.path, true, 64 * 1024); bencher.iter(|| { - let account = create_test_account(0); - if vec.append_account(&account).is_none() { + let (meta, account) = create_test_account(0); + if vec.append_account(meta, &account).is_none() { vec.reset(); } }); @@ -26,8 +26,8 @@ fn add_test_accounts(vec: &AppendVec, size: usize) -> Vec<(usize, usize)> { (0..size) .into_iter() .filter_map(|sample| { - let account = create_test_account(sample); - vec.append_account(&account).map(|pos| (sample, pos)) + let (meta, account) = create_test_account(sample); + vec.append_account(meta, &account).map(|pos| (sample, pos)) }) .collect() } @@ -40,9 +40,9 @@ fn append_vec_sequential_read(bencher: &mut Bencher) { let mut indexes = add_test_accounts(&vec, size); bencher.iter(|| { let (sample, pos) = indexes.pop().unwrap(); - let account = vec.get_account(pos); - let test = create_test_account(sample); - assert_eq!(*account, test); + let (account, _next) = vec.get_account(pos).unwrap(); + let (_meta, test) = create_test_account(sample); + assert_eq!(account.data, test.data.as_slice()); indexes.push((sample, pos)); }); } @@ -55,9 +55,9 @@ fn append_vec_random_read(bencher: &mut Bencher) { bencher.iter(|| { let random_index: usize = thread_rng().gen_range(0, indexes.len()); let (sample, pos) = &indexes[random_index]; - let account = vec.get_account(*pos); - let test = create_test_account(*sample); - assert_eq!(*account, test); + let (account, _next) = vec.get_account(*pos).unwrap(); + let (_meta, test) = create_test_account(*sample); + assert_eq!(account.data, test.data.as_slice()); }); } @@ -70,8 +70,8 @@ fn append_vec_concurrent_append_read(bencher: &mut Bencher) { let indexes1 = indexes.clone(); spawn(move || loop { let sample = indexes1.lock().unwrap().len(); - let account = create_test_account(sample); - if let Some(pos) = vec1.append_account(&account) { + let (meta, account) = create_test_account(sample); + if let Some(pos) = vec1.append_account(meta, &account) { indexes1.lock().unwrap().push((sample, pos)) } else { break; @@ -84,9 +84,9 @@ fn append_vec_concurrent_append_read(bencher: &mut Bencher) { let len = indexes.lock().unwrap().len(); let random_index: usize = thread_rng().gen_range(0, len); let (sample, pos) = indexes.lock().unwrap().get(random_index).unwrap().clone(); - let account = vec.get_account(pos); - let test = create_test_account(sample); - assert_eq!(*account, test); + let (account, _next) = vec.get_account(pos).unwrap(); + let (_meta, test) = create_test_account(sample); + assert_eq!(account.data, test.data.as_slice()); }); } @@ -106,14 +106,14 @@ fn append_vec_concurrent_read_append(bencher: &mut Bencher) { .get(random_index % len) .unwrap() .clone(); - let account = vec1.get_account(pos); - let test = create_test_account(sample); - assert_eq!(*account, test); + let (account, _next) = vec1.get_account(pos).unwrap(); + let (_meta, test) = create_test_account(sample); + assert_eq!(account.data, test.data.as_slice()); }); bencher.iter(|| { let sample: usize = thread_rng().gen_range(0, 256); - let account = create_test_account(sample); - if let Some(pos) = vec.append_account(&account) { + let (meta, account) = create_test_account(sample); + if let Some(pos) = vec.append_account(meta, &account) { indexes.lock().unwrap().push((sample, pos)) } }); diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index c62346ccc..d296c7604 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -19,7 +19,7 @@ //! commit for each fork entry would be indexed. use crate::accounts_index::{AccountsIndex, Fork}; -use crate::append_vec::{AppendVec, StoredAccount}; +use crate::append_vec::{AppendVec, StorageMeta, StoredAccount}; use crate::message_processor::has_duplicates; use bincode::serialize; use hashbrown::{HashMap, HashSet}; @@ -29,7 +29,7 @@ use rayon::prelude::*; use solana_metrics::counter::Counter; use solana_sdk::account::Account; use solana_sdk::fee_calculator::FeeCalculator; -use solana_sdk::hash::{hash, Hash, Hasher}; +use solana_sdk::hash::{Hash, Hasher}; use solana_sdk::native_loader; use solana_sdk::pubkey::Pubkey; use solana_sdk::signature::{Keypair, KeypairUtil}; @@ -278,14 +278,21 @@ impl AccountsDB { .collect() } + fn hash_account(stored_account: &StoredAccount) -> Hash { + let mut hasher = Hasher::default(); + hasher.hash(&serialize(&stored_account.balance).unwrap()); + hasher.hash(stored_account.data); + hasher.result() + } + pub fn hash_internal_state(&self, fork_id: Fork) -> Option { let accumulator: Vec> = self.scan_account_storage( fork_id, |stored_account: &StoredAccount, accum: &mut Vec<(Pubkey, u64, Hash)>| { accum.push(( - stored_account.pubkey, - stored_account.write_version, - hash(&serialize(&stored_account.account).unwrap()), + stored_account.meta.pubkey, + stored_account.meta.write_version, + Self::hash_account(stored_account), )); }, ); @@ -313,7 +320,7 @@ impl AccountsDB { //TODO: thread this as a ref storage .get(info.id) - .map(|store| store.accounts.get_account(info.offset).account.clone()) + .and_then(|store| Some(store.accounts.get_account(info.offset)?.0.clone_account())) } fn load_tx_accounts( @@ -530,13 +537,12 @@ impl AccountsDB { { let accounts = &self.storage.read().unwrap()[id]; let write_version = self.write_version.fetch_add(1, Ordering::Relaxed) as u64; - let stored_account = StoredAccount { + let meta = StorageMeta { write_version, pubkey: *pubkey, - //TODO: fix all this copy - account: account.clone(), + data_len: account.data.len() as u64, }; - result = accounts.accounts.append_account(&stored_account); + result = accounts.accounts.append_account(meta, account); accounts.add_account(); } if let Some(val) = result { @@ -691,21 +697,24 @@ impl Accounts { } pub fn load_by_program(&self, fork: Fork, program_id: &Pubkey) -> Vec<(Pubkey, Account)> { - let accumulator: Vec> = self.accounts_db.scan_account_storage( + let accumulator: Vec> = self.accounts_db.scan_account_storage( fork, - |stored_account: &StoredAccount, accum: &mut Vec| { - if stored_account.account.owner == *program_id { - accum.push(stored_account.clone()) + |stored_account: &StoredAccount, accum: &mut Vec<(Pubkey, u64, Account)>| { + if stored_account.balance.owner == *program_id { + let val = ( + stored_account.meta.pubkey, + stored_account.meta.write_version, + stored_account.clone_account(), + ); + accum.push(val) } }, ); - let mut versions: Vec = accumulator.into_iter().flat_map(|x| x).collect(); - versions.sort_by_key(|s| (s.pubkey, (s.write_version as i64).neg())); - versions.dedup_by_key(|s| s.pubkey); - versions - .into_iter() - .map(|s| (s.pubkey, s.account)) - .collect() + let mut versions: Vec<(Pubkey, u64, Account)> = + accumulator.into_iter().flat_map(|x| x).collect(); + versions.sort_by_key(|s| (s.0, (s.1 as i64).neg())); + versions.dedup_by_key(|s| s.0); + versions.into_iter().map(|s| (s.0, s.2)).collect() } /// Slow because lock is held for 1 operation instead of many diff --git a/runtime/src/append_vec.rs b/runtime/src/append_vec.rs index aa5dfa91f..6013439ce 100644 --- a/runtime/src/append_vec.rs +++ b/runtime/src/append_vec.rs @@ -16,16 +16,44 @@ macro_rules! align_up { }; } -//TODO: This structure should contain references -/// StoredAccount contains enough context to recover the index from storage itself +/// StorageMeta contains enough context to recover the index from storage itself #[derive(Clone, PartialEq, Debug)] -pub struct StoredAccount { +pub struct StorageMeta { /// global write version pub write_version: u64, /// key for the account pub pubkey: Pubkey, + pub data_len: u64, +} + +#[derive(Serialize, Deserialize, Clone, Default, Eq, PartialEq)] +pub struct AccountBalance { + /// lamports in the account + pub lamports: u64, + /// the program that owns this account. If executable, the program that loads this account. + pub owner: Pubkey, + /// this account's data contains a loaded program (and is now read-only) + pub executable: bool, +} + +/// References to Memory Mapped memory +/// The Account is stored separately from its data, so getting the actual account requires a clone +pub struct StoredAccount<'a> { + pub meta: &'a StorageMeta, /// account data - pub account: Account, + pub balance: &'a AccountBalance, + pub data: &'a [u8], +} + +impl<'a> StoredAccount<'a> { + pub fn clone_account(&self) -> Account { + Account { + lamports: self.balance.lamports, + owner: self.balance.owner, + executable: self.balance.executable, + data: self.data.to_vec(), + } + } } pub struct AppendVec { @@ -81,17 +109,20 @@ impl AppendVec { self.file_size } - // The reason for the `mut` is to allow the account data pointer to be fixed up after - // the structure is loaded - #[allow(clippy::mut_from_ref)] - fn get_slice(&self, offset: usize, size: usize) -> &mut [u8] { + // The reason for the `mut` is to allow Vec::from_raw_parts + fn get_slice(&self, offset: usize, size: usize) -> Option<(&[u8], usize)> { let len = self.len(); - assert!(len >= offset + size); - let data = &self.map[offset..offset + size]; - unsafe { - let dst = data.as_ptr() as *mut u8; - std::slice::from_raw_parts_mut(dst, size) + if len < offset + size { + return None; } + let data = &self.map[offset..offset + size]; + //Data is aligned at the next 64 byte offset. Without alignment loading the memory may + //crash on some architectures. + let next = align_up!(offset + size, mem::size_of::()); + Some(( + unsafe { std::slice::from_raw_parts(data.as_ptr() as *const u8, size) }, + next, + )) } fn append_ptr(&self, offset: &mut usize, src: *const u8, len: usize) { @@ -99,6 +130,7 @@ impl AppendVec { //crash on some architectures. let pos = align_up!(*offset as usize, mem::size_of::()); let data = &self.map[pos..(pos + len)]; + //this mut append is safe because only 1 thread can append at a time unsafe { let dst = data.as_ptr() as *mut u8; std::ptr::copy(src, dst, len); @@ -132,71 +164,64 @@ impl AppendVec { Some(pos) } - //TODO: Make this safer - //StoredAccount should be a struct of references with the same lifetime as &self - //The structure should have a method to clone the account out - #[allow(clippy::transmute_ptr_to_ptr)] - pub fn get_account(&self, offset: usize) -> &StoredAccount { - let account: *mut StoredAccount = { - let data = self.get_slice(offset, mem::size_of::()); - unsafe { std::mem::transmute::<*const u8, *mut StoredAccount>(data.as_ptr()) } - }; - //Data is aligned at the next 64 byte offset. Without alignment loading the memory may - //crash on some architectures. - let data_at = align_up!( - offset + mem::size_of::(), - mem::size_of::() - ); - let account_ref: &mut StoredAccount = unsafe { &mut *account }; - let data = self.get_slice(data_at, account_ref.account.data.len()); - unsafe { - let mut new_data = Vec::from_raw_parts(data.as_mut_ptr(), data.len(), data.len()); - std::mem::swap(&mut account_ref.account.data, &mut new_data); - std::mem::forget(new_data); - }; - account_ref + fn get_type(&self, offset: usize) -> Option<(&T, usize)> { + let (data, next) = self.get_slice(offset, mem::size_of::())?; + let ptr: *const T = data.as_ptr() as *const T; + Some((unsafe { &*ptr }, next)) } - pub fn accounts(&self, mut start: usize) -> Vec<&StoredAccount> { + pub fn get_account<'a>(&'a self, offset: usize) -> Option<(StoredAccount<'a>, usize)> { + let (meta, next): (&'a StorageMeta, _) = self.get_type(offset)?; + let (balance, next): (&'a AccountBalance, _) = self.get_type(next)?; + let (data, next) = self.get_slice(next, meta.data_len as usize)?; + Some(( + StoredAccount { + meta, + balance, + data, + }, + next, + )) + } + pub fn get_account_test(&self, offset: usize) -> Option<(StorageMeta, Account)> { + let stored = self.get_account(offset)?; + let meta = stored.0.meta.clone(); + Some((meta, stored.0.clone_account())) + } + + pub fn accounts<'a>(&'a self, mut start: usize) -> Vec> { let mut accounts = vec![]; - loop { - //Data is aligned at the next 64 byte offset. Without alignment loading the memory may - //crash on some architectures. - let end = align_up!( - start + mem::size_of::(), - mem::size_of::() - ); - if end > self.len() { - break; - } - let first = self.get_account(start); - accounts.push(first); - //Data is aligned at the next 64 byte offset. Without alignment loading the memory may - //crash on some architectures. - let data_at = align_up!( - start + mem::size_of::(), - mem::size_of::() - ); - let next = align_up!(data_at + first.account.data.len(), mem::size_of::()); + while let Some((account, next)) = self.get_account(start) { + accounts.push(account); start = next; } accounts } - pub fn append_account(&self, account: &StoredAccount) -> Option { - let acc_ptr = account as *const StoredAccount; - let data_len = account.account.data.len(); - let data_ptr = account.account.data.as_ptr(); + pub fn append_account(&self, storage_meta: StorageMeta, account: &Account) -> Option { + let meta_ptr = &storage_meta as *const StorageMeta; + let balance = AccountBalance { + lamports: account.lamports, + owner: account.owner, + executable: account.executable, + }; + let balance_ptr = &balance as *const AccountBalance; + let data_len = account.data.len(); + let data_ptr = account.data.as_ptr(); let ptrs = [ - (acc_ptr as *const u8, mem::size_of::()), + (meta_ptr as *const u8, mem::size_of::()), + (balance_ptr as *const u8, mem::size_of::()), (data_ptr, data_len), ]; self.append_ptrs(&ptrs) } + pub fn append_account_test(&self, data: &(StorageMeta, Account)) -> Option { + self.append_account(data.0.clone(), &data.1) + } } pub mod test_utils { - use super::StoredAccount; + use super::StorageMeta; use rand::distributions::Alphanumeric; use rand::{thread_rng, Rng}; use solana_sdk::account::Account; @@ -226,15 +251,16 @@ pub mod test_utils { TempFile { path: buf } } - pub fn create_test_account(sample: usize) -> StoredAccount { + pub fn create_test_account(sample: usize) -> (StorageMeta, Account) { let data_len = sample % 256; let mut account = Account::new(sample as u64, 0, &Pubkey::default()); account.data = (0..data_len).map(|_| data_len as u8).collect(); - StoredAccount { + let storage_meta = StorageMeta { write_version: 0, pubkey: Pubkey::default(), - account, - } + data_len: data_len as u64, + }; + (storage_meta, account) } } @@ -248,12 +274,12 @@ pub mod tests { use std::time::Instant; #[test] - fn test_append_vec() { + fn test_append_vec_one() { let path = get_append_vec_path("test_append"); let av = AppendVec::new(&path.path, true, 1024 * 1024); let account = create_test_account(0); - let index = av.append_account(&account).unwrap(); - assert_eq!(*av.get_account(index), account); + let index = av.append_account_test(&account).unwrap(); + assert_eq!(av.get_account_test(index).unwrap(), account); } #[test] @@ -261,12 +287,12 @@ pub mod tests { let path = get_append_vec_path("test_append_data"); let av = AppendVec::new(&path.path, true, 1024 * 1024); let account = create_test_account(5); - let index = av.append_account(&account).unwrap(); - assert_eq!(*av.get_account(index), account); + let index = av.append_account_test(&account).unwrap(); + assert_eq!(av.get_account_test(index).unwrap(), account); let account1 = create_test_account(6); - let index1 = av.append_account(&account1).unwrap(); - assert_eq!(*av.get_account(index), account); - assert_eq!(*av.get_account(index1), account1); + let index1 = av.append_account_test(&account1).unwrap(); + assert_eq!(av.get_account_test(index).unwrap(), account); + assert_eq!(av.get_account_test(index1).unwrap(), account1); } #[test] @@ -278,8 +304,8 @@ pub mod tests { let now = Instant::now(); for sample in 0..size { let account = create_test_account(sample); - let pos = av.append_account(&account).unwrap(); - assert_eq!(*av.get_account(pos), account); + let pos = av.append_account_test(&account).unwrap(); + assert_eq!(av.get_account_test(pos).unwrap(), account); indexes.push(pos) } trace!("append time: {} ms", duration_as_ms(&now.elapsed()),); @@ -288,18 +314,19 @@ pub mod tests { for _ in 0..size { let sample = thread_rng().gen_range(0, indexes.len()); let account = create_test_account(sample); - assert_eq!(*av.get_account(indexes[sample]), account); + assert_eq!(av.get_account_test(indexes[sample]).unwrap(), account); } trace!("random read time: {} ms", duration_as_ms(&now.elapsed()),); let now = Instant::now(); assert_eq!(indexes.len(), size); assert_eq!(indexes[0], 0); - let accounts = av.accounts(indexes[0]); + let mut accounts = av.accounts(indexes[0]); assert_eq!(accounts.len(), size); - for (sample, v) in accounts.iter().enumerate() { + for (sample, v) in accounts.iter_mut().enumerate() { let account = create_test_account(sample); - assert_eq!(**v, account) + let recovered = v.clone_account(); + assert_eq!(recovered, account.1) } trace!( "sequential read time: {} ms", diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 60ed866e6..b7b14fcf5 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -14,3 +14,6 @@ mod system_instruction_processor; #[macro_use] extern crate solana_metrics; + +#[macro_use] +extern crate serde_derive;