trie: optimize memory allocation #30932
Conversation
|
Before I dive into the nitty gritty; can you say anything about what's done here? |
|
Some preliminary benchmark results:
|
|
|
CI is failing Investigation needed |
rjl493456442
force-pushed
the
mpt-nocopy
branch
from
c9b1b6c to
04eb383
Compare
| // do that, since we don't overwrite/reuse keys | ||
| // cached.Key = common.CopyBytes(n.Key) | ||
| func (h *hasher) hashShortNodeChildren(n *shortNode) *shortNode { | ||
| var collapsed shortNode |
There was a problem hiding this comment.
this is essentially still a copy. You don't invoke copy(), but you create a new one and set it's fields. Was it not possible to do an in-place version here?
Just curious
There was a problem hiding this comment.
Yes, it's still a copy.
It's not possible to do an in-place modify. If so, the child will be replaced by its hash and the entire trie will be collapsed into a single hash. It's suitable for commit operation, but not for hash operation. The trie being hashed is still available for following usage.
rjl493456442
force-pushed
the
mpt-nocopy
branch
from
e03fd3d to
9156cec
Compare
rjl493456442
force-pushed
the
mpt-nocopy
branch
from
9156cec to
25ae4e5
Compare
| case hashNode: | ||
| return n | ||
| default: | ||
| panic(fmt.Sprintf("%T: unknown node type", n)) |
There was a problem hiding this comment.
| panic(fmt.Sprintf("%T: unknown node type", n)) | |
| panic(fmt.Sprintf("%T: unknown node type: %v", n, n)) |
| for i := 0; i < 10; i++ { | ||
| tr.Update(testrand.Bytes(32), testrand.Bytes(32)) | ||
| } | ||
|
|
There was a problem hiding this comment.
| tr.Hash() | |
| tr.Commit(false) |
There was a problem hiding this comment.
is here missing the Commit?
|
I run a benchmark with the master branch code about the |
| @@ -1311,3 +1311,171 @@ func printSet(set *trienode.NodeSet) string { | |||
| } | |||
| return out.String() | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
| func BenchmarkTestTrieCopy(b *testing.B) { | |
| b.ResetTimer() | |
| tr := NewEmpty(nil) | |
| for i := 0; i < 256; i++ { | |
| tr.Update(testrand.Bytes(32), testrand.Bytes(32)) | |
| } | |
| for i := 0; i < b.N; i++ { | |
| copiedTrie := tr.Copy() | |
| tr.Update(testrand.Bytes(32), testrand.Bytes(32)) | |
| _ = copiedTrie | |
| } | |
| } |
This pull request removes the node copy operation to reduce memory allocation. Key Changes as below:
(a) Use
decodeNodeUnsafefor decoding nodes retrieved from the trie node readerIn the current implementation of the MPT, once a trie node blob is retrieved, it is passed to
decodeNodefor decoding. However,decodeNodeassumes the supplied byte slice might be mutated later, so it performs a deep copy internally before parsing the node.Given that the node reader is implemented by the path database and the hash database, both of which guarantee the immutability of the returned byte slice. By restricting the node reader interface to explicitly guarantee that the returned byte slice will not be modified, we can safely replace
decodeNodewithdecodeNodeUnsafe. This eliminates the need for a redundant byte copy during each node resolution.(b) Modify the trie in place
In the current implementation of the MPT, a copy of a trie node is created before any modifications are made. These modifications include:
This mechanism ensures that modifications only affect the live tree, leaving all previously created copies unaffected.
Unfortunately, this property leads to a huge memory allocation requirement. For example, if we want to modify the fullNode for n times, the node will be copied for n times.
In this pull request, all the trie modifications are made in place. In order to make sure all previously created copies are unaffected, the
Copyfunction now will deep-copy all the live nodes rather than the root node itself.With this change, while the
Copyfunction becomes more expensive, it's totally acceptable as it's not a frequently used one. For the normal trie operations (Get, GetNode, Hash, Commit, Insert, Delete), the node copy is not required anymore.