From 572cffa0416e2ef40b92df4f985676197ddd611a Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Sun, 6 Mar 2022 20:13:39 +0000 Subject: [PATCH 1/4] wip --- lib/trie/trie.go | 174 ++++++++++++++++++------------------------ lib/trie/trie_test.go | 38 +++++---- 2 files changed, 95 insertions(+), 117 deletions(-) diff --git a/lib/trie/trie.go b/lib/trie/trie.go index e98d6b27dd..723b0a53c8 100644 --- a/lib/trie/trie.go +++ b/lib/trie/trie.go @@ -62,17 +62,33 @@ func (t *Trie) Snapshot() (newTrie *Trie) { } } +func (t *Trie) snapshotLeafOnOldGeneration(currentLeaf *node.Leaf) (newLeaf *node.Leaf) { + if currentLeaf.Generation == t.generation { + // no need to deep copy and update generation + // of current leaf. + return currentLeaf + } + + newNode := updateGeneration(currentLeaf, t.generation, t.deletedKeys) + return newNode.(*node.Leaf) +} + +func (t *Trie) snapshotBranchOnOldGeneration(currentBranch *node.Branch) (newBranch *node.Branch) { + if currentBranch.Generation == t.generation { + // no need to deep copy and update generation + // of current leaf. + return currentBranch + } + + newNode := updateGeneration(currentBranch, t.generation, t.deletedKeys) + return newNode.(*node.Branch) +} + // updateGeneration is called when the currentNode is from // an older trie generation (snapshot) so we deep copy the // node and update the generation on the newer copy. func updateGeneration(currentNode Node, trieGeneration uint64, deletedHashes map[common.Hash]struct{}) (newNode Node) { - if currentNode.GetGeneration() == trieGeneration { - panic(fmt.Sprintf( - "current node has the same generation %d as the trie generation, "+ - "make sure the caller properly checks for the node generation to "+ - "be smaller than the trie generation.", trieGeneration)) - } const copyChildren = false newNode = currentNode.Copy(copyChildren) newNode.SetGeneration(trieGeneration) @@ -322,17 +338,13 @@ func (t *Trie) insert(parent Node, key, value []byte) (newParent Node) { } // TODO ensure all values have dirty set to true - newParent = parent - if parent.GetGeneration() < t.generation { - newParent = updateGeneration(parent, t.generation, t.deletedKeys) - } - switch newParent.Type() { + switch parent.Type() { case node.BranchType, node.BranchWithValueType: - parentBranch := newParent.(*node.Branch) + parentBranch := parent.(*node.Branch) return t.insertInBranch(parentBranch, key, value) default: - parentLeaf := newParent.(*node.Leaf) + parentLeaf := parent.(*node.Leaf) return t.insertInLeaf(parentLeaf, key, value) } } @@ -340,11 +352,14 @@ func (t *Trie) insert(parent Node, key, value []byte) (newParent Node) { func (t *Trie) insertInLeaf(parentLeaf *node.Leaf, key, value []byte) (newParent Node) { if bytes.Equal(parentLeaf.Key, key) { - if !bytes.Equal(value, parentLeaf.Value) { - parentLeaf.Value = value - parentLeaf.Generation = t.generation - parentLeaf.SetDirty(true) + if bytes.Equal(value, parentLeaf.Value) { + return parentLeaf } + + parentLeaf = t.snapshotLeafOnOldGeneration(parentLeaf) + parentLeaf.Value = value + parentLeaf.Generation = t.generation + parentLeaf.SetDirty(true) return parentLeaf } @@ -364,6 +379,7 @@ func (t *Trie) insertInLeaf(parentLeaf *node.Leaf, key, if len(key) < len(parentLeafKey) { // Move the current leaf parent as a child to the new branch. + parentLeaf = t.snapshotLeafOnOldGeneration(parentLeaf) childIndex := parentLeafKey[commonPrefixLength] parentLeaf.Key = parentLeaf.Key[commonPrefixLength+1:] parentLeaf.SetDirty(true) @@ -378,6 +394,7 @@ func (t *Trie) insertInLeaf(parentLeaf *node.Leaf, key, newBranchParent.Value = parentLeaf.Value } else { // make the leaf a child of the new branch + parentLeaf = t.snapshotLeafOnOldGeneration(parentLeaf) childIndex := parentLeafKey[commonPrefixLength] parentLeaf.Key = parentLeaf.Key[commonPrefixLength+1:] parentLeaf.SetDirty(true) @@ -395,9 +412,10 @@ func (t *Trie) insertInLeaf(parentLeaf *node.Leaf, key, } func (t *Trie) insertInBranch(parentBranch *node.Branch, key, value []byte) (newParent Node) { + parentBranch = t.snapshotBranchOnOldGeneration(parentBranch) + parentBranch.SetDirty(true) + if bytes.Equal(key, parentBranch.Key) { - parentBranch.SetDirty(true) - parentBranch.Generation = t.generation parentBranch.Value = value return parentBranch } @@ -418,12 +436,10 @@ func (t *Trie) insertInBranch(parentBranch *node.Branch, key, value []byte) (new } } else { child = t.insert(child, remainingKey, value) - child.SetDirty(true) + // child.SetDirty(true) } parentBranch.Children[childIndex] = child - parentBranch.SetDirty(true) - parentBranch.Generation = t.generation return parentBranch } @@ -435,13 +451,11 @@ func (t *Trie) insertInBranch(parentBranch *node.Branch, key, value []byte) (new Generation: t.generation, Dirty: true, } - parentBranch.SetDirty(true) oldParentIndex := parentBranch.Key[commonPrefixLength] remainingOldParentKey := parentBranch.Key[commonPrefixLength+1:] parentBranch.Key = remainingOldParentKey - parentBranch.Generation = t.generation newParentBranch.Children[oldParentIndex] = parentBranch if len(key) <= commonPrefixLength { @@ -653,13 +667,8 @@ func (t *Trie) clearPrefixLimit(parent Node, prefix []byte, limit uint32) ( return nil, 0, true } - newParent = parent - if parent.GetGeneration() < t.generation { - newParent = updateGeneration(parent, t.generation, t.deletedKeys) - } - - if newParent.Type() == node.LeafType { - leaf := newParent.(*node.Leaf) + if parent.Type() == node.LeafType { + leaf := parent.(*node.Leaf) // if prefix is not found, it's also all deleted. // TODO check this is the same behaviour as in substrate const allDeleted = true @@ -667,22 +676,11 @@ func (t *Trie) clearPrefixLimit(parent Node, prefix []byte, limit uint32) ( valuesDeleted = 1 return nil, valuesDeleted, allDeleted } - // not modified so return the leaf of the original - // trie generation. The copied leaf newParent will be - // garbage collected. return parent, 0, allDeleted } - branch := newParent.(*node.Branch) - newParent, valuesDeleted, allDeleted = t.clearPrefixLimitBranch(branch, prefix, limit) - if valuesDeleted == 0 { - // not modified so return the node of the original - // trie generation. The copied newParent will be - // garbage collected. - newParent = parent - } - - return newParent, valuesDeleted, allDeleted + branch := parent.(*node.Branch) + return t.clearPrefixLimitBranch(branch, prefix, limit) } func (t *Trie) clearPrefixLimitBranch(branch *node.Branch, prefix []byte, limit uint32) ( @@ -714,13 +712,15 @@ func (t *Trie) clearPrefixLimitBranch(branch *node.Branch, prefix []byte, limit childPrefix := prefix[len(branch.Key)+1:] child := branch.Children[childIndex] - newParent = branch // mostly just a reminder for the reader - branch.Children[childIndex], valuesDeleted, allDeleted = t.clearPrefixLimit(child, childPrefix, limit) - if valuesDeleted > 0 { - branch.SetDirty(true) - newParent = handleDeletion(branch, prefix) + child, valuesDeleted, allDeleted = t.clearPrefixLimit(child, childPrefix, limit) + if valuesDeleted == 0 { + return branch, valuesDeleted, allDeleted } + branch = t.snapshotBranchOnOldGeneration(branch) + branch.Children[childIndex] = child + branch.SetDirty(true) + newParent = handleDeletion(branch, prefix) return newParent, valuesDeleted, allDeleted } @@ -738,11 +738,17 @@ func (t *Trie) clearPrefixLimitChild(branch *node.Branch, prefix []byte, limit u } nilPrefix := ([]byte)(nil) - branch.Children[childIndex], valuesDeleted = t.deleteNodesLimit(child, nilPrefix, limit) + child, valuesDeleted = t.deleteNodesLimit(child, nilPrefix, limit) + if valuesDeleted == 0 { + allDeleted = branch.Children[childIndex] == nil + return branch, valuesDeleted, allDeleted + } + + branch = t.snapshotBranchOnOldGeneration(branch) + branch.Children[childIndex] = child branch.SetDirty(true) newParent = handleDeletion(branch, prefix) - allDeleted = branch.Children[childIndex] == nil return newParent, valuesDeleted, allDeleted } @@ -757,17 +763,12 @@ func (t *Trie) deleteNodesLimit(parent Node, prefix []byte, limit uint32) ( return nil, 0 } - newParent = parent - if parent.GetGeneration() < t.generation { - newParent = updateGeneration(parent, t.generation, t.deletedKeys) - } - - if newParent.Type() == node.LeafType { + if parent.Type() == node.LeafType { valuesDeleted = 1 return nil, valuesDeleted } - branch := newParent.(*node.Branch) + branch := parent.(*node.Branch) fullKey := concatenateSlices(prefix, branch.Key) @@ -779,6 +780,7 @@ func (t *Trie) deleteNodesLimit(parent Node, prefix []byte, limit uint32) ( continue } + branch = t.snapshotBranchOnOldGeneration(branch) branch.Children[i], newDeleted = t.deleteNodesLimit(child, fullKey, limit) if branch.Children[i] == nil { nilChildren++ @@ -825,23 +827,15 @@ func (t *Trie) clearPrefix(parent Node, prefix []byte) ( return nil, false } - newParent = parent - if parent.GetGeneration() < t.generation { - newParent = updateGeneration(parent, t.generation, t.deletedKeys) - } - - if bytes.HasPrefix(newParent.GetKey(), prefix) { + if bytes.HasPrefix(parent.GetKey(), prefix) { return nil, true } - if newParent.Type() == node.LeafType { - // not modified so return the leaf of the original - // trie generation. The copied newParent will be - // garbage collected. + if parent.Type() == node.LeafType { return parent, false } - branch := newParent.(*node.Branch) + branch := parent.(*node.Branch) if len(prefix) == len(branch.Key)+1 && bytes.HasPrefix(branch.Key, prefix[:len(prefix)-1]) { @@ -850,13 +844,10 @@ func (t *Trie) clearPrefix(parent Node, prefix []byte) ( child := branch.Children[childIndex] if child == nil { - // child is already nil at the child index - // node is not modified so return the branch of the original - // trie generation. The copied newParent will be - // garbage collected. return parent, false } + branch = t.snapshotBranchOnOldGeneration(branch) branch.Children[childIndex] = nil branch.SetDirty(true) newParent = handleDeletion(branch, prefix) @@ -866,9 +857,6 @@ func (t *Trie) clearPrefix(parent Node, prefix []byte) ( noPrefixForNode := len(prefix) <= len(branch.Key) || lenCommonPrefix(branch.Key, prefix) < len(branch.Key) if noPrefixForNode { - // not modified so return the branch of the original - // trie generation. The copied newParent will be - // garbage collected. return parent, false } @@ -876,14 +864,13 @@ func (t *Trie) clearPrefix(parent Node, prefix []byte) ( childPrefix := prefix[len(branch.Key)+1:] child := branch.Children[childIndex] - branch.Children[childIndex], updated = t.clearPrefix(child, childPrefix) + child, updated = t.clearPrefix(child, childPrefix) if !updated { - // branch not modified so return the branch of the original - // trie generation. The copied newParent will be - // garbage collected. return parent, false } + branch = t.snapshotBranchOnOldGeneration(branch) + branch.Children[childIndex] = child branch.SetDirty(true) newParent = handleDeletion(branch, prefix) return newParent, true @@ -902,32 +889,15 @@ func (t *Trie) delete(parent Node, key []byte) (newParent Node, deleted bool) { return nil, false } - newParent = parent - if parent.GetGeneration() < t.generation { - newParent = updateGeneration(parent, t.generation, t.deletedKeys) - } - - if newParent.Type() == node.LeafType { - newParent = deleteLeaf(newParent, key) - if newParent == nil { + if parent.Type() == node.LeafType { + if deleteLeaf(parent, key) == nil { return nil, true } - // The leaf was not deleted so return the original - // parent without its generation updated. - // The copied newParent will be garbage collected. return parent, false } - branch := newParent.(*node.Branch) - newParent, deleted = t.deleteBranch(branch, key) - if !deleted { - // Nothing was deleted so return the original - // parent without its generation updated. - // The copied newParent will be garbage collected. - return parent, false - } - - return newParent, true + branch := parent.(*node.Branch) + return t.deleteBranch(branch, key) } func deleteLeaf(parent Node, key []byte) (newParent Node) { @@ -939,6 +909,7 @@ func deleteLeaf(parent Node, key []byte) (newParent Node) { func (t *Trie) deleteBranch(branch *node.Branch, key []byte) (newParent Node, deleted bool) { if len(key) == 0 || bytes.Equal(branch.Key, key) { + branch = t.snapshotBranchOnOldGeneration(branch) branch.Value = nil branch.SetDirty(true) return handleDeletion(branch, key), true @@ -954,6 +925,7 @@ func (t *Trie) deleteBranch(branch *node.Branch, key []byte) (newParent Node, de return branch, false } + branch = t.snapshotBranchOnOldGeneration(branch) branch.Children[childIndex] = newChild branch.SetDirty(true) newParent = handleDeletion(branch, key) diff --git a/lib/trie/trie_test.go b/lib/trie/trie_test.go index eaeb5b1ace..7ee8858c51 100644 --- a/lib/trie/trie_test.go +++ b/lib/trie/trie_test.go @@ -159,19 +159,6 @@ func Test_Trie_updateGeneration(t *testing.T) { } }) } - - t.Run("panic on same generation", func(t *testing.T) { - t.Parallel() - node := &node.Leaf{Generation: 1} - const trieGenration = 1 - assert.PanicsWithValue(t, - "current node has the same generation 1 as the trie generation, "+ - "make sure the caller properly checks for the node generation to "+ - "be smaller than the trie generation.", - func() { - updateGeneration(node, trieGenration, nil) - }) - }) } func getPointer(x interface{}) (pointer uintptr, ok bool) { @@ -1204,9 +1191,8 @@ func Test_Trie_insert(t *testing.T) { key: []byte{1}, value: []byte("same"), newNode: &node.Leaf{ - Key: []byte{1}, - Value: []byte("same"), - Generation: 1, + Key: []byte{1}, + Value: []byte("same"), }, }, "write leaf as child to parent leaf": { @@ -2518,6 +2504,26 @@ func Test_Trie_clearPrefixLimit(t *testing.T) { valuesDeleted: 2, allDeleted: true, }, + "delete child of branch with limit reached": { + trie: Trie{ + generation: 1, + }, + parent: &node.Branch{ + Key: []byte{1}, + Value: []byte{1}, + Children: [16]node.Node{ + &node.Leaf{Key: []byte{3}}, + }, + }, + prefix: []byte{1, 0}, + newParent: &node.Branch{ + Key: []byte{1}, + Value: []byte{1}, + Children: [16]node.Node{ + &node.Leaf{Key: []byte{3}}, + }, + }, + }, } for name, testCase := range testCases { From 28e8d514803502fc0161935a8036e515cbb07c8b Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Sun, 6 Mar 2022 20:28:13 +0000 Subject: [PATCH 2/4] Remove commented unneeded line --- lib/trie/trie.go | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/trie/trie.go b/lib/trie/trie.go index 723b0a53c8..08ad00c669 100644 --- a/lib/trie/trie.go +++ b/lib/trie/trie.go @@ -436,7 +436,6 @@ func (t *Trie) insertInBranch(parentBranch *node.Branch, key, value []byte) (new } } else { child = t.insert(child, remainingKey, value) - // child.SetDirty(true) } parentBranch.Children[childIndex] = child From 36653283cab4326c4e26f77fdedffd98067cacf5 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Mon, 7 Mar 2022 14:11:58 +0000 Subject: [PATCH 3/4] Remove unneeded generation set --- lib/trie/trie.go | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/trie/trie.go b/lib/trie/trie.go index 08ad00c669..3380ca1d6d 100644 --- a/lib/trie/trie.go +++ b/lib/trie/trie.go @@ -358,7 +358,6 @@ func (t *Trie) insertInLeaf(parentLeaf *node.Leaf, key, parentLeaf = t.snapshotLeafOnOldGeneration(parentLeaf) parentLeaf.Value = value - parentLeaf.Generation = t.generation parentLeaf.SetDirty(true) return parentLeaf } From 917e1c261127988f93724a396bad7e8c97d937b7 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Mon, 7 Mar 2022 15:24:03 +0000 Subject: [PATCH 4/4] SetDirty in snapshot helpers --- lib/trie/trie.go | 59 +++++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 33 deletions(-) diff --git a/lib/trie/trie.go b/lib/trie/trie.go index 3380ca1d6d..0b81cca93c 100644 --- a/lib/trie/trie.go +++ b/lib/trie/trie.go @@ -62,26 +62,30 @@ func (t *Trie) Snapshot() (newTrie *Trie) { } } -func (t *Trie) snapshotLeafOnOldGeneration(currentLeaf *node.Leaf) (newLeaf *node.Leaf) { +func (t *Trie) prepLeafForMutation(currentLeaf *node.Leaf) (newLeaf *node.Leaf) { if currentLeaf.Generation == t.generation { // no need to deep copy and update generation // of current leaf. - return currentLeaf + newLeaf = currentLeaf + } else { + newNode := updateGeneration(currentLeaf, t.generation, t.deletedKeys) + newLeaf = newNode.(*node.Leaf) } - - newNode := updateGeneration(currentLeaf, t.generation, t.deletedKeys) - return newNode.(*node.Leaf) + newLeaf.SetDirty(true) + return newLeaf } -func (t *Trie) snapshotBranchOnOldGeneration(currentBranch *node.Branch) (newBranch *node.Branch) { +func (t *Trie) prepBranchForMutation(currentBranch *node.Branch) (newBranch *node.Branch) { if currentBranch.Generation == t.generation { // no need to deep copy and update generation - // of current leaf. - return currentBranch + // of current branch. + newBranch = currentBranch + } else { + newNode := updateGeneration(currentBranch, t.generation, t.deletedKeys) + newBranch = newNode.(*node.Branch) } - - newNode := updateGeneration(currentBranch, t.generation, t.deletedKeys) - return newNode.(*node.Branch) + newBranch.SetDirty(true) + return newBranch } // updateGeneration is called when the currentNode is from @@ -356,9 +360,8 @@ func (t *Trie) insertInLeaf(parentLeaf *node.Leaf, key, return parentLeaf } - parentLeaf = t.snapshotLeafOnOldGeneration(parentLeaf) + parentLeaf = t.prepLeafForMutation(parentLeaf) parentLeaf.Value = value - parentLeaf.SetDirty(true) return parentLeaf } @@ -378,10 +381,9 @@ func (t *Trie) insertInLeaf(parentLeaf *node.Leaf, key, if len(key) < len(parentLeafKey) { // Move the current leaf parent as a child to the new branch. - parentLeaf = t.snapshotLeafOnOldGeneration(parentLeaf) + parentLeaf = t.prepLeafForMutation(parentLeaf) childIndex := parentLeafKey[commonPrefixLength] parentLeaf.Key = parentLeaf.Key[commonPrefixLength+1:] - parentLeaf.SetDirty(true) newBranchParent.Children[childIndex] = parentLeaf } @@ -393,10 +395,9 @@ func (t *Trie) insertInLeaf(parentLeaf *node.Leaf, key, newBranchParent.Value = parentLeaf.Value } else { // make the leaf a child of the new branch - parentLeaf = t.snapshotLeafOnOldGeneration(parentLeaf) + parentLeaf = t.prepLeafForMutation(parentLeaf) childIndex := parentLeafKey[commonPrefixLength] parentLeaf.Key = parentLeaf.Key[commonPrefixLength+1:] - parentLeaf.SetDirty(true) newBranchParent.Children[childIndex] = parentLeaf } childIndex := key[commonPrefixLength] @@ -411,8 +412,7 @@ func (t *Trie) insertInLeaf(parentLeaf *node.Leaf, key, } func (t *Trie) insertInBranch(parentBranch *node.Branch, key, value []byte) (newParent Node) { - parentBranch = t.snapshotBranchOnOldGeneration(parentBranch) - parentBranch.SetDirty(true) + parentBranch = t.prepBranchForMutation(parentBranch) if bytes.Equal(key, parentBranch.Key) { parentBranch.Value = value @@ -715,9 +715,8 @@ func (t *Trie) clearPrefixLimitBranch(branch *node.Branch, prefix []byte, limit return branch, valuesDeleted, allDeleted } - branch = t.snapshotBranchOnOldGeneration(branch) + branch = t.prepBranchForMutation(branch) branch.Children[childIndex] = child - branch.SetDirty(true) newParent = handleDeletion(branch, prefix) return newParent, valuesDeleted, allDeleted } @@ -742,9 +741,8 @@ func (t *Trie) clearPrefixLimitChild(branch *node.Branch, prefix []byte, limit u return branch, valuesDeleted, allDeleted } - branch = t.snapshotBranchOnOldGeneration(branch) + branch = t.prepBranchForMutation(branch) branch.Children[childIndex] = child - branch.SetDirty(true) newParent = handleDeletion(branch, prefix) allDeleted = branch.Children[childIndex] == nil @@ -778,7 +776,7 @@ func (t *Trie) deleteNodesLimit(parent Node, prefix []byte, limit uint32) ( continue } - branch = t.snapshotBranchOnOldGeneration(branch) + branch = t.prepBranchForMutation(branch) branch.Children[i], newDeleted = t.deleteNodesLimit(child, fullKey, limit) if branch.Children[i] == nil { nilChildren++ @@ -786,7 +784,6 @@ func (t *Trie) deleteNodesLimit(parent Node, prefix []byte, limit uint32) ( limit -= newDeleted valuesDeleted += newDeleted - branch.SetDirty(true) newParent = handleDeletion(branch, fullKey) if nilChildren == node.ChildrenCapacity && branch.Value == nil { @@ -845,9 +842,8 @@ func (t *Trie) clearPrefix(parent Node, prefix []byte) ( return parent, false } - branch = t.snapshotBranchOnOldGeneration(branch) + branch = t.prepBranchForMutation(branch) branch.Children[childIndex] = nil - branch.SetDirty(true) newParent = handleDeletion(branch, prefix) return newParent, true } @@ -867,9 +863,8 @@ func (t *Trie) clearPrefix(parent Node, prefix []byte) ( return parent, false } - branch = t.snapshotBranchOnOldGeneration(branch) + branch = t.prepBranchForMutation(branch) branch.Children[childIndex] = child - branch.SetDirty(true) newParent = handleDeletion(branch, prefix) return newParent, true } @@ -907,9 +902,8 @@ func deleteLeaf(parent Node, key []byte) (newParent Node) { func (t *Trie) deleteBranch(branch *node.Branch, key []byte) (newParent Node, deleted bool) { if len(key) == 0 || bytes.Equal(branch.Key, key) { - branch = t.snapshotBranchOnOldGeneration(branch) + branch = t.prepBranchForMutation(branch) branch.Value = nil - branch.SetDirty(true) return handleDeletion(branch, key), true } @@ -923,9 +917,8 @@ func (t *Trie) deleteBranch(branch *node.Branch, key []byte) (newParent Node, de return branch, false } - branch = t.snapshotBranchOnOldGeneration(branch) + branch = t.prepBranchForMutation(branch) branch.Children[childIndex] = newChild - branch.SetDirty(true) newParent = handleDeletion(branch, key) return newParent, true }