Fjl state cache #3028

karalabe · 2016-09-23T05:18:08Z

No description provided.

mention-bot · 2016-09-23T05:18:09Z

@karalabe, thanks for your PR! By analyzing the annotation information on this pull request, we identified @obscuren, @fjl and @Gustav-Simonsson to be potential reviewers

pirapira · 2016-09-23T07:23:16Z

core/state/state_object.go

 	}
 }

+// UpdateRoot sets the trie root to the current root hash of


Maybe this comment can be finished like "... of the trie."

pirapira · 2016-09-23T07:26:43Z

core/state/state_object.go

-	stateObject.nonce = self.nonce
+func (self *StateObject) Copy(db trie.Database) *StateObject {
+	stateObject := NewObject(self.address, self.data)
+	stateObject.data.Balance.Set(self.data.Balance)
 	stateObject.trie = self.trie
 	stateObject.code = self.code
 	stateObject.storage = self.storage.Copy()
 	stateObject.remove = self.remove
 	stateObject.dirty = self.dirty


Should dirtyCode be copied as well?

Brilliant catch. I just found it but was looking for it for an hour straight... whyyyy didn't I see your message..

holiman · 2016-09-23T13:06:58Z

core/state/state_object.go

-	stateObject.nonce = self.nonce
+func (self *StateObject) Copy(db trie.Database) *StateObject {
+	stateObject := NewObject(self.address, self.data)
+	stateObject.data.Balance.Set(self.data.Balance)


I don't understand this line.
stateObject.data is the same object as self.data at this point. So, to me, it looks like:
stateObject.data.Balance.Set(stateObject.data.Balance))

Am I missing something or is this unnecessary?

Yeah, I wanted to delete this too. Accident that it got left in.

holiman · 2016-09-23T13:19:01Z

core/state/statedb.go

 }

-// Creates creates a new state object and takes ownership. This is different from "NewStateObject"
+// Creates creates a new state object and takes ownership.


I'd just like to verify; the CreateStateObject does not stash away the data into s.all, so it does not wind up in there until after commit. Is that intended behaviour?

yes. The idea is that s.all should mirror DB content at all times.

Arachnid · 2016-09-23T14:43:06Z

core/state/statedb.go

@@ -242,12 +242,12 @@ func (self *StateDB) DeleteStateObject(stateObject *StateObject) {

 	addr := stateObject.Address()
 	self.trie.Delete(addr[:])
-	//delete(self.stateObjects, addr.Str())


Maybe just delete this?

Arachnid · 2016-09-23T14:56:15Z

core/state/state_object.go

+		return value
+	}
+	// Load from DB in case it is missing.
+	self.initTrie(db)


Maybe just have an accessor getTrie that returns it if it's not initialized instead?

Arachnid · 2016-09-23T14:58:26Z

core/state/state_object.go

+	self.data.Root = self.trie.Hash()
+}
+
+// CommitTrie the storage trie of the object to dwb.


Arachnid · 2016-09-23T15:05:03Z

core/state/statedb.go

 		if stateObject.remove {
 			// If the object has been removed, don't bother syncing it
 			// and just mark it for deletion in the trie.
 			s.DeleteStateObject(stateObject)
+			delete(s.all, addr)


Won't s.all get obliterated anyway when the state root changes?

The cache is retained as long as blocks follow a progression. It gets obliterated only if a new block is being imported that has a different parent than the previous import (since then the cached state is invalid from the new block's perspective).

This could be worked around with a journal for the cache, but that's a whole new can of worms.

Arachnid · 2016-09-23T15:11:00Z

core/blockchain.go

-	blockCache   *lru.Cache // Cache for the most recent entire blocks
-	futureBlocks *lru.Cache // future blocks are blocks added for later processing
+	stateCache   *state.StateDB // State database to reuse between imports (contains state cache)
+	bodyCache    *lru.Cache     // Cache for the most recent block bodies


Why do we have two different caches caching the same thing in different encodings? And a third caching the blocks themselves?

The caches should be thoroughly reexamined. I think noone ever had the time to dissect every cache's purpose and drop them. The only reasonable way is with a lot of benchmarking which is a PITA so noone took the initiative. I agree we should do it.

I will remove bodyCache in this PR

Arachnid · 2016-09-23T15:15:08Z

core/state/state_object.go

+// CodeSize returns the size of the contract code associated with this object.
+func (self *StateObject) CodeSize(db trie.Database) int {
+	if self.data.codeSize == nil {
+		self.data.codeSize = new(int)


Why are we using a pointer to an int here? What's wrong with -1 to indicate we haven't loaded the code size?

I usually prefer using the default value to mean non-initialized. Using -1 would require taking extra care to always initialize it properly (e.g. there is code that just did Account{}, which would be invalid from now on).

Well, using naked constructors everywhere where there are critical fields that have to be set correctly is another antipattern. :)

Gustav-Simonsson · 2016-09-23T20:05:38Z

Testing this on mainnet reveals that it does significantly speed up processing of the tx spam blocks, but only for some blocks - it looks like the regular (small) reorgs of 1-2 blocks resets the all cache - so it only speeds up spam blocks until the next reorg.

db is essentially unused and removing it shrinks StateObject by 16 bytes. I'd love to remove address as well but that's a very invasive change.

With this change, StateObject can work without loading the storage trie and contract code. This is required because we're about to cache state objects and want to reduce the amount of memory referenced by idle objects.

This change introduces a global, per-state cache that keeps account data in the canon state.

Until now the dirty flag was unset in intermediate root. That resulted in not knowing whether a state object should be writte to disk or not. Being removed in 1.4.12, we can rely on it to avoid a ton of writes.

pirapira · 2016-09-26T07:57:26Z

trie/secure_trie.go

@@ -46,7 +46,7 @@ type SecureTrie struct {
 	secKeyCache map[string][]byte
 }

-// NewSecure creates a trie with an existing root node from db.
+// NewSecure creFGetates a trie with an existing root node from db.


obscuren added the in progress label Sep 23, 2016

karalabe mentioned this pull request Sep 23, 2016

core/state: track all accounts in canon state #3027

Closed

2 tasks

pirapira reviewed Sep 23, 2016

View reviewed changes

karalabe force-pushed the fjl-state-cache branch from d45927a to a5cd548 Compare September 23, 2016 08:07

holiman reviewed Sep 23, 2016

View reviewed changes

Arachnid reviewed Sep 23, 2016

View reviewed changes

karalabe force-pushed the fjl-state-cache branch from 0db8282 to e8db0f6 Compare September 25, 2016 11:40

fjl and others added 14 commits September 26, 2016 09:18

core/state: remove StateObject.db, prepare for removal of address

4d989d0

db is essentially unused and removing it shrinks StateObject by 16 bytes. I'd love to remove address as well but that's a very invasive change.

core/state: lazy-load account storage tries and code

f1834c0

With this change, StateObject can work without loading the storage trie and contract code. This is required because we're about to cache state objects and want to reduce the amount of memory referenced by idle objects.

core/state: rename dump types to Dump*

aa2a4f8

core/state: track all accounts in canon state

8ecd29b

This change introduces a global, per-state cache that keeps account data in the canon state.

core/state: add note about code size caching

0ff5f90

core/state: robustify commit error handling

9d9c64b

core, internal: fix up the state caching

c9f5e4d

core/state: use dirty flag to avoid db writes

3b26d83

Until now the dirty flag was unset in intermediate root. That resulted in not knowing whether a state object should be writte to disk or not. Being removed in 1.4.12, we can rely on it to avoid a ton of writes.

core: retain the state database cache between import batches

8fbdde5

state: track code dirtyness and only write if changed

3c26ba4

core: cache code size retrievals to avoid db trashing

ad16f46

core/state, trie: add account existential bloom filter

d733309

core, tests: track dirty objects in separate map

e005202

Godeps: add bloom filters package to dependencies

8e4687e

karalabe force-pushed the fjl-state-cache branch from 47c1768 to 8e4687e Compare September 26, 2016 06:19

pirapira reviewed Sep 26, 2016

View reviewed changes

fjl closed this Sep 26, 2016

fjl removed the in progress label Sep 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fjl state cache #3028

Fjl state cache #3028

karalabe commented Sep 23, 2016

mention-bot commented Sep 23, 2016

pirapira Sep 23, 2016

pirapira Sep 23, 2016

karalabe Sep 23, 2016

holiman Sep 23, 2016 •

edited

Loading

karalabe Sep 23, 2016

holiman Sep 23, 2016

fjl Sep 23, 2016

Arachnid Sep 23, 2016

Arachnid Sep 23, 2016

Arachnid Sep 23, 2016

karalabe Sep 23, 2016

Arachnid Sep 23, 2016

karalabe Sep 23, 2016

Arachnid Sep 23, 2016

karalabe Sep 23, 2016

fjl Sep 23, 2016 •

edited

Loading

Arachnid Sep 23, 2016 •

edited

Loading

karalabe Sep 23, 2016

Arachnid Sep 24, 2016

Gustav-Simonsson commented Sep 23, 2016

pirapira Sep 26, 2016

Fjl state cache #3028

Fjl state cache #3028

Conversation

karalabe commented Sep 23, 2016

mention-bot commented Sep 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman Sep 23, 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

Choose a reason for hiding this comment

fjl Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Arachnid Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gustav-Simonsson commented Sep 23, 2016

Choose a reason for hiding this comment

holiman Sep 23, 2016 •

edited

Loading

fjl Sep 23, 2016 •

edited

Loading

Arachnid Sep 23, 2016 •

edited

Loading