From 34cf51332d85e5de74664f1462fb7d533019f347 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 24 Feb 2020 19:38:10 +0100 Subject: [PATCH 1/3] 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. --- ethcore/account-state/src/state.rs | 29 ++++++++++----------- ethcore/executive-state/src/lib.rs | 6 ++--- ethcore/machine/src/executive.rs | 14 +++++++--- ethcore/src/test_helpers/evm_test_client.rs | 13 ++++----- 4 files changed, 35 insertions(+), 27 deletions(-) diff --git a/ethcore/account-state/src/state.rs b/ethcore/account-state/src/state.rs index 13086d7b129..557e313836b 100644 --- a/ethcore/account-state/src/state.rs +++ b/ethcore/account-state/src/state.rs @@ -352,7 +352,7 @@ impl State { 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,21 +759,20 @@ impl State { } /// Remove any touched empty or dust accounts. - pub fn kill_garbage(&mut self, touched: &HashSet
, remove_empty_touched: bool, min_balance: &Option, 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. - || min_balance.map_or(false, |ref balance| entry.account.as_ref().map_or(false, |account| + pub fn kill_garbage(&mut self, touched: &HashSet
, min_balance: &Option, kill_contracts: bool) -> TrieResult<()> { + touched.iter().for_each(|address| { // Check all touched accounts + if let Some(entry) = self.cache.borrow().get(address) { + 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)))) { - - Some(address.clone()) - } else { None }).collect() - }; - for address in to_kill { - self.kill_account(&address); - } + && account.balance() < balance && entry.old_balance.as_ref().map_or(false, |b| account.balance() < b))) { + // todo[dvdplm] This will call self.cache.borrow_mut() – is that ok here? + // Yes(?), because self.cache is `RefCell` so this is a single threaded call + // and nobody else can call `borrow()` here? + self.insert_cache(address, AccountEntry::new_dirty(None)); + } + } + }); Ok(()) } diff --git a/ethcore/executive-state/src/lib.rs b/ethcore/executive-state/src/lib.rs index f41dba4430f..0751aac8ce9 100644 --- a/ethcore/executive-state/src/lib.rs +++ b/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()); diff --git a/ethcore/machine/src/executive.rs b/ethcore/machine/src/executive.rs index 740075346d8..7cc76548b88 100644 --- a/ethcore/machine/src/executive.rs +++ b/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) => { diff --git a/ethcore/src/test_helpers/evm_test_client.rs b/ethcore/src/test_helpers/evm_test_client.rs index 8769af2e80d..1f225828059 100644 --- a/ethcore/src/test_helpers/evm_test_client.rs +++ b/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(); From a876b28bf5d89574b93fd326190265b85df2fded Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 24 Feb 2020 20:33:35 +0100 Subject: [PATCH 2/3] Walk back panicking behaviour --- ethcore/account-state/src/state.rs | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/ethcore/account-state/src/state.rs b/ethcore/account-state/src/state.rs index 557e313836b..2e888e091b3 100644 --- a/ethcore/account-state/src/state.rs +++ b/ethcore/account-state/src/state.rs @@ -760,19 +760,21 @@ impl State { /// Remove any touched empty or dust accounts. pub fn kill_garbage(&mut self, touched: &HashSet
, min_balance: &Option, kill_contracts: bool) -> TrieResult<()> { - touched.iter().for_each(|address| { // Check all touched accounts - if let Some(entry) = self.cache.borrow().get(address) { - 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))) { - // todo[dvdplm] This will call self.cache.borrow_mut() – is that ok here? - // Yes(?), because self.cache is `RefCell` so this is a single threaded call - // and nobody else can call `borrow()` here? - self.insert_cache(address, AccountEntry::new_dirty(None)); - } - } - }); + let to_kill: HashSet<_> = + touched.iter().filter_map(|address| { // Check all touched accounts + if let Some(entry) = self.cache.borrow().get(address) { + 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))) { + Some(address) + } else { None } + } else { None } + }).collect(); + + for address in to_kill { + self.kill_account(address) + } Ok(()) } From 75dc7849e06b3a94cee188c6941d035cf6574b56 Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 25 Feb 2020 15:11:35 +0100 Subject: [PATCH 3/3] Review grumble: prefer `and_then` to `if let` --- ethcore/account-state/src/state.rs | 4 ++-- ethcore/machine/src/executive.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ethcore/account-state/src/state.rs b/ethcore/account-state/src/state.rs index 2e888e091b3..b7e02eef556 100644 --- a/ethcore/account-state/src/state.rs +++ b/ethcore/account-state/src/state.rs @@ -762,14 +762,14 @@ impl State { pub fn kill_garbage(&mut self, touched: &HashSet
, min_balance: &Option, kill_contracts: bool) -> TrieResult<()> { let to_kill: HashSet<_> = touched.iter().filter_map(|address| { // Check all touched accounts - if let Some(entry) = self.cache.borrow().get(address) { + 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))) { Some(address) } else { None } - } else { None } + }) }).collect(); for address in to_kill { diff --git a/ethcore/machine/src/executive.rs b/ethcore/machine/src/executive.rs index 7cc76548b88..13181722244 100644 --- a/ethcore/machine/src/executive.rs +++ b/ethcore/machine/src/executive.rs @@ -1197,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, }) },