feat: refactor the node key as `version + local nonce(seq id)` #676

cool-develope · 2023-02-02T21:15:32Z

No description provided.

kocubinski · 2023-02-24T22:59:44Z

keyformat/key_format.go

@@ -105,6 +106,13 @@ func (kf *KeyFormat) Key(args ...interface{}) []byte {
 	return kf.KeyBytes(segments...)
 }

+func (kf *KeyFormat) NodeKey(nodeKey int64) []byte {


I can't find usages of this function anywhere, am I missing something?

catShaark · 2023-03-03T07:18:16Z

This maybe out of scope for this PR but I think we should change the key format for leaf nodes to be version | l | key where key is the value's key. Doing so, we can fetch key/value fair from iavl.Store and iterate iavl.Store without tree traversal.

ValarDragon · 2023-03-03T12:04:20Z

This maybe out of scope for this PR but I think we should change the key format for leaf nodes to be version | l | key where key is the value's key. Doing so, we can fetch key/value fair from iavl.Store and iterate iavl.Store without tree traversal.

Shouldn't hardware prefetching mostly eliminate the tree traversal as a time overhead?

yihuang · 2023-03-03T14:31:05Z

This maybe out of scope for this PR but I think we should change the key format for leaf nodes to be version | l | key where key is the value's key. Doing so, we can fetch key/value fair from iavl.Store and iterate iavl.Store without tree traversal.

Sounds like a very clever idea, but this may not serve what you want because you don't know what exact version the key is modified, unless also maintain some other indexes.

catShaark · 2023-03-03T15:02:29Z

This maybe out of scope for this PR but I think we should change the key format for leaf nodes to be version | l | key where key is the value's key. Doing so, we can fetch key/value fair from iavl.Store and iterate iavl.Store without tree traversal.

Sounds like a very clever idea, but this may not serve what you want because you don't know what exact version the key is modified, unless also maintain some other indexes.

But we always know the version since when we load iavl.Store, we always specify the version

yihuang · 2023-03-03T15:17:40Z

This maybe out of scope for this PR but I think we should change the key format for leaf nodes to be version | l | key where key is the value's key. Doing so, we can fetch key/value fair from iavl.Store and iterate iavl.Store without tree traversal.

Shouldn't hardware prefetching mostly eliminate the tree traversal as a time overhead?

Tree nodes are scattered at different versions, prefetching don't help much I think.

yihuang · 2023-03-03T15:47:07Z

This maybe out of scope for this PR but I think we should change the key format for leaf nodes to be version | l | key where key is the value's key. Doing so, we can fetch key/value fair from iavl.Store and iterate iavl.Store without tree traversal.

Sounds like a very clever idea, but this may not serve what you want because you don't know what exact version the key is modified, unless also maintain some other indexes.

But we always know the version since when we load iavl.Store, we always specify the version

That is the version you want to query in, not the version the key is last modified, the version contained in the node is the version the node is created in, aka. when the key is modified or inserted.

cool-develope · 2023-03-03T15:50:29Z

This maybe out of scope for this PR but I think we should change the key format for leaf nodes to be version | l | key where key is the value's key. Doing so, we can fetch key/value fair from iavl.Store and iterate iavl.Store without tree traversal.

Sounds like a very clever idea, but this may not serve what you want because you don't know what exact version the key is modified, unless also maintain some other indexes.

But we always know the version since when we load iavl.Store, we always specify the version

@yihuang is right, we have different versions, how is key-iteration query possible? since the version prefix is different

Makefile

node.go

kocubinski

Logically things look OK to me. Maybe it's a big nit, but I wonder if this diff and implementation would be cleaner and more readable without the NodeKey struct and the key just represented as []byte (same as it is now as hash).

To that I end I did a little refactoring PoC to see what it looks like. Let me know what you think, since you've spent time on this problem here than me.

https://github.com/cosmos/iavl/compare/592/refactor-nonce-new...kocubinski/592?expand=1

node.go

kocubinski · 2023-03-07T18:17:43Z

node.go

 	}
 }

 // MakeNode constructs an *Node from an encoded byte slice.
 //
 // The new node doesn't have its hash saved or set. The caller must set it
 // afterwards.
-func MakeNode(buf []byte) (*Node, error) {
-	// Read node header (height, size, version, key).
+func MakeNode(nodeKey *NodeKey, buf []byte) (*Node, error) {


Do we really want/need an API breaking change here?

I think this API is not exposed

It is exported, see: https://go.dev/tour/basics/3

absolutely, it is exported. I believe there is no usecase of this api outside.

node.go

kocubinski · 2023-03-07T18:42:05Z

proof.go

 	if node.subtreeHeight == 0 {
 		if bytes.Equal(node.key, key) {
 			return node, nil
 		}
 		return node, errors.New("key does not exist")
 	}

+	nodeVersion := version


Why don't we just use the field version on the Node? There is no valid case where a node does not have a db key right?

it is related to new design, we have new added nodes while uncommitting stage, and these nodes will have no nodekey and version, in that case, we should indicate the current version for those nodes.

kocubinski · 2023-03-07T18:49:00Z

docs/node/node.md


 The version of a node is the first version of the IAVL tree that the node gets added in. Future versions of the IAVL may point to this node if they also contain the node, however the node's version itself does not change.

 Size is the number of leaves under a given node. With a full subtree, `node.size = 2^(node.height)`.

 ### Marshaling 

-Every node is persisted by encoding the key, version, height, size and hash. If the node is a leaf node, then the value is persisted as well. If the node is not a leaf node, then the leftHash and rightHash are persisted as well.
+Every node is persisted by encoding the key, height, and size. If the node is a leaf node, then the value is persisted as well. If the node is not a leaf node, then the hash, leftNodeKey and rightNodeKey are persisted as well.


I think there should be some explanation of why hash is now written for inner nodes since this is a key departure from the previous design.

kocubinski · 2023-03-07T19:40:06Z

node.go

 		}
-		cause = encoding.EncodeBytes(w, node.leftHash)
+		cause = encoding.EncodeVarint(w, int64(node.leftNodeKey.nonce))


If we're going to cast this to int64 anyway on write why not type it as int64?

it only cares about how much is the number, not about the integer variable type in EncodeVarint.
the result is same for 2(int64) and 2(int32), we will reduce the memory usage with int32 nonce

cool-develope · 2023-03-08T14:56:10Z

Here is the rocksDB benchmarking result, there is not much improvement in RocksDB

iavl: 0.20.0-alpha1-23-gde1532f
goos: linux
goarch: amd64
pkg: github.com/cosmos/iavl/benchmarks
cpu: DO-Premium-Intel
                                                                     │   old.txt          │   new.txt    │    
                                                                     │    sec/op          │    sec/op    │ 
Larger/memdb-1000000-100-16-40/query-no-in-tree-guarantee-fast-4       3.818µ ±  3%         4.010µ ±  3% 
Larger/memdb-1000000-100-16-40/query-no-in-tree-guarantee-slow-4       20.66µ ±  6%         18.34µ ±  5% 
Larger/memdb-1000000-100-16-40/query-hits-fast-4                       5.650µ ±  7%         5.913µ ±  4% 
Larger/memdb-1000000-100-16-40/query-hits-slow-4                       25.17µ ±  8%         22.22µ ±  2% 
Larger/memdb-1000000-100-16-40/iteration-fast-4                        592.3m ±  3%         553.8m ±  5% 
Larger/memdb-1000000-100-16-40/iteration-slow-4                         12.78 ±  4%          6.023 ±  2%
Larger/memdb-1000000-100-16-40/update-4                                317.9µ ±  5%         212.5µ ± 10%
Larger/memdb-1000000-100-16-40/block-4                                 34.30m ±  6%         24.11m ±  3%
Larger/goleveldb-1000000-100-16-40/query-no-in-tree-guarantee-fast-4   4.123µ ±  7%         4.169µ ±  2%
Larger/goleveldb-1000000-100-16-40/query-no-in-tree-guarantee-slow-4   32.94µ ± 12%         26.92µ ±  6%
Larger/goleveldb-1000000-100-16-40/query-hits-fast-4                   12.36µ ±  3%         12.22µ ±  4%
Larger/goleveldb-1000000-100-16-40/query-hits-slow-4                   41.00µ ±  5%         33.70µ ±  4%
Larger/goleveldb-1000000-100-16-40/iteration-fast-4                    780.4m ±  3%         819.9m ± 10%
Larger/goleveldb-1000000-100-16-40/iteration-slow-4                     30.43 ±  3%          13.92 ±  2%
Larger/goleveldb-1000000-100-16-40/update-4                            373.6µ ±  9%         276.1µ ± 10%
Larger/goleveldb-1000000-100-16-40/block-4                             50.60m ±  3%         35.58m ±  5%
Larger/rocksdb-1000000-100-16-40/query-no-in-tree-guarantee-fast-4     6.655µ ±  6%         6.695µ ±  8%
Larger/rocksdb-1000000-100-16-40/query-no-in-tree-guarantee-slow-4     22.06µ ±  9%         25.07µ ±  7%
Larger/rocksdb-1000000-100-16-40/query-hits-fast-4                     8.097µ ±  4%         8.287µ ±  3%
Larger/rocksdb-1000000-100-16-40/query-hits-slow-4                     29.97µ ±  4%         28.99µ ±  4%
Larger/rocksdb-1000000-100-16-40/iteration-fast-4                       4.352 ±  2%          4.359 ±  3%
Larger/rocksdb-1000000-100-16-40/iteration-slow-4                       16.81 ±  2%          15.28 ±  2%
Larger/rocksdb-1000000-100-16-40/update-4                              436.1µ ± 10%         281.0µ ± 10%
Larger/rocksdb-1000000-100-16-40/block-4                               45.70m ±  6%         32.38m ±  4%
geomean                                                                1.356m               1.135m

ValarDragon · 2023-03-13T14:51:20Z

🎉

Since cosmos#676, nodes are indexed by an integer nonce instead of its hash

cool-develope and others added 30 commits October 19, 2022 08:58

feat: add nodeKey integer field (#593)

7204160

replace the node key

f3168eb

fix lint

35c73c5

feat: remove nodeKey from node db writes (#602)

d6347ef

small fix

36a150e

optimize key format

042b877

wip

970635e

wip

39edd63

wip

1f91cae

fix lint issues

e43fa1b

feat: refactor subtask 2 (#626)

43dcaff

remove getOrphans

61bf3c6

Merge branch '592/refactor-subtask-2' into 592/subtask-2

b7d631b

fix memory leak and nonce

6d95cfb

reflect comments

dcc0d07

fix save node

03baaf5

wip

9427e3a

remove orphans

4fa9b76

update CHANGELOG

cefd10d

Merge branch 'master' into 592/remove_orphans

a2e2b18

fix conflicts

d460ee6

fix lint issues

946c32d

fix merge conflicts

1e72e89

wip

2ab3799

linting

cfd2e15

fix conflicts

9fca5d7

small fix

8fb9bbc

test failure fix

9c40e85

remove orphans

12caa26

update CHANGELOG

57f3a5f

cool-develope and others added 2 commits February 24, 2023 11:37

update the docs

78653f2

Merge branch 'master' into 592/refactor-nonce-new

f1257c2

kocubinski reviewed Feb 24, 2023

View reviewed changes

cool-develope and others added 2 commits February 27, 2023 07:04

Merge branch 'master' into 592/refactor-nonce-new

6254855

prune unnecessary one

221b84a

alexanderbez self-requested a review March 2, 2023 14:59

Merge branch 'master' into 592/refactor-nonce-new

4b1cf11

alexanderbez approved these changes Mar 5, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

kocubinski reviewed Mar 7, 2023

View reviewed changes

node.go Show resolved Hide resolved

kocubinski reviewed Mar 7, 2023

View reviewed changes

cool-develope and others added 2 commits March 8, 2023 04:20

Merge branch 'master' into 592/refactor-nonce-new

7d8da49

comments

cbcd5e7

cool-develope requested a review from kocubinski March 8, 2023 14:41

kocubinski approved these changes Mar 8, 2023

View reviewed changes

tac0turtle merged commit e46665c into master Mar 13, 2023

tac0turtle deleted the 592/refactor-nonce-new branch March 13, 2023 11:52

cool-develope mentioned this pull request Apr 10, 2023

[Epic]: migration of the new store module cosmos/cosmos-sdk#15769

Closed

larry0x added a commit to larry0x/iavl that referenced this pull request May 11, 2023

fix outdated docs

532283f

Since cosmos#676, nodes are indexed by an integer nonce instead of its hash

kocubinski mentioned this pull request May 15, 2023

feat: implement the lazy_set feature #750

Merged

drklee3 mentioned this pull request May 30, 2024

Root hash breaking change in v1.x.x #956

Closed

cool-develope mentioned this pull request Oct 4, 2024

fix: working hash with initial version #943

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor the node key as `version + local nonce(seq id)` #676

feat: refactor the node key as `version + local nonce(seq id)` #676

cool-develope commented Feb 2, 2023

kocubinski Feb 24, 2023

cool-develope Feb 27, 2023

catShaark commented Mar 3, 2023 •

edited

Loading

ValarDragon commented Mar 3, 2023 •

edited

Loading

yihuang commented Mar 3, 2023 •

edited

Loading

catShaark commented Mar 3, 2023

yihuang commented Mar 3, 2023

yihuang commented Mar 3, 2023

cool-develope commented Mar 3, 2023

kocubinski left a comment

kocubinski Mar 7, 2023

cool-develope Mar 8, 2023

kocubinski Mar 8, 2023

cool-develope Mar 8, 2023

kocubinski Mar 7, 2023

cool-develope Mar 8, 2023 •

edited

Loading

kocubinski Mar 7, 2023

kocubinski Mar 7, 2023

cool-develope Mar 8, 2023

cool-develope commented Mar 8, 2023 •

edited

Loading

ValarDragon commented Mar 13, 2023

feat: refactor the node key as version + local nonce(seq id) #676

feat: refactor the node key as version + local nonce(seq id) #676

Conversation

cool-develope commented Feb 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catShaark commented Mar 3, 2023 • edited Loading

ValarDragon commented Mar 3, 2023 • edited Loading

yihuang commented Mar 3, 2023 • edited Loading

catShaark commented Mar 3, 2023

yihuang commented Mar 3, 2023

yihuang commented Mar 3, 2023

cool-develope commented Mar 3, 2023

kocubinski left a 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

cool-develope Mar 8, 2023 • edited Loading

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

cool-develope commented Mar 8, 2023 • edited Loading

ValarDragon commented Mar 13, 2023

feat: refactor the node key as `version + local nonce(seq id)` #676

feat: refactor the node key as `version + local nonce(seq id)` #676

catShaark commented Mar 3, 2023 •

edited

Loading

ValarDragon commented Mar 3, 2023 •

edited

Loading

yihuang commented Mar 3, 2023 •

edited

Loading

cool-develope Mar 8, 2023 •

edited

Loading

cool-develope commented Mar 8, 2023 •

edited

Loading