Merge pull request #4583 from ethcore/sstore-opt

Optimize key directory reloads

ethstore/src/dir/disk.rs

@@ -90,11 +90,8 @@ impl<T> DiskDirectory<T> where T: KeyFileManager {
 		}
 	}
 
-	/// all accounts found in keys directory
-	fn files(&self) -> Result<HashMap<PathBuf, SafeAccount>, Error> {
-		// it's not done using one iterator cause
-		// there is an issue with rustc and it takes tooo much time to compile
-		let paths = fs::read_dir(&self.path)?
+	fn files(&self) -> Result<Vec<PathBuf>, Error>  {
+		Ok(fs::read_dir(&self.path)?
 			.flat_map(Result::ok)
 			.filter(|entry| {
 				let metadata = entry.metadata().ok();
@@ -108,8 +105,28 @@ impl<T> DiskDirectory<T> where T: KeyFileManager {
 					!IGNORED_FILES.contains(&&*name)
 			})
 			.map(|entry| entry.path())
-			.collect::<Vec<PathBuf>>();
+			.collect::<Vec<PathBuf>>()
+		)
+	}
 
+	pub fn files_hash(&self) -> Result<u64, Error> {
+		use std::collections::hash_map::DefaultHasher;
+		use std::hash::Hasher;
+
+    	let mut hasher = DefaultHasher::new();
+		let files = self.files()?;
+		for file in files {
+			hasher.write(file.to_str().unwrap_or("").as_bytes())
+		}
+
+		Ok(hasher.finish())
+	}
+
+	/// all accounts found in keys directory
+	fn files_content(&self) -> Result<HashMap<PathBuf, SafeAccount>, Error> {
+		// it's not done using one iterator cause
+		// there is an issue with rustc and it takes tooo much time to compile
+		let paths = self.files()?;
 		Ok(paths
 			.into_iter()
 			.filter_map(|path| {
@@ -166,7 +183,7 @@ impl<T> DiskDirectory<T> where T: KeyFileManager {
 
 impl<T> KeyDirectory for DiskDirectory<T> where T: KeyFileManager {
 	fn load(&self) -> Result<Vec<SafeAccount>, Error> {
-		let accounts = self.files()?
+		let accounts = self.files_content()?
 			.into_iter()
 			.map(|(_, account)| account)
 			.collect();
@@ -191,7 +208,7 @@ impl<T> KeyDirectory for DiskDirectory<T> where T: KeyFileManager {
 	fn remove(&self, account: &SafeAccount) -> Result<(), Error> {
 		// enumerate all entries in keystore
 		// and find entry with given address
-		let to_remove = self.files()?
+		let to_remove = self.files_content()?
 			.into_iter()
 			.find(|&(_, ref acc)| acc.id == account.id && acc.address == account.address);
 
@@ -207,6 +224,10 @@ impl<T> KeyDirectory for DiskDirectory<T> where T: KeyFileManager {
 	fn as_vault_provider(&self) -> Option<&VaultKeyDirectoryProvider> {
 		Some(self)
 	}
+
+	fn unique_repr(&self) -> Result<u64, Error> { 
+		self.files_hash() 
+	}
 }
 
 impl<T> VaultKeyDirectoryProvider for DiskDirectory<T> where T: KeyFileManager {
@@ -279,7 +300,6 @@ mod test {
 		let account = SafeAccount::create(&keypair, [0u8; 16], password, 1024, "Test".to_owned(), "{}".to_owned());
 		let res = directory.insert(account);
 
-
 		// then
 		assert!(res.is_ok(), "Should save account succesfuly.");
 		assert!(res.unwrap().filename.is_some(), "Filename has been assigned.");
@@ -336,4 +356,25 @@ mod test {
 		assert!(vaults.iter().any(|v| &*v == "vault1"));
 		assert!(vaults.iter().any(|v| &*v == "vault2"));
 	}
+
+	#[test]
+	fn hash_of_files() {
+		let temp_path = RandomTempPath::new();
+		let directory = RootDiskDirectory::create(&temp_path).unwrap();
+
+		let hash = directory.files_hash().expect("Files hash should be calculated ok"); 
+		assert_eq!( 
+			hash,
+			15130871412783076140
+		);
+
+		let keypair = Random.generate().unwrap();
+		let password = "test pass";
+		let account = SafeAccount::create(&keypair, [0u8; 16], password, 1024, "Test".to_owned(), "{}".to_owned());
+		directory.insert(account).expect("Account should be inserted ok");
+
+		let new_hash = directory.files_hash().expect("New files hash should be calculated ok"); 
+
+		assert!(new_hash != hash, "hash of the file list should change once directory content changed");
+	}
 }

ethstore/src/dir/geth.rs

@@ -95,4 +95,8 @@ impl KeyDirectory for GethDirectory {
 	fn remove(&self, account: &SafeAccount) -> Result<(), Error> {
 		self.dir.remove(account)
 	}
+
+	fn unique_repr(&self) -> Result<u64, Error> {
+		self.dir.unique_repr()
+	}
 }

ethstore/src/dir/memory.rs

@@ -63,5 +63,12 @@ impl KeyDirectory for MemoryDirectory {
 		}
 		Ok(())
 	}
+
+	fn unique_repr(&self) -> Result<u64, Error> {
+		let mut val = 0u64;
+		let accounts = self.accounts.read();
+		for acc in accounts.keys() { val = val ^ ::util::FixedHash::low_u64(acc) }
+		Ok(val)
+	}
 }
 

ethstore/src/dir/mod.rs

@@ -62,6 +62,8 @@ pub trait KeyDirectory: Send + Sync {
 	fn path(&self) -> Option<&PathBuf> { None }
 	/// Return vault provider, if available
 	fn as_vault_provider(&self) -> Option<&VaultKeyDirectoryProvider> { None }
+	/// Unique representation of directory account collection
+	fn unique_repr(&self) -> Result<u64, Error>;
 }
 
 /// Vaults provider

ethstore/src/dir/parity.rs

@@ -74,4 +74,8 @@ impl KeyDirectory for ParityDirectory {
 	fn remove(&self, account: &SafeAccount) -> Result<(), Error> {
 		self.dir.remove(account)
 	}
+
+	fn unique_repr(&self) -> Result<u64, Error> {
+		self.dir.unique_repr()
+	}
 }

ethstore/src/ethstore.rs

@@ -230,6 +230,7 @@ pub struct EthMultiStore {
 	// order lock: cache, then vaults
 	cache: RwLock<BTreeMap<StoreAccountRef, Vec<SafeAccount>>>,
 	vaults: Mutex<HashMap<String, Box<VaultKeyDirectory>>>,
+	dir_hash: Mutex<Option<u64>>,
 }
 
 impl EthMultiStore {
@@ -244,11 +245,23 @@ impl EthMultiStore {
 			vaults: Mutex::new(HashMap::new()),
 			iterations: iterations,
 			cache: Default::default(),
+			dir_hash: Default::default(),
 		};
 		store.reload_accounts()?;
 		Ok(store)
 	}
 
+	fn reload_if_changed(&self) -> Result<(), Error> {
+		let mut last_dir_hash = self.dir_hash.lock();
+		let dir_hash = Some(self.dir.unique_repr()?);
+		if *last_dir_hash == dir_hash {
+			return Ok(())
+		}
+		self.reload_accounts()?;
+		*last_dir_hash = dir_hash;
+		Ok(()) 
+	}
+
 	fn reload_accounts(&self) -> Result<(), Error> {
 		let mut cache = self.cache.write();
 
@@ -284,7 +297,7 @@ impl EthMultiStore {
 			}
 		}
 
-		self.reload_accounts()?;
+		self.reload_if_changed()?;
 		let cache = self.cache.read();
 		let accounts = cache.get(account).ok_or(Error::InvalidAccount)?;
 		if accounts.is_empty() {
@@ -431,7 +444,7 @@ impl SimpleSecretStore for EthMultiStore {
 	}
 
 	fn account_ref(&self, address: &Address) -> Result<StoreAccountRef, Error> {
-		self.reload_accounts()?;
+		self.reload_if_changed()?;
 		self.cache.read().keys()
 			.find(|r| &r.address == address)
 			.cloned()
@@ -439,7 +452,7 @@ impl SimpleSecretStore for EthMultiStore {
 	}
 
 	fn accounts(&self) -> Result<Vec<StoreAccountRef>, Error> {
-		self.reload_accounts()?;
+		self.reload_if_changed()?;
 		Ok(self.cache.read().keys().cloned().collect())
 	}
 

ethstore/tests/util/transient_dir.rs

@@ -74,4 +74,8 @@ impl KeyDirectory for TransientDir {
 	fn remove(&self, account: &SafeAccount) -> Result<(), Error> {
 		self.dir.remove(account)
 	}
+
+	fn unique_repr(&self) -> Result<u64, Error> {
+		self.dir.unique_repr()
+	}
 }