Faster kill_garbage (#11514)

* Faster kill_garbage

Benchmark shows that almost half the time `apply()`-ing a transaction is spent in garbage collection. This PR avoids visiting each cache item and `collect()`-ing accounts to clean up.

* Walk back panicking behaviour

* Review grumble: prefer `and_then` to `if let`

ethcore/account-state/src/state.rs

@@ -352,7 +352,7 @@ impl<B: Backend> State<B> {
 
 	fn insert_cache(&self, address: &Address, account: AccountEntry) {
 		// Dirty account which is not in the cache means this is a new account.
-		// It goes directly into the checkpoint as there's nothing to rever to.
+		// It goes directly into the checkpoint as there's nothing to revert to.
 		//
 		// In all other cases account is read as clean first, and after that made
 		// dirty in and added to the checkpoint with `note_cache`.
@@ -759,20 +759,21 @@ impl<B: Backend> State<B> {
 	}
 
 	/// Remove any touched empty or dust accounts.
-	pub fn kill_garbage(&mut self, touched: &HashSet<Address>, remove_empty_touched: bool, min_balance: &Option<U256>, kill_contracts: bool) -> TrieResult<()> {
-		let to_kill: HashSet<_> = {
-			self.cache.borrow().iter().filter_map(|(address, ref entry)|
-				if touched.contains(address) && // Check all touched accounts
-					((remove_empty_touched && entry.exists_and_is_null()) // Remove all empty touched accounts.
+	pub fn kill_garbage(&mut self, touched: &HashSet<Address>, min_balance: &Option<U256>, kill_contracts: bool) -> TrieResult<()> {
+		let to_kill: HashSet<_> =
+			touched.iter().filter_map(|address| { // Check all touched accounts
+				self.cache.borrow().get(address).and_then(|entry| {
+					if entry.exists_and_is_null() // Remove all empty touched accounts.
 						|| min_balance.map_or(false, |ref balance| entry.account.as_ref().map_or(false, |account|
-						(account.is_basic() || kill_contracts) // Remove all basic and optionally contract accounts where balance has been decreased.
-							&& account.balance() < balance && entry.old_balance.as_ref().map_or(false, |b| account.balance() < b)))) {
+							(account.is_basic() || kill_contracts) // Remove all basic and optionally contract accounts where balance has been decreased.
+							&& account.balance() < balance && entry.old_balance.as_ref().map_or(false, |b| account.balance() < b))) {
+						Some(address)
+					} else { None }
+				})
+			}).collect();
 
-					Some(address.clone())
-				} else { None }).collect()
-		};
 		for address in to_kill {
-			self.kill_account(&address);
+			self.kill_account(address)
 		}
 		Ok(())
 	}

ethcore/executive-state/src/lib.rs

@@ -1573,15 +1573,15 @@ mod tests {
 		state.transfer_balance(&b, &x, &1.into(), CleanupMode::TrackTouched(&mut touched)).unwrap(); // touch an account decreasing its balance
 		state.transfer_balance(&c, &x, &1.into(), CleanupMode::TrackTouched(&mut touched)).unwrap(); // touch an account decreasing its balance
 		state.transfer_balance(&e, &x, &1.into(), CleanupMode::TrackTouched(&mut touched)).unwrap(); // touch an account decreasing its balance
-		state.kill_garbage(&touched, true, &None, false).unwrap();
+		state.kill_garbage(&touched, &None, false).unwrap();
 		assert!(!state.exists(&a).unwrap());
 		assert!(state.exists(&b).unwrap());
-		state.kill_garbage(&touched, true, &Some(100.into()), false).unwrap();
+		state.kill_garbage(&touched,&Some(100.into()), false).unwrap();
 		assert!(!state.exists(&b).unwrap());
 		assert!(state.exists(&c).unwrap());
 		assert!(state.exists(&d).unwrap());
 		assert!(state.exists(&e).unwrap());
-		state.kill_garbage(&touched, true, &Some(100.into()), true).unwrap();
+		state.kill_garbage(&touched, &Some(100.into()), true).unwrap();
 		assert!(state.exists(&c).unwrap());
 		assert!(state.exists(&d).unwrap());
 		assert!(!state.exists(&e).unwrap());

ethcore/machine/src/executive.rs

@@ -1175,9 +1175,17 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
 		}
 
 		// perform garbage-collection
-		let min_balance = if schedule.kill_dust != CleanDustMode::Off { Some(U256::from(schedule.tx_gas).overflowing_mul(t.gas_price).0) } else { None };
-		self.state.kill_garbage(&substate.touched, schedule.kill_empty, &min_balance, schedule.kill_dust == CleanDustMode::WithCodeAndStorage)?;
-
+		if schedule.kill_empty {
+			let (min_balance, kill_contracts) = if schedule.kill_dust != CleanDustMode::Off {
+				(
+					Some(U256::from(schedule.tx_gas).overflowing_mul(t.gas_price).0),
+					schedule.kill_dust == CleanDustMode::WithCodeAndStorage,
+				)
+			} else {
+				(None, false)
+			};
+			self.state.kill_garbage(&substate.touched, &min_balance, kill_contracts)?;
+		}
 		match result {
 			Err(vm::Error::Internal(msg)) => Err(ExecutionError::Internal(msg)),
 			Err(exception) => {
@@ -1189,9 +1197,9 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
 					cumulative_gas_used: self.info.gas_used + t.gas,
 					logs: vec![],
 					contracts_created: vec![],
-					output: output,
-					trace: trace,
-					vm_trace: vm_trace,
+					output,
+					trace,
+					vm_trace,
 					state_diff: None,
 				})
 			},

ethcore/src/test_helpers/evm_test_client.rs

@@ -286,12 +286,13 @@ impl<'a> EvmTestClient<'a> {
 		}).ok();
 		// Touching also means that we should remove the account if it's within eip161
 		// conditions.
-		self.state.kill_garbage(
-			&vec![env_info.author].into_iter().collect(),
-			schedule.kill_empty,
-			&None,
-			false
-		).ok();
+		if schedule.kill_empty {
+			self.state.kill_garbage(
+				&vec![env_info.author].into_iter().collect(),
+				&None,
+				false
+			).ok();
+		}
 
 		self.state.commit().ok();