trie: improve Iterator performance

The old iterator performed a full lookup for each call to Next, loading
nodes over and over again. It still worked well enough because the
global node cache prevented most DB loads. The Iterator in this commit
builds on top of NodeIterator to ensure that each node is loaded exactly
once without relying on the global cache.

trie/iterator.go

@@ -16,18 +16,13 @@
 
 package trie
 
-import (
+import "github.com/ethereum/go-ethereum/common"
-	"bytes"
-	"fmt"
 
-	"github.com/ethereum/go-ethereum/common"
+// Iterator is a key-value trie iterator that traverses a Trie.
-	"github.com/ethereum/go-ethereum/logger"
-	"github.com/ethereum/go-ethereum/logger/glog"
-)
-
-// Iterator is a key-value trie iterator to traverse the data contents.
 type Iterator struct {
 	trie   *Trie
+	nodeIt *NodeIterator
+	keyBuf []byte
 
 	Key   []byte // Current data key on which the iterator is positioned on
 	Value []byte // Current data value on which the iterator is positioned on
@@ -35,119 +30,45 @@ type Iterator struct {
 
 // NewIterator creates a new key-value iterator.
 func NewIterator(trie *Trie) *Iterator {
-	return &Iterator{trie: trie, Key: nil}
+	return &Iterator{
+		trie:   trie,
+		nodeIt: NewNodeIterator(trie),
+		keyBuf: make([]byte, 0, 64),
+		Key:    nil,
+	}
 }
 
-// Next moves the iterator forward with one key-value entry.
+// Next moves the iterator forward one key-value entry.
-func (self *Iterator) Next() bool {
+func (it *Iterator) Next() bool {
-	isIterStart := false
+	for it.nodeIt.Next() {
-	if self.Key == nil {
+		if it.nodeIt.Leaf {
-		isIterStart = true
+			it.Key = it.makeKey()
-		self.Key = make([]byte, 32)
+			it.Value = it.nodeIt.LeafBlob
+			return true
+		}
+	}
+	it.Key = nil
+	it.Value = nil
+	return false
 }
 
-	key := remTerm(compactHexDecode(self.Key))
+func (it *Iterator) makeKey() []byte {
-	k := self.next(self.trie.root, key, isIterStart)
+	key := it.keyBuf[:0]
-
+	for _, se := range it.nodeIt.stack {
-	self.Key = []byte(decodeCompact(k))
+		switch node := se.node.(type) {
-
-	return len(k) > 0
-}
-
-func (self *Iterator) next(node interface{}, key []byte, isIterStart bool) []byte {
-	if node == nil {
-		return nil
-	}
-
-	switch node := node.(type) {
 		case fullNode:
-		if len(key) > 0 {
+			if se.child <= 16 {
-			k := self.next(node.Children[key[0]], key[1:], isIterStart)
+				key = append(key, byte(se.child))
-			if k != nil {
-				return append([]byte{key[0]}, k...)
 			}
-		}
-
-		var r byte
-		if len(key) > 0 {
-			r = key[0] + 1
-		}
-
-		for i := r; i < 16; i++ {
-			k := self.key(node.Children[i])
-			if k != nil {
-				return append([]byte{i}, k...)
-			}
-		}
-
 		case shortNode:
-		k := remTerm(node.Key)
+			if hasTerm(node.Key) {
-		if vnode, ok := node.Val.(valueNode); ok {
+				key = append(key, node.Key[:len(node.Key)-1]...)
-			switch bytes.Compare([]byte(k), key) {
-			case 0:
-				if isIterStart {
-					self.Value = vnode
-					return k
-				}
-			case 1:
-				self.Value = vnode
-				return k
-			}
 			} else {
-			cnode := node.Val
+				key = append(key, node.Key...)
-
-			var ret []byte
-			skey := key[len(k):]
-			if bytes.HasPrefix(key, k) {
-				ret = self.next(cnode, skey, isIterStart)
-			} else if bytes.Compare(k, key[:len(k)]) > 0 {
-				return self.key(node)
-			}
-
-			if ret != nil {
-				return append(k, ret...)
 			}
 		}
-
-	case hashNode:
-		rn, err := self.trie.resolveHash(node, nil, nil)
-		if err != nil && glog.V(logger.Error) {
-			glog.Errorf("Unhandled trie error: %v", err)
 	}
-		return self.next(rn, key, isIterStart)
+	return decodeCompact(key)
-	}
-	return nil
-}
-
-func (self *Iterator) key(node interface{}) []byte {
-	switch node := node.(type) {
-	case shortNode:
-		// Leaf node
-		k := remTerm(node.Key)
-		if vnode, ok := node.Val.(valueNode); ok {
-			self.Value = vnode
-			return k
-		}
-		return append(k, self.key(node.Val)...)
-	case fullNode:
-		if node.Children[16] != nil {
-			self.Value = node.Children[16].(valueNode)
-			return []byte{16}
-		}
-		for i := 0; i < 16; i++ {
-			k := self.key(node.Children[i])
-			if k != nil {
-				return append([]byte{byte(i)}, k...)
-			}
-		}
-	case hashNode:
-		rn, err := self.trie.resolveHash(node, nil, nil)
-		if err != nil && glog.V(logger.Error) {
-			glog.Errorf("Unhandled trie error: %v", err)
-		}
-		return self.key(rn)
-	}
-	return nil
 }
 
 // nodeIteratorState represents the iteration state at one particular node of the
@@ -199,25 +120,27 @@ func (it *NodeIterator) Next() bool {
 
 // step moves the iterator to the next node of the trie.
 func (it *NodeIterator) step() error {
-	// Abort if we reached the end of the iteration
 	if it.trie == nil {
+		// Abort if we reached the end of the iteration
 		return nil
 	}
-	// Initialize the iterator if we've just started, or pop off the old node otherwise
 	if len(it.stack) == 0 {
-		// Always start with a collapsed root
+		// Initialize the iterator if we've just started.
 		root := it.trie.Hash()
-		it.stack = append(it.stack, &nodeIteratorState{node: hashNode(root[:]), child: -1})
+		it.stack = append(it.stack, &nodeIteratorState{
-		if it.stack[0].node == nil {
+			hash:  root,
-			return fmt.Errorf("root node missing: %x", it.trie.Hash())
+			node:  it.trie.root,
-		}
+			child: -1,
+		})
 	} else {
+		// Continue iterating at the previous node otherwise.
 		it.stack = it.stack[:len(it.stack)-1]
 		if len(it.stack) == 0 {
 			it.trie = nil
 			return nil
 		}
 	}
+
 	// Continue iteration to the next child
 	for {
 		parent := it.stack[len(it.stack)-1]
@@ -232,7 +155,12 @@ func (it *NodeIterator) step() error {
 			}
 			for parent.child++; parent.child < len(node.Children); parent.child++ {
 				if current := node.Children[parent.child]; current != nil {
-					it.stack = append(it.stack, &nodeIteratorState{node: current, parent: ancestor, child: -1})
+					it.stack = append(it.stack, &nodeIteratorState{
+						hash:   common.BytesToHash(node.hash),
+						node:   current,
+						parent: ancestor,
+						child:  -1,
+					})
 					break
 				}
 			}
@@ -242,7 +170,12 @@ func (it *NodeIterator) step() error {
 				break
 			}
 			parent.child++
-			it.stack = append(it.stack, &nodeIteratorState{node: node.Val, parent: ancestor, child: -1})
+			it.stack = append(it.stack, &nodeIteratorState{
+				hash:   common.BytesToHash(node.hash),
+				node:   node.Val,
+				parent: ancestor,
+				child:  -1,
+			})
 		} else if hash, ok := parent.node.(hashNode); ok {
 			// Hash node, resolve the hash child from the database, then the node itself
 			if parent.child >= 0 {
@@ -254,7 +187,12 @@ func (it *NodeIterator) step() error {
 			if err != nil {
 				return err
 			}
-			it.stack = append(it.stack, &nodeIteratorState{hash: common.BytesToHash(hash), node: node, parent: ancestor, child: -1})
+			it.stack = append(it.stack, &nodeIteratorState{
+				hash:   common.BytesToHash(hash),
+				node:   node,
+				parent: ancestor,
+				child:  -1,
+			})
 		} else {
 			break
 		}

trie/iterator_test.go

@@ -34,21 +34,60 @@ func TestIterator(t *testing.T) {
 		{"dog", "puppy"},
 		{"somethingveryoddindeedthis is", "myothernodedata"},
 	}
-	v := make(map[string]bool)
+	all := make(map[string]string)
 	for _, val := range vals {
-		v[val.k] = false
+		all[val.k] = val.v
 		trie.Update([]byte(val.k), []byte(val.v))
 	}
 	trie.Commit()
 
+	found := make(map[string]string)
 	it := NewIterator(trie)
 	for it.Next() {
-		v[string(it.Key)] = true
+		found[string(it.Key)] = string(it.Value)
 	}
 
-	for k, found := range v {
+	for k, v := range all {
-		if !found {
+		if found[k] != v {
-			t.Error("iterator didn't find", k)
+			t.Errorf("iterator value mismatch for %s: got %q want %q", k, found[k], v)
+		}
+	}
+}
+
+type kv struct {
+	k, v []byte
+	t    bool
+}
+
+func TestIteratorLargeData(t *testing.T) {
+	trie := newEmpty()
+	vals := make(map[string]*kv)
+
+	for i := byte(0); i < 255; i++ {
+		value := &kv{common.LeftPadBytes([]byte{i}, 32), []byte{i}, false}
+		value2 := &kv{common.LeftPadBytes([]byte{10, i}, 32), []byte{i}, false}
+		trie.Update(value.k, value.v)
+		trie.Update(value2.k, value2.v)
+		vals[string(value.k)] = value
+		vals[string(value2.k)] = value2
+	}
+
+	it := NewIterator(trie)
+	for it.Next() {
+		vals[string(it.Key)].t = true
+	}
+
+	var untouched []*kv
+	for _, value := range vals {
+		if !value.t {
+			untouched = append(untouched, value)
+		}
+	}
+
+	if len(untouched) > 0 {
+		t.Errorf("Missed %d nodes", len(untouched))
+		for _, value := range untouched {
+			t.Error(value)
 		}
 	}
 }

trie/secure_trie.go

@@ -174,12 +174,7 @@ func (t *SecureTrie) CommitTo(db DatabaseWriter) (root common.Hash, err error) {
 		}
 		t.secKeyCache = make(map[string][]byte)
 	}
-	n, clean, err := t.trie.hashRoot(db)
+	return t.trie.CommitTo(db)
-	if err != nil {
-		return (common.Hash{}), err
-	}
-	t.trie.root = clean
-	return common.BytesToHash(n.(hashNode)), nil
 }
 
 func (t *SecureTrie) secKey(key []byte) []byte {

trie/trie_test.go

@@ -359,44 +359,6 @@ func TestLargeValue(t *testing.T) {
 
 }
 
-type kv struct {
-	k, v []byte
-	t    bool
-}
-
-func TestLargeData(t *testing.T) {
-	trie := newEmpty()
-	vals := make(map[string]*kv)
-
-	for i := byte(0); i < 255; i++ {
-		value := &kv{common.LeftPadBytes([]byte{i}, 32), []byte{i}, false}
-		value2 := &kv{common.LeftPadBytes([]byte{10, i}, 32), []byte{i}, false}
-		trie.Update(value.k, value.v)
-		trie.Update(value2.k, value2.v)
-		vals[string(value.k)] = value
-		vals[string(value2.k)] = value2
-	}
-
-	it := NewIterator(trie)
-	for it.Next() {
-		vals[string(it.Key)].t = true
-	}
-
-	var untouched []*kv
-	for _, value := range vals {
-		if !value.t {
-			untouched = append(untouched, value)
-		}
-	}
-
-	if len(untouched) > 0 {
-		t.Errorf("Missed %d nodes", len(untouched))
-		for _, value := range untouched {
-			t.Error(value)
-		}
-	}
-}
-
 func BenchmarkGet(b *testing.B)      { benchGet(b, false) }
 func BenchmarkGetDB(b *testing.B)    { benchGet(b, true) }
 func BenchmarkUpdateBE(b *testing.B) { benchUpdate(b, binary.BigEndian) }