trie: review feedback, mainly docs + minor changes

trie/pure_committer.go

@@ -26,6 +26,10 @@ import (
 	"golang.org/x/crypto/sha3"
 )
 
+// LeafChanSize is the size of the leafCh. It's a pretty arbitrary number, to allow
+// some paralellism but not incur too much memory overhead.
+const LeafChanSize = 200
+
 // Leaf represents a trie leaf value
 type Leaf struct {
 	size   int         // size of the rlp data (estimate)
@@ -34,6 +38,12 @@ type Leaf struct {
 	vnodes bool        // set to true if the node (possibly) contains a valueNode
 }
 
+// committer is a type used for the trie Commit operation. A committer has some
+// internal preallocated temp space, and also a callback that is invoked when
+// leaves are committed. The leafs are passed through the `leafCh`,  to allow
+// some level of paralellism.
+// By 'some level' of paralellism, it's still the case that all leaves will be
+// processed sequentially - onleaf will never be called in paralell or out of order.
 type committer struct {
 	tmp sliceBuffer
 	sha keccakState
@@ -52,11 +62,17 @@ var committerPool = sync.Pool{
 	},
 }
 
+// newCommitter creates a new committer or picks one from the pool, and
+// initializes the leafCh, if needed.
+// In case no onleaf-callback is provided, the committer does not
+// use a channel-based commit, but inlined.
+// Typically, the account trie is committed with a channel-based leaf-commit,
+// whereas storage tries are committed 'inline'.
 func newCommitter(onleaf LeafCallback) *committer {
 	h := committerPool.Get().(*committer)
 	h.onleaf = onleaf
 	if onleaf != nil {
-		h.leafCh = make(chan *Leaf, 200) // arbitrary number
+		h.leafCh = make(chan *Leaf, LeafChanSize)
 	}
 	return h
 }
@@ -200,8 +216,7 @@ func (h *committer) store(n node, db *Database, force bool, hasVnodeChildren boo
 }
 
 // commitLoop does the actual insert + leaf callback for nodes
-func (h *committer) commitLoop(db *Database, wg *sync.WaitGroup) {
+func (h *committer) commitLoop(db *Database) {
-	defer wg.Done()
 	for item := range h.leafCh {
 		var (
 			hash      = item.hash

trie/pure_hasher.go

@@ -23,6 +23,8 @@ import (
 	"golang.org/x/crypto/sha3"
 )
 
+// pureHasher is a type used for the trie Hash operation. A pureHasher has some
+// internal preallocated temp space
 type pureHasher struct {
 	sha keccakState
 
@@ -30,7 +32,7 @@ type pureHasher struct {
 	tmpKey []byte
 }
 
-// hashers live in a global db.
+// pureHasherPool holds pureHashers
 var pureHasherPool = sync.Pool{
 	New: func() interface{} {
 		return &pureHasher{
@@ -139,7 +141,7 @@ func (h *pureHasher) shortnodeToHash(n *shortNode, force bool) node {
 func (h *pureHasher) fullnodeToHash(n *fullNode, force bool) node {
 	h.tmp.Reset()
 	// Generate the RLP encoding of the node
-	if err := rlp.Encode(&h.tmp, n); err != nil {
+	if err := n.EncodeRLP(&h.tmp); err != nil {
 		panic("encode error: " + err.Error())
 	}
 

trie/trie.go

@@ -410,27 +410,8 @@ func (t *Trie) Hash() common.Hash {
 	return common.BytesToHash(hash.(hashNode))
 }
 
-// oldCommit is the old implementation of Commit, which uses the
+// Commit writes all nodes to the trie's memory database, tracking the internal
-// regular hasher.
-// It writes all nodes to the trie's memory database, tracking the internal
 // and external (for account tries) references.
-func (t *Trie) oldCommit(onleaf LeafCallback) (root common.Hash, err error) {
-	if t.db == nil {
-		panic("commit called on trie with nil database")
-	}
-	if t.root == nil {
-		return emptyRoot, nil
-	}
-	h := newHasher(onleaf)
-	defer returnHasherToPool(h)
-	hash, cached, err := h.hash(t.root, t.db, true)
-	if err != nil {
-		return common.Hash{}, err
-	}
-	t.root = cached
-	return common.BytesToHash(hash.(hashNode)), nil
-}
-
 func (t *Trie) Commit(onleaf LeafCallback) (root common.Hash, err error) {
 	if t.db == nil {
 		panic("commit called on trie with nil database")
@@ -450,10 +431,17 @@ func (t *Trie) Commit(onleaf LeafCallback) (root common.Hash, err error) {
 	var wg sync.WaitGroup
 	if onleaf != nil {
 		wg.Add(1)
-		go h.commitLoop(t.db, &wg)
+		go func() {
+			defer wg.Done()
+			h.commitLoop(t.db)
+		}()
 	}
 	_, err = h.commit(t.root, t.db, true)
 	if onleaf != nil {
+		// The leafch is created in newCommitter if there was an onleaf callback
+		// provided. The commitLoop only _reads_ from it, and the commit
+		// operation was the sole writer. Therefore, it's safe to close this
+		// channel here.
 		close(h.leafCh)
 		wg.Wait()
 	}