Canonical state cache #2308

arkpar · 2016-09-24T17:42:31Z

Still left to do:

Handle reorgs more efficiently. Right now the whole cache is cleared on reorg
Don't copy data from global cache to state and back (a major slowdown)
Bind cache size to memory used, not to the number of accounts

rphmeier · 2016-09-24T17:44:18Z

(note that this is against beta -- review carefully)

arkpar · 2016-09-24T18:05:06Z

util/network/src/connection.rs

 		}
+		io.update_registration(self.token).ok();


This is a fix from the master branch. Prevents peer disconnects in some cases.

coveralls · 2016-09-24T18:25:28Z

Coverage decreased (-0.03%) to 87.367% when pulling 009f491 on state-cache into decca7f on beta.

gavofyork · 2016-09-24T18:49:07Z

ethcore/src/block.rs

@@ -465,7 +466,10 @@ impl LockedBlock {

 impl Drain for LockedBlock {
 	/// Drop this object and return the underlieing database.
-	fn drain(self) -> Box<JournalDB> { self.block.state.drop().1 }
+	fn drain(mut self) -> StateDB {
+		self.block.state.commit_cache();


if commit_cache is now necessary to be called as part of LockedBlock's general maintenance, what guarantees are there that the rest of the codebase is using it correctly?

this is now removed

gavofyork

this is a pretty substantial change to the internals; i'd like to see the new model of usage documented properly.

coveralls · 2016-09-24T18:51:20Z

Coverage decreased (-0.03%) to 87.367% when pulling 1685032 on state-cache into decca7f on beta.

gavofyork · 2016-09-24T19:01:29Z

so this introduces a secondary cache for account data in StateDB which sticks around as long as there is no reorg and thus avoids flushing the temporary State cache and heading to the database for lookups between blocks?

gavofyork

looks pretty good in general.

a bit of docs on usage, particularly for State::commit_cache and some argument as to why it will get called when needed (e.g. in drain) would be nice to have.

gavofyork · 2016-09-24T18:56:55Z

ethcore/src/state.rs

+									e.remove();
+								}
+							},
+							_ => ()


we tend to use {}, no?

changed to {}

gavofyork · 2016-09-24T18:58:02Z

ethcore/src/state.rs

@@ -285,10 +305,17 @@ impl State {
 		Ok(())
 	}

+	pub fn commit_cache(&mut self) {


why the separate function?

Made if private. Called automatically on drop.

Whitespace and optional symbols

arkpar · 2016-09-25T12:34:38Z

Added in 25d66f4:
commit_cache is called on drop
Clean account items are not copied from StateDB to State and back

gavofyork · 2016-09-25T12:45:14Z

ethcore/src/state.rs

-			&mut Some(ref mut acc) => not_default(acc),
-			slot @ &mut None => *slot = Some(default()),
+			&mut AccountEntry::Cached(ref mut acc) => not_default(acc),
+			slot @ _ => *slot = AccountEntry::Cached(default()),


why not just slot =>...?

coveralls · 2016-09-25T13:15:02Z

Coverage decreased (-0.02%) to 87.422% when pulling 25d66f4 on state-cache into c374164 on beta.

coveralls · 2016-09-25T14:08:04Z

Coverage decreased (-0.02%) to 87.422% when pulling d04b2b6 on state-cache into c374164 on beta.

gavofyork

a few questions and some additional docs would be good.

gavofyork · 2016-09-26T13:05:27Z

ethcore/src/account.rs

-		match self.storage_overlay.borrow_mut().entry(key) {
-			Entry::Occupied(ref mut entry) if entry.get().1 != value => {
-				entry.insert((Filth::Dirty, value));
+		match self.storage_changes.entry(key) {


Does this work properly if the storage was previously cached? What about if the change is later back to the original value?

storage_changes will hold the same value as storage_cache but there's nothing wrong with that. Everything will be merged properly on commit_storage

what if the two hold different values? is there a proof that this can never happen?

It may happen and is supported. storage_cache reflects the contents of the disk database, storage_changes keeps changes over this contents. I'd prefer caching and tracking changes to be split into different structs, so that only the state can have changes. This is left for a future PR.

gavofyork · 2016-09-26T13:06:23Z

ethcore/src/account.rs

-	// Overlay on trie-backed storage - tuple is (<clean>, <value>).
-	storage_overlay: RefCell<HashMap<H256, (Filth, H256)>>,
+	// Cache of trie-backed storage
+	storage_cache: RefCell<LruCache<H256, H256>>,


Should be made explicit in the struct docs about the meaning (and precedence) of these members. Particularly what happens when the same key appears in both.

gavofyork · 2016-09-26T13:09:45Z

ethcore/src/account.rs

+			our_cache.insert(k.clone() , v.clone()); //TODO: cloning should not be required here
+		}
+		*self = other;
+		self.storage_cache = RefCell::new(our_cache);


seems a little over-complex in terms of assignments; why do our_cache and updated_cache need pulling out only to be replaced here? why reassign self?

gavofyork · 2016-09-26T13:11:46Z

ethcore/src/state.rs

+		}
+	}
+
+	fn clone_basic(&self) -> AccountEntry {


maybe add some docs for this function - difficult to immediately see what it's trying to do

gavofyork · 2016-09-26T13:12:18Z

ethcore/src/state.rs

@@ -56,6 +56,22 @@ impl AccountEntry {
 			AccountEntry::Missing => false,
 		}
 	}
+
+	fn clone_dirty(&self) -> Option<AccountEntry> {


maybe add some docs for this function - difficult to immediately see what it's trying to do

gavofyork · 2016-09-26T13:24:06Z

ethcore/src/state.rs

+		// check local cache first
+		if let Some(value) = self.cache.borrow().get(address).and_then(|a|
+			match *a {
+				AccountEntry::Cached(ref account) => account.modified_storage_at(key),


why only modified_storage_at (don't we want to see the cached storage value?)

Storage key search and update now works like this:

If there's an entry for the account in the local cache check for the key and return it if found.

If there's an entry for the account in the global cache check for the key or load it into that account.

If account is missing in the global cache load it into the local cache and cache the key there.

refactored to reflect algorithm mentioned above

arkpar · 2016-09-27T05:56:48Z

Added docs
Removed commit_cache and clear_cache from StateDB interface.

coveralls · 2016-09-27T06:36:14Z

Coverage decreased (-0.02%) to 87.42% when pulling 49ab189 on state-cache into c374164 on beta.

coveralls · 2016-09-27T09:09:02Z

Coverage decreased (-0.02%) to 87.423% when pulling ca997e5 on state-cache into c374164 on beta.

gavofyork

Generally looks good. Quite a few questions though.

gavofyork · 2016-09-27T11:42:46Z

ethcore/src/account.rs

+
+	/// Get cached storage value if any. Returns `None` if the
+	/// key is not in the cache.
+	pub fn cached_storage_at(&self, key: &H256) -> Option<H256> {


Just call this from storage_at instead of the copy'n'paste?

gavofyork · 2016-09-27T11:45:42Z

ethcore/src/account.rs

 			}
+			self.storage_cache.borrow_mut().insert(k, v);


i guess there's no issue here with DB commits being pending while storage_cache is already updated? (db gets written out when t is dropped, right?)

This is just the local cache. Changes to the shared canonical state cache are applied along with the database write.

gavofyork · 2016-09-27T11:46:35Z

ethcore/src/account.rs

+			balance: self.balance.clone(),
+			nonce: self.nonce.clone(),
+			storage_root: self.storage_root.clone(),
+			storage_cache: Self::empty_storage_cache(),


why not copy the cache? waste of memory?

Exactly. And waste of time copying it around all the time. Storage queries will be cached in the shared Account anyway

gavofyork · 2016-09-27T11:48:31Z

ethcore/src/account.rs

+	pub fn clone_dirty(&self) -> Account {
+		let mut account = self.clone_basic();
+		account.storage_changes = self.storage_changes.clone();
+		account.code_cache = self.code_cache.clone();


is code_cache a cache of the database's current code entries? or does it contain dirty changes? if changes, then should probably be renamed code_changes, if a simple cache like storage_cache then surely if should be assigned only in clone_all?

There's no distinction currently. I'll add it.

gavofyork · 2016-09-27T11:50:31Z

ethcore/src/account.rs

+		let mut cache = self.storage_cache.borrow_mut();
+		for (k, v) in other.storage_cache.into_inner().into_iter() {
+			cache.insert(k.clone() , v.clone()); //TODO: cloning should not be required here
+		}


shouldn't we be doing something with other's storage_changes? asserting it's empty, perhaps? if it isn't always empty, it's probably safest to require the call-site to explicitly drop that data first, since dropping potentially important changes implicitly doesn't seem particularly anti-fragile.

there's already an assert for this at the beginning of the function

gavofyork · 2016-09-27T11:59:09Z

ethcore/src/state.rs

+				},
+				AccountEntry::Missing => {
+					self.db.cache_account(address, None);
+				}


missing , (or superfluous one above and below - i good with either rule)

gavofyork · 2016-09-27T12:01:46Z

ethcore/src/state.rs

-				account.cache_code(&AccountDB::from_hash(self.db.as_hashdb(), addr_hash));
+	fn update_account_cache(require: RequireCache, account: &mut Account, address: &Address, db: &HashDB) {
+		match require {
+			RequireCache::None => (),


gavofyork · 2016-09-27T12:07:40Z

ethcore/src/state_db.rs

+/// State database abstraction.
+/// Manages shared global state cache.
+/// A clone of `StateDB` may be created as canonical or not.
+/// For canonical clones cache changes are accumulated and applied


is this additional caching write-through? does it leave the state and block dbs in inconsistent states when there's an unexpected exit during a run of canon blocks?

It is write-through, writes are not affected by this PR

gavofyork · 2016-09-27T12:12:25Z

ethcore/src/state_db.rs

+		let mut cache = self.account_cache.lock();
+		for (address, account) in self.cache_overlay.drain(..) {
+			if let Some(&mut Some(ref mut existing)) = cache.accounts.get_mut(&address) {
+				if let Some(new) = account {


what would account == None mean?

account is known to be missing in the DB

gavofyork · 2016-09-27T12:12:55Z

ethcore/src/state_db.rs

+	}
+
+	/// Apply pending cache changes.
+	fn commit_cache(&mut self) {


this doesn't write through to the DB - is that not an issue on unexpected shutdown?

this only updated in-memory cache, writes are unaffected

gavofyork · 2016-09-27T12:54:43Z

looks good - be good to get those minor doc points and DRY in prior to merge.

arkpar · 2016-09-27T13:26:49Z

Pushed documenation and syle changes

coveralls · 2016-09-27T14:40:47Z

Coverage decreased (-0.03%) to 87.408% when pulling 7e34afa on state-cache into c374164 on beta.

arkpar force-pushed the state-cache branch from 4fdd82d to 009f491 Compare September 24, 2016 18:04

arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Sep 24, 2016

arkpar commented Sep 24, 2016

View reviewed changes

gavofyork reviewed Sep 24, 2016

View reviewed changes

gavofyork approved these changes Sep 24, 2016

View reviewed changes

arkpar added 2 commits September 25, 2016 14:30

State cache

9d34f8a

Reduced copying data between caches

25d66f4

Whitespace and optional symbols

arkpar force-pushed the state-cache branch from 1685032 to 25d66f4 Compare September 25, 2016 12:32

gavofyork reviewed Sep 25, 2016

View reviewed changes

arkpar added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 25, 2016

gavofyork added M4-core ⛓ Core client code / Rust. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 26, 2016

arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Sep 26, 2016

gavofyork suggested changes Sep 26, 2016

View reviewed changes

gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 26, 2016

gavofyork modified the milestone: 1.3.2 Sep 26, 2016

NikVolf mentioned this pull request Sep 26, 2016

Accounts bloom filter #2342

Closed

arkpar force-pushed the state-cache branch from 11ed196 to 49ab189 Compare September 27, 2016 05:55

arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 27, 2016

Set a limit on storage cache

ca997e5

arkpar force-pushed the state-cache branch from 49ab189 to ca997e5 Compare September 27, 2016 08:29

NikVolf mentioned this pull request Sep 27, 2016

Accounts bloom #2357

Merged

gavofyork reviewed Sep 27, 2016

View reviewed changes

gavofyork approved these changes Sep 27, 2016

View reviewed changes

gavofyork added the A8-looksgood 🦄 Pull request is reviewed well. label Sep 27, 2016

gavofyork removed the A0-pleasereview 🤓 Pull request needs code review. label Sep 27, 2016

Style and docs

7e34afa

gavofyork merged commit c9cfcd2 into beta Sep 27, 2016

arkpar mentioned this pull request Sep 27, 2016

Canonical state cache (master) #2311

Merged

Canonical state cache #2308

Canonical state cache #2308

Conversation

arkpar commented Sep 24, 2016 • edited Loading

rphmeier commented Sep 24, 2016

Choose a reason for hiding this comment

coveralls commented Sep 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork left a comment

Choose a reason for hiding this comment

coveralls commented Sep 24, 2016

gavofyork commented Sep 24, 2016

gavofyork left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Sep 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 25, 2016

coveralls commented Sep 25, 2016

gavofyork left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Sep 27, 2016

coveralls commented Sep 27, 2016

coveralls commented Sep 27, 2016 • edited Loading

gavofyork left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork commented Sep 27, 2016

arkpar commented Sep 27, 2016 • edited Loading

coveralls commented Sep 27, 2016

arkpar commented Sep 24, 2016 •

edited

Loading

coveralls commented Sep 27, 2016 •

edited

Loading

arkpar Sep 27, 2016 •

edited

Loading

arkpar commented Sep 27, 2016 •

edited

Loading