trie, core/state: improve memory usage and performance #3135

fjl · 2016-10-14T04:35:46Z

This PR adds a new cache eviction strategy to the trie and improves the performance
of basic Get/Insert operations.

mention-bot · 2016-10-14T04:35:47Z

@fjl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @karalabe, @zsfelfoldi and @obscuren to be potential reviewers.

This avoids memory copies when unwrapping node interface values. name old time/op new time/op delta Get 388ns ± 8% 215ns ± 2% -44.56% (p=0.000 n=15+15) GetDB 363ns ± 3% 202ns ± 2% -44.21% (p=0.000 n=15+15) UpdateBE 1.57µs ± 2% 1.29µs ± 3% -17.80% (p=0.000 n=13+15) UpdateLE 1.92µs ± 2% 1.61µs ± 2% -16.25% (p=0.000 n=14+14) HashBE 2.16µs ± 6% 2.18µs ± 6% ~ (p=0.436 n=15+15) HashLE 7.43µs ± 3% 7.21µs ± 3% -2.96% (p=0.000 n=15+13)

Nodes decoded from a DB load kept hashes and values as sub-slices of the DB value. This can be a problem because loading from leveldb often returns []byte with a cap that's larger than necessary, increasing memory usage.

Arachnid · 2016-10-14T07:28:13Z

core/state/statedb.go

@@ -43,6 +43,9 @@ const (
 	// is max uncle depth + 1.
 	maxTrieCacheLength = 8


While you're at it, would you mind renaming this? It's really confusing.

Arachnid · 2016-10-14T07:42:28Z

trie/node.go

+// canUnload tells whether a node can be unloaded.
+func (n nodeFlag) canUnload(cachegen, cachelimit uint16) bool {
+	var dist uint16
+	if n.gen > cachegen {


This if statement should not be here. Suppose n.gen = 65534; when cachegen = 65535, you calculate (correctly) cachegen - n.gen = 65535 - 65534 = 1. When cachegen = 0 you calculate (incorrectly) n.gen - cachegen = 65534 - 0 = 65534.

If instead you always compute cachegen - n.gen, the result in the last case is 0 - 65534 = -65534 = 2 mod 65536.

Are you able to add a test for integer overflow?

I added the extra check this morning because I got worried about overflow issues. It had n.gen - cachegen before. I'll add a test instead.

Arachnid · 2016-10-14T07:44:13Z

trie/node.go

-func (n shortNode) String() string { return n.fstring("") }
-func (n hashNode) String() string  { return n.fstring("") }
-func (n valueNode) String() string { return n.fstring("") }
+func (n *fullNode) String() string  { return n.fstring("") }


Can you add a bit to the PR about why fullNodes and shortNodes are now pointers, but hashNodes and valueNodes still aren't? Are you sure this is an improvement?

My guess is that both hash and value nodes are already byte slices. So they are effectively 1 x pointer + 2 x int.

Yeah that's the reason. I doubt that there'd be much of a difference with pointers to slices.

I'm a little worried about unintuitive differences in how things behave when some things are pointers and some are not, though. In general, pointer-izing the nodes in this PR seems like it introduces extra risk.

Did you see the benchmark (it's in the commit message).

Arachnid · 2016-10-14T07:52:21Z

trie/hasher.go

+		if db == nil {
+			return hash, n, nil
+		}
+		if n.canUnload(h.cachegen, h.cachelimit) {


Could you add a comment somewhere explaining when cache eviction happens? If I'm understanding correctly, whenever a node is modified, we evaluate all its unmodified children for eviction, in effect. Meaning we could retain parts of the trie much longer than the cache policy implies, but as soon as a parent node is modified, we'll notice and discard any old nodes.

This also means that we could theoretically retain a node so long that the generation counter wraps around a 16 bit int and the node appears recent even though it's not. With 100 generation retention and a 16 bit int, this should happen at most 100/65536 = .15% of the time, which seems acceptable.

It's also an invariant, if I'm not mistaken, that the cache generation of a node is >= that of all its subnodes, meaning when a node expires, we can be certain all its subnodes are also expired and discardable; it'd be good to note that, too.

karalabe · 2016-10-14T08:07:51Z

trie/hasher.go

-				return n, original, err
+			collapsed.Val, cached.Val, err = h.hash(n.Val, db, false)
+			if err != nil {
+				return original, original, err


Why did the first argument change from n to original? Shouldn't it in this case be collapsed? In the original code n had it's Key compactEncode-ed, whereas the new code doesn't do that.

You're right. It doesn't really change semantics because this is the error case but probably needs to be fixed.

karalabe · 2016-10-14T08:12:44Z

trie/hasher.go

 		// Hash the full node's children, caching the newly hashed subtrees
-		cached := fullNode{dirty: n.dirty}
+		collapsed, cached := n.copy(), n.copy()


Wouldn't it save a few copy ops if we didn't copy the entire n as in the original code into cache, rather just the few fields really needed?

copy is a shallow copy.

karalabe · 2016-10-14T08:14:22Z

trie/hasher.go

-					return n, original, err
+				collapsed.Children[i], cached.Children[i], err = h.hash(n.Children[i], db, false)
+				if err != nil {
+					return original, original, err


The semantics changed here too a bit. The old code was returning a copy of original as the first parameter and we're here returning the original itself (instead of the collapsed copy). It may be correct, but I'd curious to hear the rationale behind it.

karalabe · 2016-10-14T08:34:30Z

trie/node.go

 	key := compactDecode(kbuf)
 	if key[len(key)-1] == 16 {
 		// value node
 		val, _, err := rlp.SplitString(rest)
 		if err != nil {
 			return nil, fmt.Errorf("invalid value node: %v", err)
 		}
-		return shortNode{key, valueNode(val), hash, false}, nil
+		return &shortNode{key, append(valueNode{}, val...), flag}, nil


I'm not exactly sure how Go behaves in this case. Are we sure it won't allocate more memory that needed if we append? In theory appending elements grows the underlying array exponentially. Maybe appending to an empty array could be a special case, but I can equally easily imagine that it will leave a lot of extra allocated but unused bytes at the end.

AFAIK append to empty does the right thing.

karalabe · 2016-10-14T08:36:36Z

trie/node.go

+	} else {
+		dist = cachegen - n.gen
+	}
+	return !n.dirty && dist > cachelimit


Curious whether we want dist > cachelimit or dist >= cachelimit. Specifically thinking about the special case where cachelimit == 0 (I've seen it some places throughout the code).

It should probably be >=. That way cachelimit zero discards all nodes on Commit every time.

karalabe · 2016-10-14T08:39:16Z

trie/secure_trie.go

-	return &SecureTrie{
-		trie: *trie,
-	}, nil
+	trie.SetCacheLimit(cachelimit)


Any particular reason for having a separate SetCacheLimit method and not add it (cachelimit param) to the New (the same way it's part of NewSecure)?

I did this because most Trie instances don't need the cache. The idea is that caching is an optional feature of Trie but you must choose a cache size for SecureTrie. Does that make sense?

Arachnid

Would you mind renaming maxTrieCacheLength while you're working on this file?

Arachnid · 2016-10-14T13:08:47Z

trie/node.go

-		dist = cachegen - n.gen
-	}
-	return !n.dirty && dist > cachelimit
+	return !n.dirty && cachegen-n.gen >= cachelimit


Style nit: spacing around -.

gofmt chooses the spacing

…ce (ethereum#3135) * trie: store nodes as pointers This avoids memory copies when unwrapping node interface values. name old time/op new time/op delta Get 388ns ± 8% 215ns ± 2% -44.56% (p=0.000 n=15+15) GetDB 363ns ± 3% 202ns ± 2% -44.21% (p=0.000 n=15+15) UpdateBE 1.57µs ± 2% 1.29µs ± 3% -17.80% (p=0.000 n=13+15) UpdateLE 1.92µs ± 2% 1.61µs ± 2% -16.25% (p=0.000 n=14+14) HashBE 2.16µs ± 6% 2.18µs ± 6% ~ (p=0.436 n=15+15) HashLE 7.43µs ± 3% 7.21µs ± 3% -2.96% (p=0.000 n=15+13) * trie: close temporary databases in GetDB benchmark * trie: don't keep []byte from DB load around Nodes decoded from a DB load kept hashes and values as sub-slices of the DB value. This can be a problem because loading from leveldb often returns []byte with a cap that's larger than necessary, increasing memory usage. * trie: unload old cached nodes * trie, core/state: use cache unloading for account trie * trie: use explicit private flags (fixes Go 1.5 reflection issue). * trie: fixup cachegen overflow at request of nick * core/state: rename journal size constant (cherry picked from commit 40cdcf1)

rjl493456442 · 2016-10-23T11:00:02Z

In this PR, "old" not will be evicted if it is old enough. But how can this approach improve the basic GET/INSERT method's performance? There can be redundant resolve from database between two get action, which make me really confused

fjl added review labels Oct 14, 2016

fjl force-pushed the trie-perf branch 3 times, most recently from d5e3508 to 199c9a3 Compare October 14, 2016 07:10

fjl added 5 commits October 14, 2016 09:11

trie: close temporary databases in GetDB benchmark

2819f1c

trie: don't keep []byte from DB load around

36e4f76

Nodes decoded from a DB load kept hashes and values as sub-slices of the DB value. This can be a problem because loading from leveldb often returns []byte with a cap that's larger than necessary, increasing memory usage.

trie: unload old cached nodes

c4568fb

trie, core/state: use cache unloading for account trie

b74240f

fjl force-pushed the trie-perf branch from 199c9a3 to b74240f Compare October 14, 2016 07:12

Arachnid reviewed Oct 14, 2016

View reviewed changes

karalabe reviewed Oct 14, 2016

View reviewed changes

karalabe and others added 2 commits October 14, 2016 12:48

trie: use explicit private flags (fixes Go 1.5 reflection issue).

543ed15

trie: fixup cachegen overflow at request of nick

64097b8

Arachnid approved these changes Oct 14, 2016

View reviewed changes

core/state: rename journal size constant

27a6fa4

karalabe merged commit 40cdcf1 into ethereum:develop Oct 14, 2016

obscuren removed the review label Oct 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trie, core/state: improve memory usage and performance #3135

trie, core/state: improve memory usage and performance #3135

fjl commented Oct 14, 2016 •

edited

Loading

mention-bot commented Oct 14, 2016

Arachnid Oct 14, 2016

Arachnid Oct 14, 2016

fjl Oct 14, 2016

Arachnid Oct 14, 2016

karalabe Oct 14, 2016

fjl Oct 14, 2016

Arachnid Oct 14, 2016

fjl Oct 14, 2016

Arachnid Oct 14, 2016

karalabe Oct 14, 2016

fjl Oct 14, 2016

karalabe Oct 14, 2016

fjl Oct 14, 2016

karalabe Oct 14, 2016

karalabe Oct 14, 2016

fjl Oct 14, 2016

karalabe Oct 14, 2016

fjl Oct 14, 2016

karalabe Oct 14, 2016

fjl Oct 14, 2016

Arachnid left a comment

Arachnid Oct 14, 2016

fjl Oct 14, 2016

rjl493456442 commented Oct 23, 2016

		@@ -43,6 +43,9 @@ const (
		// is max uncle depth + 1.
		maxTrieCacheLength = 8

trie, core/state: improve memory usage and performance #3135

trie, core/state: improve memory usage and performance #3135

Conversation

fjl commented Oct 14, 2016 • edited Loading

mention-bot commented Oct 14, 2016

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

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

Arachnid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjl493456442 commented Oct 23, 2016

fjl commented Oct 14, 2016 •

edited

Loading