-
Notifications
You must be signed in to change notification settings - Fork 464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Perf/HalfPath state db key #6331
Conversation
@asdacap Does this change requires resync of the DB? |
It require resync to take effect. But we can add a fallback read, so it can be made to work without resync. However, we are effectively doing two read at each node which is not great. I guess the first read would get bloom filtered, but still not great. |
1aca153
to
9ff1fbc
Compare
af2cee0
to
5457d76
Compare
18ffa26
to
e769795
Compare
2fdb41c
to
f183e2b
Compare
@@ -237,26 +236,8 @@ void AddAgainAllItems() | |||
NodeDataType nodeDataType = currentStateSyncItem.NodeDataType; | |||
if (nodeDataType == NodeDataType.Code) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complication was removed. State item always need path now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have worked with code base and made changes, so would put more weight on other reviewers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing a lot of Hash256
, would it be better to move to ValueHash256
in those places?
Most my comments are just nitpicks.
Co-authored-by: Lukasz Rozmej <[email protected]>
So, two place where Hash256 is kept:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few initial comments, for better understanding of the decisions taken here.
ReadFlags flags = visitor.ExtraReadFlag; | ||
if (visitor.IsFullDbScan) | ||
{ | ||
if (TrieStore.Scheme == INodeStorage.KeyScheme.HalfPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this code elsewhere. It's dependent on the TrieStore.
Schemeso maybe TrieStore could provide
FullDbScanFlags`?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. NodeStorage already change the readahead flag already.
Co-authored-by: Lukasz Rozmej <[email protected]>
Co-authored-by: Lukasz Rozmej <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor and some comments.
Prefix state db entry with part of path
--Init.StateDbKeyScheme Hash
can be specified. An explicit--Init.StateDbKeyScheme HalfPath
is needed for full pruning to migrate to the new key layout.The keys are separated with section byte due to the different characteristics of these nodes. The idea being that top level
node can be up to 5 times bigger than lower node, and grew a lot due to pruning. So mixing them makes lower
node sparser and have poorer cache hit, and make traversing leaves for snap serving slower.
Performance
Sync performance
Migration.
--Init.StateDbKeyScheme HalfPath
and then run full pruning to migrate.Database growth
18310000
from 9 Oct, and it was stopped on block18400000
(21 Oct).`Full pruning
Changes
TreePath
for representing path. Aref TreePath
parameters is added everywhere in PatriciaTree and TrieNode. On traversal to child, the tree path is mutated to add necessary key to become the correct path for its child and it is mutated back to its parent after returning from traversing its child. Its messy, but it significantly reduce memory allocation overhead.PatriciaTree
and friends, accept aIScopedTreeStore
which is a subinterface ofINodeResolver
. The implementation,ScopedTreeStore
keep the hash of the storage, which is then proxied toITreeStore
that we've known and love.ITreeStore
has been modified to also accept the storage and path in addition to the standard hash.TreeStore
has been modified to be keyed byHash256?, TreePath, Keccak
tuple instead of justKeccak
.INodeStorage
replacesIKeyValueStore
as the storage layer of the nodes, accepting the storage root and the path also. This is the class the determine the key and allow fallback read as well as configuring between hash and halfpath.INodeStorage
instead ofIKeyValueStore
.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Documentation
Requires documentation update
Requires explanation in Release Notes
New state db key structure improve block processing time by up to 50%. Existing users are unaffected.