-
Notifications
You must be signed in to change notification settings - Fork 325
feat: add adr-001 for node key refactoring #608
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
Changes from 2 commits
4f9a004
8646a9a
6e5b081
4ee7804
9468ee2
0c2d610
1459a18
d8d0bf9
88885f1
5272de4
006d76c
7cc7280
4e6044f
0bf7486
a0dcc0e
ee11dff
d9f7d2e
10184ac
5f94844
85a90e7
8fb87b6
6bbf7f9
204d881
f743311
bf85d92
b064ede
6095bc4
7873267
fb0f182
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| # ADR ADR-001: Node Key Refactoring | ||
|
|
||
| ## Changelog | ||
|
|
||
| - 2022-10-31: First draft | ||
|
|
||
| ## Status | ||
|
|
||
| Proposed | ||
|
|
||
| ## Context | ||
|
|
||
| The original node key of IAVL is a hash of the node and it does not take advantage of data locality on disk. The nodes are stored in a random location of the disk due to the random hash value, so it needs to do a random search of the disk to find the node. | ||
|
|
||
| The `orphans` are used to manage the removed nodes in the current version and allow to deletion of the removed nodes for the specific version from the disk through the `DeleteVersion`. It needs to track every time when updating the tree and also requires extra storage to store `orphans`, but there are not many use cases of `DeleteVersion`. There are two use cases, the first one is the rollback of the tree and the second one is to remove the unnecessary old nodes. | ||
|
cool-develope marked this conversation as resolved.
Outdated
|
||
|
|
||
| ## Decision | ||
|
|
||
| - Use the sequenced integer ID as a node key like `bigendian(nodeKey)` format. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is nodeKey computed, I remember it's sth like "version+path"?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, it is a just sequenced integer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems better to use "version/seq", where seq is only unique inside the version.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "version/seq" is globally unique as well, seq itself is locally unique within the version. For each version, the nodes are written in a batch anyway, we only need to maintain the seq in memory.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why do we need to update the left and right node keys?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Meaning that every time we have a new version we update all referenced node keys to refer to that version rather than the original version where they were created? That seems really complex. I feel like I'm maybe not understanding something really basic here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yihuang , I agree with you. If we keep the local nonce as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think the size difference should be minimal, assuming version is
|
||
| - Remove the `version` field from the node structure. | ||
| - Remove the `orphans` from the tree. | ||
|
|
||
| New node structure | ||
|
tac0turtle marked this conversation as resolved.
|
||
|
|
||
| ```go | ||
| type Node struct { | ||
| key []byte | ||
| value []byte | ||
| hash []byte | ||
|
cool-develope marked this conversation as resolved.
Outdated
|
||
| leftHash []byte | ||
| rightHash []byte | ||
| nodeKey int64 // new field, use as a key | ||
| leftNodeKey int64 // new field, need to store | ||
| rightNodeKey int64 // new field, need to store | ||
| version int64 // will remove | ||
| size int64 | ||
| leftNode *Node | ||
| rightNode *Node | ||
| subtreeHeight int8 | ||
| persisted bool | ||
| } | ||
| ``` | ||
|
|
||
| New tree structure | ||
|
|
||
| ```go | ||
| type MutableTree struct { | ||
| *ImmutableTree | ||
| lastSaved *ImmutableTree | ||
| nonce int64 // new field to track the current ID | ||
| orphans map[int64]int64 // will remove | ||
| versions map[int64]bool | ||
| allRootLoaded bool | ||
| unsavedFastNodeAdditions map[string]*fastnode.Node | ||
| unsavedFastNodeRemovals map[string]interface{} | ||
| ndb *nodeDB | ||
| skipFastStorageUpgrade bool | ||
|
|
||
| mtx sync.Mutex | ||
| } | ||
| ``` | ||
|
|
||
| ## Consequences | ||
|
cool-develope marked this conversation as resolved.
cool-develope marked this conversation as resolved.
Outdated
|
||
|
|
||
| ### Positive | ||
|
|
||
| Using the sequenced integer ID, we take advantage of data locality in the bTree and it leads to performance improvements. | ||
|
|
||
| Removing orphans also provides performance improvements including memory and storage saving. Also, it makes it easy to rollback the tree. Because we will keep the sequenced segment IDs for the specific version, and we can remove all nodes for which the `nodeKey` is greater than the specified integer value. | ||
|
|
||
| ### Negative | ||
|
|
||
| It requires extra storage to store the node because it should keep `leftNodeKey` and `rightNodeKey` to iterate the tree. Instead, we can delete the `version` in the node and reduce the key size. | ||
|
|
||
| It can't delete the old nodes for the specific version due to removing orphans. But it makes `rollback` easier and it makes it possible to remove old nodes through `import` and `export` functionalities. The `export` will restruct the tree to make node IDs to a sequenced segment like (1 ... node_sieze). | ||
|
cool-develope marked this conversation as resolved.
Outdated
|
||
|
|
||
| ## References | ||
|
|
||
| - https://github.com/cosmos/iavl/issues/548 | ||
| - https://github.com/cosmos/iavl/issues/137 | ||
| - https://github.com/cosmos/iavl/issues/571 | ||
Uh oh!
There was an error while loading. Please reload this page.