trie: add difference iterator #3637

Arachnid · 2017-02-02T18:20:23Z

This PR implements a differenceIterator, which allows iterating over trie nodes that exist in one trie but not in another. This is a prerequisite for most GC strategies, in order to find obsolete nodes.

mention-bot · 2017-02-02T18:20:24Z

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

GitCop · 2017-02-02T18:20:25Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: 2b78c04
Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

karalabe · 2017-02-14T14:52:12Z

core/state/iterator.go


 	accountHash common.Hash // Hash of the node containing the account
 	codeHash    common.Hash // Hash of the contract source code
 	code        []byte      // Source code associated with a contract

 	Hash   common.Hash // Hash of the current entry being iterated (nil if not standalone)
-	Entry  interface{} // Current state entry being iterated (internal representation)


Did you delete this because it wasn't used anywhere? Any particular reason? Doesn't this make the iterator a lot less useful as there's no way to actually access the data being iterated?

Yes, I deleted it because it isn't being used anywhere. Because the entry types are all private anyway, this field wasn't usable outside the package.

karalabe · 2017-02-14T15:01:13Z

trie/iterator.go

 		nodeIt: NewNodeIterator(trie),
-		keyBuf: make([]byte, 0, 64),
+		Key:    nil,


This is not needed, nil is the default value anyway.

Ha, just saw this was in the original code too. Anyway, probably should be deleted.

karalabe · 2017-02-14T15:03:31Z

trie/iterator.go

+// FromNodeIterator creates a new key-value iterator from a node iterator
+func FromNodeIterator(it NodeIterator) *Iterator {
+	return &Iterator{
+		nodeIt: it,
 		Key:    nil,


Key: nil shouldn't be needed.

karalabe · 2017-02-14T15:04:45Z

trie/iterator.go

+}
+
+// FromNodeIterator creates a new key-value iterator from a node iterator
+func FromNodeIterator(it NodeIterator) *Iterator {


I think the correct name is NewIteratorFromNodeIterator. A bit unwieldy but NewSomething should be part of the method signature to make it visible what it does. I'm open to suggestions though :D

NewIterator

Except that already exists

karalabe · 2017-02-14T15:05:50Z

trie/iterator.go

-	}
-	return decodeCompact(key)
+// NodeIterator is an iterator to traverse the trie pre-order.
+type NodeIterator interface {


Given that this interface is a public one, please document all the methods.

A few special mentions probably warranted for:

The caller of the interface methods must not modify the returned values (at least for LeadBlob and Path), since those are not copied by the iterator, just returned as is.

The caller should not retain the result of Path or should make a copy, since the next invocation to "Next" will potentially change its content.

karalabe · 2017-02-14T15:13:34Z

trie/iterator.go

+
+// Hash returns the hash of the current node
+func (it *nodeIterator) Hash() common.Hash {
+	if it.trie == nil {


This is up for debate, but I think it would be cleaner to have if len(it.stack) == 0 here, rather than looking at it.trie == nil because below you are indexing into the stack and you need an extra "mental jump" to realize what the connection between it.trie and it.stack is.

Same for the methods below.

karalabe · 2017-02-14T15:15:09Z

trie/iterator.go

+		return nil
+	}
+
+	if node, ok := it.stack[len(it.stack)-1].node.(valueNode); ok {


Not sure about it but I think you could convert to []byte directly here instead of valueNode first and then []byte later.

You can't, unfortunately.

karalabe · 2017-02-14T15:17:00Z

trie/iterator.go

-func (it *NodeIterator) Next() bool {
+// sets the Error field to the encountered failure. If `children` is false,
+// skips iterating over any subnodes of the current node.
+func (it *nodeIterator) Next(children bool) bool {


Nitpick. Maybe descend would be a more appropriate name than children? It more verb-y, at least fits better with a bool.

karalabe · 2017-02-14T15:23:44Z

trie/iterator_test.go

-			hashes[it.Hash] = struct{}{}
+	for it := NewNodeIterator(trie); it.Next(true); {
+		if it.Hash() != (common.Hash{}) {
+			hashes[it.Hash()] = struct{}{}


It's fine here since it's only a test, but live code should cache the result of .Hash() since it allocates a new common.Hash memory area. Just saying to make sure any code after this will be ok :)

karalabe · 2017-02-14T15:48:53Z

trie/iterator.go

-func (it *NodeIterator) retrieve() bool {
-	// Clear out any previously set values
-	it.Hash, it.Node, it.Parent, it.Leaf, it.LeafBlob = common.Hash{}, nil, common.Hash{}, false, nil
+type differenceIterator struct {


Please add some dox. I know it's only an internal type, but often it helps when skimming the code if internals have at least slight docs.

karalabe · 2017-02-14T15:51:00Z

trie/iterator.go

+		a:     a,
+		b:     b,
+		eof:   false,
+		count: 0,


You don't need to set eof to false and count to 0, those are the default values.

karalabe · 2017-02-14T15:51:28Z

trie/iterator.go

-	// If the iteration's done, return no available data
-	if it.trie == nil {
+// NewDifferenceIterator constructs a NodeIterator that iterates over elements in b that
+// are not in a.


Please also mention what the int pointer return is, and why it's a pointer.

karalabe · 2017-02-14T15:53:07Z

trie/iterator.go

-	it.Hash, it.Node, it.Parent, it.Leaf, it.LeafBlob = common.Hash{}, nil, common.Hash{}, false, nil
+type differenceIterator struct {
+	a, b  NodeIterator
+	eof   bool


Please document that eof marks the end of a.

karalabe · 2017-02-14T15:54:58Z

trie/iterator.go

+
+	for {
+		apath, bpath := it.a.Path(), it.b.Path()
+		cmp := bytes.Compare(apath, bpath)


This would probably be nicer with:

switch bytes.Compare(it.a.Path(), it.b.Path()) { case -1: // ... case 1: // ... default: // ... }

Maybe not, see and chose whichever :)

karalabe · 2017-02-14T16:00:34Z

trie/iterator.go

+			// a and b are identical; skip this whole subtree if the nodes have hashes
+			if !it.b.Next(it.a.Hash() == common.Hash{}) {
+				return false
+			}


I think you ought to increment count by one here, and by one below. Otherwise you might miss a count if a goes EOF.

karalabe · 2017-02-14T16:04:44Z

trie/iterator.go

+			}
+
+			// a and b are identical; skip this whole subtree if the nodes have hashes
+			if !it.b.Next(it.a.Hash() == common.Hash{}) {


Should be the same as the current one, but at least it doesn't look like an accident :P

I've refactored a bit to make the logic clearer.

GitCop · 2017-02-21T11:08:23Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: 2b78c04
Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

karalabe

SGTM

Arachnid added 5 commits January 16, 2017 16:21

trie: Make iterator postorder instead of preorder

71a442f

trie, core/state: Add 'children' parameter to TrieIterator.next()

4d7c0ca

Move key generation to NodeIterator

2b78c04

core/state, trie: Rewrite NodeIterator as an interface

f105ac0

trie: Implement differenceIterator

d21b527

obscuren added the in progress label Feb 2, 2017

Arachnid requested review from fjl and obscuren February 2, 2017 18:20

Arachnid added review and removed in progress labels Feb 2, 2017

fjl changed the title ~~Implement differenceIterator~~ trie: add difference iterator Feb 3, 2017

Arachnid requested a review from karalabe February 3, 2017 19:09

karalabe reviewed Feb 14, 2017

View reviewed changes

trie: Fixes in response to review

3015bfd

karalabe approved these changes Feb 22, 2017

View reviewed changes

karalabe added this to the 1.6.0 milestone Feb 22, 2017

fjl approved these changes Feb 22, 2017

View reviewed changes

fjl merged commit 5552734 into ethereum:master Feb 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trie: add difference iterator #3637

trie: add difference iterator #3637

Arachnid commented Feb 2, 2017

mention-bot commented Feb 2, 2017

GitCop commented Feb 2, 2017

karalabe Feb 14, 2017

Arachnid Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

fjl Feb 15, 2017

karalabe Feb 15, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

Arachnid Feb 21, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017

karalabe Feb 14, 2017 •

edited

karalabe Feb 14, 2017

karalabe Feb 14, 2017

Arachnid Feb 21, 2017

GitCop commented Feb 21, 2017

karalabe left a comment

trie: add difference iterator #3637

trie: add difference iterator #3637

Conversation

Arachnid commented Feb 2, 2017

mention-bot commented Feb 2, 2017

GitCop commented Feb 2, 2017

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

Choose a reason for hiding this comment

karalabe Feb 14, 2017 • edited

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

GitCop commented Feb 21, 2017

karalabe left a comment

Choose a reason for hiding this comment

karalabe Feb 14, 2017 •

edited