From 1874a29601dae352c65d8bc7c859e56d112abd51 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 11:11:43 +0800 Subject: [PATCH 01/19] Optimize iteration on nested cache context Closes: #10310 Solution: - cache the valid status --- store/cachekv/benchmark_test.go | 162 ++++++++++++++++++++++++++++++++ store/cachekv/mergeiterator.go | 46 +++++---- 2 files changed, 184 insertions(+), 24 deletions(-) create mode 100644 store/cachekv/benchmark_test.go diff --git a/store/cachekv/benchmark_test.go b/store/cachekv/benchmark_test.go new file mode 100644 index 000000000000..66873f8e047b --- /dev/null +++ b/store/cachekv/benchmark_test.go @@ -0,0 +1,162 @@ +package cachekv_test + +import ( + fmt "fmt" + "testing" + + "github.com/cosmos/cosmos-sdk/store" + storetypes "github.com/cosmos/cosmos-sdk/store/types" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/tendermint/tendermint/libs/log" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + dbm "github.com/tendermint/tm-db" +) + +func DoBenchmarkDeepContextStack(b *testing.B, depth int) { + begin := []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} + end := []byte{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff} + key := storetypes.NewKVStoreKey("test") + + db := dbm.NewMemDB() + cms := store.NewCommitMultiStore(db) + cms.MountStoreWithDB(key, storetypes.StoreTypeIAVL, db) + cms.LoadLatestVersion() + ctx := sdk.NewContext(cms, tmproto.Header{}, false, log.NewNopLogger()) + + var stack ContextStack + stack.Reset(ctx) + + for i := 0; i < depth; i++ { + stack.Snapshot() + + store := stack.CurrentContext().KVStore(key) + store.Set(begin, []byte("value")) + } + + store := stack.CurrentContext().KVStore(key) + for i := 0; i < b.N; i++ { + it := store.Iterator(begin, end) + it.Valid() + it.Key() + it.Value() + it.Next() + it.Close() + } +} + +func BenchmarkDeepContextStack1(b *testing.B) { + DoBenchmarkDeepContextStack(b, 1) +} + +func BenchmarkDeepContextStack3(b *testing.B) { + DoBenchmarkDeepContextStack(b, 3) +} +func BenchmarkDeepContextStack10(b *testing.B) { + DoBenchmarkDeepContextStack(b, 10) +} + +func BenchmarkDeepContextStack13(b *testing.B) { + DoBenchmarkDeepContextStack(b, 13) +} + +// cachedContext is a pair of cache context and its corresponding commit method. +// They are obtained from the return value of `context.CacheContext()`. +type cachedContext struct { + ctx sdk.Context + commit func() +} + +// ContextStack manages the initial context and a stack of cached contexts, +// to support the `StateDB.Snapshot` and `StateDB.RevertToSnapshot` methods. +type ContextStack struct { + // Context of the initial state before transaction execution. + // It's the context used by `StateDB.CommitedState`. + initialCtx sdk.Context + cachedContexts []cachedContext +} + +// CurrentContext returns the top context of cached stack, +// if the stack is empty, returns the initial context. +func (cs *ContextStack) CurrentContext() sdk.Context { + l := len(cs.cachedContexts) + if l == 0 { + return cs.initialCtx + } + return cs.cachedContexts[l-1].ctx +} + +// Reset sets the initial context and clear the cache context stack. +func (cs *ContextStack) Reset(ctx sdk.Context) { + cs.initialCtx = ctx + if len(cs.cachedContexts) > 0 { + cs.cachedContexts = []cachedContext{} + } +} + +// IsEmpty returns true if the cache context stack is empty. +func (cs *ContextStack) IsEmpty() bool { + return len(cs.cachedContexts) == 0 +} + +// Commit commits all the cached contexts from top to bottom in order and clears the stack by setting an empty slice of cache contexts. +func (cs *ContextStack) Commit() { + // commit in order from top to bottom + for i := len(cs.cachedContexts) - 1; i >= 0; i-- { + // keep all the cosmos events + cs.initialCtx.EventManager().EmitEvents(cs.cachedContexts[i].ctx.EventManager().Events()) + if cs.cachedContexts[i].commit == nil { + panic(fmt.Sprintf("commit function at index %d should not be nil", i)) + } else { + cs.cachedContexts[i].commit() + } + } + cs.cachedContexts = []cachedContext{} +} + +// CommitToRevision commit the cache after the target revision, +// to improve efficiency of db operations. +func (cs *ContextStack) CommitToRevision(target int) error { + if target < 0 || target >= len(cs.cachedContexts) { + return fmt.Errorf("snapshot index %d out of bound [%d..%d)", target, 0, len(cs.cachedContexts)) + } + + targetCtx := cs.cachedContexts[target].ctx + // commit in order from top to bottom + for i := len(cs.cachedContexts) - 1; i > target; i-- { + // keep all the cosmos events + targetCtx.EventManager().EmitEvents(cs.cachedContexts[i].ctx.EventManager().Events()) + if cs.cachedContexts[i].commit == nil { + return fmt.Errorf("commit function at index %d should not be nil", i) + } + cs.cachedContexts[i].commit() + } + cs.cachedContexts = cs.cachedContexts[0 : target+1] + + return nil +} + +// Snapshot pushes a new cached context to the stack, +// and returns the index of it. +func (cs *ContextStack) Snapshot() int { + i := len(cs.cachedContexts) + ctx, commit := cs.CurrentContext().CacheContext() + cs.cachedContexts = append(cs.cachedContexts, cachedContext{ctx: ctx, commit: commit}) + return i +} + +// RevertToSnapshot pops all the cached contexts after the target index (inclusive). +// the target should be snapshot index returned by `Snapshot`. +// This function panics if the index is out of bounds. +func (cs *ContextStack) RevertToSnapshot(target int) { + if target < 0 || target >= len(cs.cachedContexts) { + panic(fmt.Errorf("snapshot index %d out of bound [%d..%d)", target, 0, len(cs.cachedContexts))) + } + cs.cachedContexts = cs.cachedContexts[:target] +} + +// RevertAll discards all the cache contexts. +func (cs *ContextStack) RevertAll() { + if len(cs.cachedContexts) > 0 { + cs.RevertToSnapshot(0) + } +} diff --git a/store/cachekv/mergeiterator.go b/store/cachekv/mergeiterator.go index a6c7a035aba0..59bb10190ae9 100644 --- a/store/cachekv/mergeiterator.go +++ b/store/cachekv/mergeiterator.go @@ -18,6 +18,8 @@ type cacheMergeIterator struct { parent types.Iterator cache types.Iterator ascending bool + + valid bool } var _ types.Iterator = (*cacheMergeIterator)(nil) @@ -29,6 +31,7 @@ func newCacheMergeIterator(parent, cache types.Iterator, ascending bool) *cacheM ascending: ascending, } + iter.valid = iter.skipUntilExistsOrInvalid() return iter } @@ -40,42 +43,38 @@ func (iter *cacheMergeIterator) Domain() (start, end []byte) { // Valid implements Iterator. func (iter *cacheMergeIterator) Valid() bool { - return iter.skipUntilExistsOrInvalid() + return iter.valid } // Next implements Iterator func (iter *cacheMergeIterator) Next() { - iter.skipUntilExistsOrInvalid() iter.assertValid() - // If parent is invalid, get the next cache item. - if !iter.parent.Valid() { + switch { + case !iter.parent.Valid(): + // If parent is invalid, get the next cache item. iter.cache.Next() - return - } - - // If cache is invalid, get the next parent item. - if !iter.cache.Valid() { + case !iter.cache.Valid(): + // If cache is invalid, get the next parent item. iter.parent.Next() - return - } - - // Both are valid. Compare keys. - keyP, keyC := iter.parent.Key(), iter.cache.Key() - switch iter.compare(keyP, keyC) { - case -1: // parent < cache - iter.parent.Next() - case 0: // parent == cache - iter.parent.Next() - iter.cache.Next() - case 1: // parent > cache - iter.cache.Next() + default: + // Both are valid. Compare keys. + keyP, keyC := iter.parent.Key(), iter.cache.Key() + switch iter.compare(keyP, keyC) { + case -1: // parent < cache + iter.parent.Next() + case 0: // parent == cache + iter.parent.Next() + iter.cache.Next() + case 1: // parent > cache + iter.cache.Next() + } } + iter.valid = iter.skipUntilExistsOrInvalid() } // Key implements Iterator func (iter *cacheMergeIterator) Key() []byte { - iter.skipUntilExistsOrInvalid() iter.assertValid() // If parent is invalid, get the cache key. @@ -106,7 +105,6 @@ func (iter *cacheMergeIterator) Key() []byte { // Value implements Iterator func (iter *cacheMergeIterator) Value() []byte { - iter.skipUntilExistsOrInvalid() iter.assertValid() // If parent is invalid, get the cache value. From 0c60aa15749b3189b9f5e0e491976d6ce7210bf3 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 14:15:23 +0800 Subject: [PATCH 02/19] use btree directly --- go.mod | 1 + go.sum | 2 + simapp/go.mod | 1 + simapp/go.sum | 2 + store/cachekv/btree.go | 72 +++++++++ store/cachekv/btree_test.go | 202 +++++++++++++++++++++++++ store/cachekv/memiterator.go | 97 +++++++++--- store/cachekv/search_benchmark_test.go | 4 +- store/cachekv/store.go | 17 +-- tests/go.mod | 1 + tests/go.sum | 2 + 11 files changed, 366 insertions(+), 35 deletions(-) create mode 100644 store/cachekv/btree.go create mode 100644 store/cachekv/btree_test.go diff --git a/go.mod b/go.mod index cca7fd7fe954..cc001c2e1621 100644 --- a/go.mod +++ b/go.mod @@ -54,6 +54,7 @@ require ( github.com/tendermint/go-amino v0.16.0 github.com/tendermint/tendermint v0.37.0-rc1 github.com/tendermint/tm-db v0.6.7 + github.com/tidwall/btree v1.5.2 golang.org/x/crypto v0.2.0 golang.org/x/exp v0.0.0-20221019170559-20944726eadf google.golang.org/genproto v0.0.0-20221024183307-1bc688fe9f3e diff --git a/go.sum b/go.sum index 72ec7fc66826..9740d310228f 100644 --- a/go.sum +++ b/go.sum @@ -827,6 +827,8 @@ github.com/tendermint/tendermint v0.37.0-rc1 h1:+m+u7s10QD+7vPh5MORrnYjulCdYtGuz github.com/tendermint/tendermint v0.37.0-rc1/go.mod h1:z0MZllXL+s0PgIMMpf2P0PrMttQufQio3kUjY2zebeo= github.com/tendermint/tm-db v0.6.7 h1:fE00Cbl0jayAoqlExN6oyQJ7fR/ZtoVOmvPJ//+shu8= github.com/tendermint/tm-db v0.6.7/go.mod h1:byQDzFkZV1syXr/ReXS808NxA2xvyuuVgXOJ/088L6I= +github.com/tidwall/btree v1.5.2 h1:5eA83Gfki799V3d3bJo9sWk+yL2LRoTEah3O/SA6/8w= +github.com/tidwall/btree v1.5.2/go.mod h1:twD9XRA5jj9VUQGELzDO4HPQTNJsoWWfYEL+EUQ2cKY= github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U= github.com/tv42/httpunix v0.0.0-20150427012821-b75d8614f926/go.mod h1:9ESjWnEqriFuLhtthL60Sar/7RFoluCcXsuvEwTV5KM= github.com/ugorji/go v1.2.7 h1:qYhyWUUd6WbiM+C6JZAUkIJt/1WrjzNHY9+KCIjVqTo= diff --git a/simapp/go.mod b/simapp/go.mod index e59ba3e489aa..5ee0b972bb12 100644 --- a/simapp/go.mod +++ b/simapp/go.mod @@ -137,6 +137,7 @@ require ( github.com/tendermint/btcd v0.1.1 // indirect github.com/tendermint/crypto v0.0.0-20191022145703-50d29ede1e15 // indirect github.com/tendermint/go-amino v0.16.0 // indirect + github.com/tidwall/btree v1.5.2 // indirect github.com/ulikunitz/xz v0.5.8 // indirect github.com/zondax/hid v0.9.1-0.20220302062450-5552068d2266 // indirect go.etcd.io/bbolt v1.3.6 // indirect diff --git a/simapp/go.sum b/simapp/go.sum index a6d3c29ea1ab..3c337cbf615e 100644 --- a/simapp/go.sum +++ b/simapp/go.sum @@ -815,6 +815,8 @@ github.com/tendermint/tendermint v0.37.0-rc1 h1:+m+u7s10QD+7vPh5MORrnYjulCdYtGuz github.com/tendermint/tendermint v0.37.0-rc1/go.mod h1:z0MZllXL+s0PgIMMpf2P0PrMttQufQio3kUjY2zebeo= github.com/tendermint/tm-db v0.6.7 h1:fE00Cbl0jayAoqlExN6oyQJ7fR/ZtoVOmvPJ//+shu8= github.com/tendermint/tm-db v0.6.7/go.mod h1:byQDzFkZV1syXr/ReXS808NxA2xvyuuVgXOJ/088L6I= +github.com/tidwall/btree v1.5.2 h1:5eA83Gfki799V3d3bJo9sWk+yL2LRoTEah3O/SA6/8w= +github.com/tidwall/btree v1.5.2/go.mod h1:twD9XRA5jj9VUQGELzDO4HPQTNJsoWWfYEL+EUQ2cKY= github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U= github.com/tv42/httpunix v0.0.0-20150427012821-b75d8614f926/go.mod h1:9ESjWnEqriFuLhtthL60Sar/7RFoluCcXsuvEwTV5KM= github.com/ugorji/go v1.2.7 h1:qYhyWUUd6WbiM+C6JZAUkIJt/1WrjzNHY9+KCIjVqTo= diff --git a/store/cachekv/btree.go b/store/cachekv/btree.go new file mode 100644 index 000000000000..88984a4a11a9 --- /dev/null +++ b/store/cachekv/btree.go @@ -0,0 +1,72 @@ +package cachekv + +import ( + "bytes" + "errors" + + "github.com/tidwall/btree" +) + +var errKeyEmpty = errors.New("key cannot be empty") + +type BTree struct { + tree btree.BTreeG[item] +} + +// NewBTree creates a wrapper around `btree.BTreeG`. +func NewBTree() *BTree { + return &BTree{tree: *btree.NewBTreeGOptions(byKeys, btree.Options{ + Degree: 32, + NoLocks: true, + })} +} + +func (bt *BTree) Set(key, value []byte) { + bt.tree.Set(newPair(key, value)) +} + +func (bt *BTree) Get(key []byte) []byte { + i, found := bt.tree.Get(newKey(key)) + if !found { + return nil + } + return i.value +} + +func (bt *BTree) Delete(key []byte) { + bt.tree.Delete(newKey(key)) +} + +func (bt *BTree) Iterator(start, end []byte) (*memIterator, error) { + if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) { + return nil, errKeyEmpty + } + return newMemIterator(start, end, bt, make(map[string]struct{}), true), nil +} + +func (bt *BTree) ReverseIterator(start, end []byte) (*memIterator, error) { + if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) { + return nil, errKeyEmpty + } + return newMemIterator(start, end, bt, make(map[string]struct{}), false), nil +} + +// item is a btree.Item with byte slices as keys and values +type item struct { + key []byte + value []byte +} + +// byKeys compares the items by key +func byKeys(a, b item) bool { + return bytes.Compare(a.key, b.key) == -1 +} + +// newPair creates a new pair item. +func newPair(key, value []byte) item { + return item{key: key, value: value} +} + +func newKey(key []byte) item { + return item{key: key} +} diff --git a/store/cachekv/btree_test.go b/store/cachekv/btree_test.go new file mode 100644 index 000000000000..df42028ee709 --- /dev/null +++ b/store/cachekv/btree_test.go @@ -0,0 +1,202 @@ +package cachekv + +import ( + "encoding/binary" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestGetSetDelete(t *testing.T) { + db := NewBTree() + + // A nonexistent key should return nil. + value := db.Get([]byte("a")) + require.Nil(t, value) + + // Set and get a value. + db.Set([]byte("a"), []byte{0x01}) + db.Set([]byte("b"), []byte{0x02}) + value = db.Get([]byte("a")) + require.Equal(t, []byte{0x01}, value) + + value = db.Get([]byte("b")) + require.Equal(t, []byte{0x02}, value) + + // Deleting a non-existent value is fine. + db.Delete([]byte("x")) + + // Delete a value. + db.Delete([]byte("a")) + + value = db.Get([]byte("a")) + require.Nil(t, value) + + db.Delete([]byte("b")) + + value = db.Get([]byte("b")) + require.Nil(t, value) +} + +func TestDBIterator(t *testing.T) { + db := NewBTree() + + for i := 0; i < 10; i++ { + if i != 6 { // but skip 6. + db.Set(int642Bytes(int64(i)), []byte{}) + } + } + + // Blank iterator keys should error + _, err := db.ReverseIterator([]byte{}, nil) + require.Equal(t, errKeyEmpty, err) + _, err = db.ReverseIterator(nil, []byte{}) + require.Equal(t, errKeyEmpty, err) + + itr, err := db.Iterator(nil, nil) + require.NoError(t, err) + verifyIterator(t, itr, []int64{0, 1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator") + + ritr, err := db.ReverseIterator(nil, nil) + require.NoError(t, err) + verifyIterator(t, ritr, []int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator") + + itr, err = db.Iterator(nil, int642Bytes(0)) + require.NoError(t, err) + verifyIterator(t, itr, []int64(nil), "forward iterator to 0") + + ritr, err = db.ReverseIterator(int642Bytes(10), nil) + require.NoError(t, err) + verifyIterator(t, ritr, []int64(nil), "reverse iterator from 10 (ex)") + + itr, err = db.Iterator(int642Bytes(0), nil) + require.NoError(t, err) + verifyIterator(t, itr, []int64{0, 1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator from 0") + + itr, err = db.Iterator(int642Bytes(1), nil) + require.NoError(t, err) + verifyIterator(t, itr, []int64{1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator from 1") + + ritr, err = db.ReverseIterator(nil, int642Bytes(10)) + require.NoError(t, err) + verifyIterator(t, ritr, + []int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 10 (ex)") + + ritr, err = db.ReverseIterator(nil, int642Bytes(9)) + require.NoError(t, err) + verifyIterator(t, ritr, + []int64{8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 9 (ex)") + + ritr, err = db.ReverseIterator(nil, int642Bytes(8)) + require.NoError(t, err) + verifyIterator(t, ritr, + []int64{7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 8 (ex)") + + itr, err = db.Iterator(int642Bytes(5), int642Bytes(6)) + require.NoError(t, err) + verifyIterator(t, itr, []int64{5}, "forward iterator from 5 to 6") + + itr, err = db.Iterator(int642Bytes(5), int642Bytes(7)) + require.NoError(t, err) + verifyIterator(t, itr, []int64{5}, "forward iterator from 5 to 7") + + itr, err = db.Iterator(int642Bytes(5), int642Bytes(8)) + require.NoError(t, err) + verifyIterator(t, itr, []int64{5, 7}, "forward iterator from 5 to 8") + + itr, err = db.Iterator(int642Bytes(6), int642Bytes(7)) + require.NoError(t, err) + verifyIterator(t, itr, []int64(nil), "forward iterator from 6 to 7") + + itr, err = db.Iterator(int642Bytes(6), int642Bytes(8)) + require.NoError(t, err) + verifyIterator(t, itr, []int64{7}, "forward iterator from 6 to 8") + + itr, err = db.Iterator(int642Bytes(7), int642Bytes(8)) + require.NoError(t, err) + verifyIterator(t, itr, []int64{7}, "forward iterator from 7 to 8") + + ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(5)) + require.NoError(t, err) + verifyIterator(t, ritr, []int64{4}, "reverse iterator from 5 (ex) to 4") + + ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(6)) + require.NoError(t, err) + verifyIterator(t, ritr, + []int64{5, 4}, "reverse iterator from 6 (ex) to 4") + + ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(7)) + require.NoError(t, err) + verifyIterator(t, ritr, + []int64{5, 4}, "reverse iterator from 7 (ex) to 4") + + ritr, err = db.ReverseIterator(int642Bytes(5), int642Bytes(6)) + require.NoError(t, err) + verifyIterator(t, ritr, []int64{5}, "reverse iterator from 6 (ex) to 5") + + ritr, err = db.ReverseIterator(int642Bytes(5), int642Bytes(7)) + require.NoError(t, err) + verifyIterator(t, ritr, []int64{5}, "reverse iterator from 7 (ex) to 5") + + ritr, err = db.ReverseIterator(int642Bytes(6), int642Bytes(7)) + require.NoError(t, err) + verifyIterator(t, ritr, + []int64(nil), "reverse iterator from 7 (ex) to 6") + + ritr, err = db.ReverseIterator(int642Bytes(10), nil) + require.NoError(t, err) + verifyIterator(t, ritr, []int64(nil), "reverse iterator to 10") + + ritr, err = db.ReverseIterator(int642Bytes(6), nil) + require.NoError(t, err) + verifyIterator(t, ritr, []int64{9, 8, 7}, "reverse iterator to 6") + + ritr, err = db.ReverseIterator(int642Bytes(5), nil) + require.NoError(t, err) + verifyIterator(t, ritr, []int64{9, 8, 7, 5}, "reverse iterator to 5") + + ritr, err = db.ReverseIterator(int642Bytes(8), int642Bytes(9)) + require.NoError(t, err) + verifyIterator(t, ritr, []int64{8}, "reverse iterator from 9 (ex) to 8") + + ritr, err = db.ReverseIterator(int642Bytes(2), int642Bytes(4)) + require.NoError(t, err) + verifyIterator(t, ritr, + []int64{3, 2}, "reverse iterator from 4 (ex) to 2") + + ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(2)) + require.NoError(t, err) + verifyIterator(t, ritr, + []int64(nil), "reverse iterator from 2 (ex) to 4") + + // Ensure that the iterators don't panic with an empty database. + db2 := NewBTree() + + itr, err = db2.Iterator(nil, nil) + require.NoError(t, err) + verifyIterator(t, itr, nil, "forward iterator with empty db") + + ritr, err = db2.ReverseIterator(nil, nil) + require.NoError(t, err) + verifyIterator(t, ritr, nil, "reverse iterator with empty db") +} + +func verifyIterator(t *testing.T, itr *memIterator, expected []int64, msg string) { + var list []int64 + for itr.Valid() { + key := itr.Key() + list = append(list, bytes2Int64(key)) + itr.Next() + } + require.Equal(t, expected, list, msg) +} + +func int642Bytes(i int64) []byte { + buf := make([]byte, 8) + binary.BigEndian.PutUint64(buf, uint64(i)) + return buf +} + +func bytes2Int64(buf []byte) int64 { + return int64(binary.BigEndian.Uint64(buf)) +} diff --git a/store/cachekv/memiterator.go b/store/cachekv/memiterator.go index a12ff9acfd11..f1337af5fcb6 100644 --- a/store/cachekv/memiterator.go +++ b/store/cachekv/memiterator.go @@ -3,46 +3,101 @@ package cachekv import ( "bytes" - dbm "github.com/tendermint/tm-db" - "github.com/cosmos/cosmos-sdk/store/types" + "github.com/tidwall/btree" ) +var _ types.Iterator = &memIterator{} + // memIterator iterates over iterKVCache items. // if key is nil, means it was deleted. // Implements Iterator. type memIterator struct { - types.Iterator + iter btree.GenericIter[item] - lastKey []byte - deleted map[string]struct{} + start []byte + end []byte + ascending bool + lastKey []byte + deleted map[string]struct{} + valid bool } -func newMemIterator(start, end []byte, items *dbm.MemDB, deleted map[string]struct{}, ascending bool) *memIterator { - var ( - iter types.Iterator - err error - ) - +func newMemIterator(start, end []byte, items *BTree, deleted map[string]struct{}, ascending bool) *memIterator { + iter := items.tree.Iter() + var valid bool if ascending { - iter, err = items.Iterator(start, end) + if start != nil { + valid = iter.Seek(newKey(start)) + } else { + valid = iter.First() + } } else { - iter, err = items.ReverseIterator(start, end) + if end != nil { + valid = iter.Seek(newKey(end)) + if !valid { + valid = iter.Last() + } else { + // end is exclusive + valid = iter.Prev() + } + } else { + valid = iter.Last() + } + } + return &memIterator{ + iter: iter, + start: start, + end: end, + ascending: ascending, + lastKey: nil, + deleted: deleted, + valid: valid, } +} + +func (mi *memIterator) Domain() (start []byte, end []byte) { + return mi.start, mi.end +} + +func (mi *memIterator) Close() error { + mi.iter.Release() + return nil +} - if err != nil { - panic(err) +func (mi *memIterator) Error() error { + return nil +} + +func (mi *memIterator) Valid() bool { + if !mi.valid { + return false + } + key := mi.iter.Item().key + if mi.ascending && mi.end != nil && bytes.Compare(key, mi.end) >= 0 { + return false } + if !mi.ascending && mi.start != nil && bytes.Compare(key, mi.start) < 0 { + return false + } + return true +} - return &memIterator{ - Iterator: iter, - lastKey: nil, - deleted: deleted, +func (mi *memIterator) Next() { + if mi.ascending { + mi.valid = mi.iter.Next() + } else { + mi.valid = mi.iter.Prev() } } +func (mi *memIterator) Key() []byte { + return mi.iter.Item().key +} + func (mi *memIterator) Value() []byte { - key := mi.Iterator.Key() + item := mi.iter.Item() + key := item.key // We need to handle the case where deleted is modified and includes our current key // We handle this by maintaining a lastKey object in the iterator. // If the current key is the same as the last key (and last key is not nil / the start) @@ -53,5 +108,5 @@ func (mi *memIterator) Value() []byte { return nil } mi.lastKey = key - return mi.Iterator.Value() + return item.value } diff --git a/store/cachekv/search_benchmark_test.go b/store/cachekv/search_benchmark_test.go index 921bff4e3864..808241b1b826 100644 --- a/store/cachekv/search_benchmark_test.go +++ b/store/cachekv/search_benchmark_test.go @@ -3,8 +3,6 @@ package cachekv import ( "strconv" "testing" - - db "github.com/tendermint/tm-db" ) func BenchmarkLargeUnsortedMisses(b *testing.B) { @@ -39,6 +37,6 @@ func generateStore() *Store { return &Store{ cache: cache, unsortedCache: unsorted, - sortedCache: db.NewMemDB(), + sortedCache: NewBTree(), } } diff --git a/store/cachekv/store.go b/store/cachekv/store.go index 6d38fd09181a..c8acc22c105e 100644 --- a/store/cachekv/store.go +++ b/store/cachekv/store.go @@ -31,7 +31,7 @@ type Store struct { cache map[string]*cValue deleted map[string]struct{} unsortedCache map[string]struct{} - sortedCache *dbm.MemDB // always ascending sorted + sortedCache *BTree // always ascending sorted parent types.KVStore } @@ -43,7 +43,7 @@ func NewStore(parent types.KVStore) *Store { cache: make(map[string]*cValue), deleted: make(map[string]struct{}), unsortedCache: make(map[string]struct{}), - sortedCache: dbm.NewMemDB(), + sortedCache: NewBTree(), parent: parent, } } @@ -103,7 +103,7 @@ func (store *Store) Write() { defer store.mtx.Unlock() if len(store.cache) == 0 && len(store.deleted) == 0 && len(store.unsortedCache) == 0 { - store.sortedCache = dbm.NewMemDB() + store.sortedCache = NewBTree() return } @@ -150,7 +150,7 @@ func (store *Store) Write() { for key := range store.unsortedCache { delete(store.unsortedCache, key) } - store.sortedCache = dbm.NewMemDB() + store.sortedCache = NewBTree() } // CacheWrap implements CacheWrapper. @@ -378,16 +378,11 @@ func (store *Store) clearUnsortedCacheSubset(unsorted []*kv.Pair, sortState sort if item.Value == nil { // deleted element, tracked by store.deleted // setting arbitrary value - if err := store.sortedCache.Set(item.Key, []byte{}); err != nil { - panic(err) - } - + store.sortedCache.Set(item.Key, []byte{}) continue } - if err := store.sortedCache.Set(item.Key, item.Value); err != nil { - panic(err) - } + store.sortedCache.Set(item.Key, item.Value) } } diff --git a/tests/go.mod b/tests/go.mod index 93871a5bc2fc..62b82e73b532 100644 --- a/tests/go.mod +++ b/tests/go.mod @@ -140,6 +140,7 @@ require ( github.com/tendermint/btcd v0.1.1 // indirect github.com/tendermint/crypto v0.0.0-20191022145703-50d29ede1e15 // indirect github.com/tendermint/go-amino v0.16.0 // indirect + github.com/tidwall/btree v1.5.2 // indirect github.com/ulikunitz/xz v0.5.8 // indirect github.com/zondax/hid v0.9.1-0.20220302062450-5552068d2266 // indirect go.etcd.io/bbolt v1.3.6 // indirect diff --git a/tests/go.sum b/tests/go.sum index e89f01ca6913..94eb2d6ad809 100644 --- a/tests/go.sum +++ b/tests/go.sum @@ -815,6 +815,8 @@ github.com/tendermint/tendermint v0.37.0-rc1 h1:+m+u7s10QD+7vPh5MORrnYjulCdYtGuz github.com/tendermint/tendermint v0.37.0-rc1/go.mod h1:z0MZllXL+s0PgIMMpf2P0PrMttQufQio3kUjY2zebeo= github.com/tendermint/tm-db v0.6.7 h1:fE00Cbl0jayAoqlExN6oyQJ7fR/ZtoVOmvPJ//+shu8= github.com/tendermint/tm-db v0.6.7/go.mod h1:byQDzFkZV1syXr/ReXS808NxA2xvyuuVgXOJ/088L6I= +github.com/tidwall/btree v1.5.2 h1:5eA83Gfki799V3d3bJo9sWk+yL2LRoTEah3O/SA6/8w= +github.com/tidwall/btree v1.5.2/go.mod h1:twD9XRA5jj9VUQGELzDO4HPQTNJsoWWfYEL+EUQ2cKY= github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U= github.com/tv42/httpunix v0.0.0-20150427012821-b75d8614f926/go.mod h1:9ESjWnEqriFuLhtthL60Sar/7RFoluCcXsuvEwTV5KM= github.com/ugorji/go v1.2.7 h1:qYhyWUUd6WbiM+C6JZAUkIJt/1WrjzNHY9+KCIjVqTo= From 08454af92b4598e2454faf1bb8cb9f3692588df0 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 17:47:55 +0800 Subject: [PATCH 03/19] add comments --- store/cachekv/btree.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/store/cachekv/btree.go b/store/cachekv/btree.go index 88984a4a11a9..c4f65c164a97 100644 --- a/store/cachekv/btree.go +++ b/store/cachekv/btree.go @@ -9,6 +9,11 @@ import ( var errKeyEmpty = errors.New("key cannot be empty") +// BTree implements the sorted cache for cachekv store, +// we don't use MemDB here because we don't need thread safty here, +// and since cachekv is used extensively in sdk core path, the faster the better. +// +// We choose tidwall/btree over google/btree here because it provides API for step iterator. type BTree struct { tree btree.BTreeG[item] } From 48fb0a71ecf4edf2c9fa386f2b36c3810c2105ff Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 17:55:46 +0800 Subject: [PATCH 04/19] move to internal package --- store/cachekv/{ => internal}/btree.go | 6 +++--- store/cachekv/{ => internal}/btree_test.go | 2 +- store/cachekv/{ => internal}/memiterator.go | 4 ++-- store/cachekv/{ => internal}/mergeiterator.go | 4 ++-- store/cachekv/search_benchmark_test.go | 4 +++- store/cachekv/store.go | 13 +++++++------ 6 files changed, 18 insertions(+), 15 deletions(-) rename store/cachekv/{ => internal}/btree.go (92%) rename store/cachekv/{ => internal}/btree_test.go (99%) rename store/cachekv/{ => internal}/memiterator.go (96%) rename store/cachekv/{ => internal}/mergeiterator.go (98%) diff --git a/store/cachekv/btree.go b/store/cachekv/internal/btree.go similarity index 92% rename from store/cachekv/btree.go rename to store/cachekv/internal/btree.go index c4f65c164a97..a81c2e454150 100644 --- a/store/cachekv/btree.go +++ b/store/cachekv/internal/btree.go @@ -1,4 +1,4 @@ -package cachekv +package internal import ( "bytes" @@ -46,14 +46,14 @@ func (bt *BTree) Iterator(start, end []byte) (*memIterator, error) { if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) { return nil, errKeyEmpty } - return newMemIterator(start, end, bt, make(map[string]struct{}), true), nil + return NewMemIterator(start, end, bt, make(map[string]struct{}), true), nil } func (bt *BTree) ReverseIterator(start, end []byte) (*memIterator, error) { if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) { return nil, errKeyEmpty } - return newMemIterator(start, end, bt, make(map[string]struct{}), false), nil + return NewMemIterator(start, end, bt, make(map[string]struct{}), false), nil } // item is a btree.Item with byte slices as keys and values diff --git a/store/cachekv/btree_test.go b/store/cachekv/internal/btree_test.go similarity index 99% rename from store/cachekv/btree_test.go rename to store/cachekv/internal/btree_test.go index df42028ee709..7fcf65423f31 100644 --- a/store/cachekv/btree_test.go +++ b/store/cachekv/internal/btree_test.go @@ -1,4 +1,4 @@ -package cachekv +package internal import ( "encoding/binary" diff --git a/store/cachekv/memiterator.go b/store/cachekv/internal/memiterator.go similarity index 96% rename from store/cachekv/memiterator.go rename to store/cachekv/internal/memiterator.go index f1337af5fcb6..0f31af98e3c3 100644 --- a/store/cachekv/memiterator.go +++ b/store/cachekv/internal/memiterator.go @@ -1,4 +1,4 @@ -package cachekv +package internal import ( "bytes" @@ -23,7 +23,7 @@ type memIterator struct { valid bool } -func newMemIterator(start, end []byte, items *BTree, deleted map[string]struct{}, ascending bool) *memIterator { +func NewMemIterator(start, end []byte, items *BTree, deleted map[string]struct{}, ascending bool) *memIterator { iter := items.tree.Iter() var valid bool if ascending { diff --git a/store/cachekv/mergeiterator.go b/store/cachekv/internal/mergeiterator.go similarity index 98% rename from store/cachekv/mergeiterator.go rename to store/cachekv/internal/mergeiterator.go index 59bb10190ae9..7938e63399be 100644 --- a/store/cachekv/mergeiterator.go +++ b/store/cachekv/internal/mergeiterator.go @@ -1,4 +1,4 @@ -package cachekv +package internal import ( "bytes" @@ -24,7 +24,7 @@ type cacheMergeIterator struct { var _ types.Iterator = (*cacheMergeIterator)(nil) -func newCacheMergeIterator(parent, cache types.Iterator, ascending bool) *cacheMergeIterator { +func NewCacheMergeIterator(parent, cache types.Iterator, ascending bool) *cacheMergeIterator { iter := &cacheMergeIterator{ parent: parent, cache: cache, diff --git a/store/cachekv/search_benchmark_test.go b/store/cachekv/search_benchmark_test.go index 808241b1b826..4007c7cda202 100644 --- a/store/cachekv/search_benchmark_test.go +++ b/store/cachekv/search_benchmark_test.go @@ -3,6 +3,8 @@ package cachekv import ( "strconv" "testing" + + "github.com/cosmos/cosmos-sdk/store/cachekv/internal" ) func BenchmarkLargeUnsortedMisses(b *testing.B) { @@ -37,6 +39,6 @@ func generateStore() *Store { return &Store{ cache: cache, unsortedCache: unsorted, - sortedCache: NewBTree(), + sortedCache: internal.NewBTree(), } } diff --git a/store/cachekv/store.go b/store/cachekv/store.go index c8acc22c105e..42e20d760b14 100644 --- a/store/cachekv/store.go +++ b/store/cachekv/store.go @@ -10,6 +10,7 @@ import ( dbm "github.com/tendermint/tm-db" "github.com/cosmos/cosmos-sdk/internal/conv" + "github.com/cosmos/cosmos-sdk/store/cachekv/internal" "github.com/cosmos/cosmos-sdk/store/listenkv" "github.com/cosmos/cosmos-sdk/store/tracekv" "github.com/cosmos/cosmos-sdk/store/types" @@ -31,7 +32,7 @@ type Store struct { cache map[string]*cValue deleted map[string]struct{} unsortedCache map[string]struct{} - sortedCache *BTree // always ascending sorted + sortedCache *internal.BTree // always ascending sorted parent types.KVStore } @@ -43,7 +44,7 @@ func NewStore(parent types.KVStore) *Store { cache: make(map[string]*cValue), deleted: make(map[string]struct{}), unsortedCache: make(map[string]struct{}), - sortedCache: NewBTree(), + sortedCache: internal.NewBTree(), parent: parent, } } @@ -103,7 +104,7 @@ func (store *Store) Write() { defer store.mtx.Unlock() if len(store.cache) == 0 && len(store.deleted) == 0 && len(store.unsortedCache) == 0 { - store.sortedCache = NewBTree() + store.sortedCache = internal.NewBTree() return } @@ -150,7 +151,7 @@ func (store *Store) Write() { for key := range store.unsortedCache { delete(store.unsortedCache, key) } - store.sortedCache = NewBTree() + store.sortedCache = internal.NewBTree() } // CacheWrap implements CacheWrapper. @@ -194,9 +195,9 @@ func (store *Store) iterator(start, end []byte, ascending bool) types.Iterator { } store.dirtyItems(start, end) - cache = newMemIterator(start, end, store.sortedCache, store.deleted, ascending) + cache = internal.NewMemIterator(start, end, store.sortedCache, store.deleted, ascending) - return newCacheMergeIterator(parent, cache, ascending) + return internal.NewCacheMergeIterator(parent, cache, ascending) } func findStartIndex(strL []string, startQ string) int { From 1bd5213de010408d93ecb3afd42c012e5da4e554 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 17:57:58 +0800 Subject: [PATCH 05/19] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c61d11a6d03a..5ada86218650 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#13802](https://github.com/cosmos/cosmos-sdk/pull/13802) Add --output-document flag to the export CLI command to allow writing genesis state to a file. * [#13794](https://github.com/cosmos/cosmos-sdk/pull/13794) `types/module.Manager` now supports the `cosmossdk.io/core/appmodule.AppModule` API via the new `NewManagerFromMap` constructor. +* [#13881](https://github.com/cosmos/cosmos-sdk/pull/13881) optimize iteration on nested cache context and other operations in general. ### State Machine Breaking From 1f629ae74d6c1a355c1478085629577fc7c21d46 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 18:03:35 +0800 Subject: [PATCH 06/19] fix lint --- store/cachekv/internal/btree.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go index a81c2e454150..196af5d98fa7 100644 --- a/store/cachekv/internal/btree.go +++ b/store/cachekv/internal/btree.go @@ -10,7 +10,7 @@ import ( var errKeyEmpty = errors.New("key cannot be empty") // BTree implements the sorted cache for cachekv store, -// we don't use MemDB here because we don't need thread safty here, +// we don't use MemDB here because we don't need thread safety here, // and since cachekv is used extensively in sdk core path, the faster the better. // // We choose tidwall/btree over google/btree here because it provides API for step iterator. From d9bec0d8cfd9f1f01022de2443474b849aeabf3a Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 18:21:20 +0800 Subject: [PATCH 07/19] update comments --- store/cachekv/internal/btree.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go index 196af5d98fa7..ab87a64546d5 100644 --- a/store/cachekv/internal/btree.go +++ b/store/cachekv/internal/btree.go @@ -7,13 +7,19 @@ import ( "github.com/tidwall/btree" ) +const ( + // The approximate number of items and children per B-tree node. Tuned with benchmarks. + // copied from memdb. + bTreeDegree = 32 +) + var errKeyEmpty = errors.New("key cannot be empty") // BTree implements the sorted cache for cachekv store, -// we don't use MemDB here because we don't need thread safety here, -// and since cachekv is used extensively in sdk core path, the faster the better. +// we don't use MemDB here because cachekv is used extensively in sdk core path, +// we need it to be as fast as possible. // -// We choose tidwall/btree over google/btree here because it provides API for step iterator. +// We choose tidwall/btree over google/btree here because it provides API to implement step iterator directly. type BTree struct { tree btree.BTreeG[item] } @@ -21,8 +27,10 @@ type BTree struct { // NewBTree creates a wrapper around `btree.BTreeG`. func NewBTree() *BTree { return &BTree{tree: *btree.NewBTreeGOptions(byKeys, btree.Options{ - Degree: 32, - NoLocks: true, + Degree: bTreeDegree, + // we do need to enable locks here, although cachekv is protected with lock, + // because multiple iterators could access the btree concurrently. + NoLocks: false, })} } From a5d2f2015f5c1f161dca6e9ab7c34e39e52077cf Mon Sep 17 00:00:00 2001 From: yihuang Date: Wed, 16 Nov 2022 18:54:10 +0800 Subject: [PATCH 08/19] Update store/cachekv/internal/btree.go --- store/cachekv/internal/btree.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go index ab87a64546d5..3ff25728c926 100644 --- a/store/cachekv/internal/btree.go +++ b/store/cachekv/internal/btree.go @@ -17,7 +17,7 @@ var errKeyEmpty = errors.New("key cannot be empty") // BTree implements the sorted cache for cachekv store, // we don't use MemDB here because cachekv is used extensively in sdk core path, -// we need it to be as fast as possible. +// we need it to be as fast as possible, while `MemDB` is mainly used as a mocking db in unit tests. // // We choose tidwall/btree over google/btree here because it provides API to implement step iterator directly. type BTree struct { From 6d301f49bd079c18b058cad2b7dba0e2b99ca7ba Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 19:48:54 +0800 Subject: [PATCH 09/19] no locks --- store/cachekv/internal/btree.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go index ab87a64546d5..d476219ee36f 100644 --- a/store/cachekv/internal/btree.go +++ b/store/cachekv/internal/btree.go @@ -28,9 +28,8 @@ type BTree struct { func NewBTree() *BTree { return &BTree{tree: *btree.NewBTreeGOptions(byKeys, btree.Options{ Degree: bTreeDegree, - // we do need to enable locks here, although cachekv is protected with lock, - // because multiple iterators could access the btree concurrently. - NoLocks: false, + // we don't need to enable locks here, because cachekv should not be called concurrently. + NoLocks: true, })} } From cf19f22437d827b2c1f43a49625f43275c8110d3 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 20:23:09 +0800 Subject: [PATCH 10/19] add contract comment about concurrency --- store/cachekv/internal/btree.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go index c5deb9d3f6b6..d1060be91328 100644 --- a/store/cachekv/internal/btree.go +++ b/store/cachekv/internal/btree.go @@ -28,7 +28,7 @@ type BTree struct { func NewBTree() *BTree { return &BTree{tree: *btree.NewBTreeGOptions(byKeys, btree.Options{ Degree: bTreeDegree, - // we don't need to enable locks here, because cachekv should not be called concurrently. + // Contract: cachekv store must not be called concurrently NoLocks: true, })} } From 5eed69704e6aae3b95364742fc3aedf146c15a9b Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 20:28:46 +0800 Subject: [PATCH 11/19] close iterators properly --- store/cachekv/internal/mergeiterator.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/store/cachekv/internal/mergeiterator.go b/store/cachekv/internal/mergeiterator.go index 7938e63399be..4186a178a863 100644 --- a/store/cachekv/internal/mergeiterator.go +++ b/store/cachekv/internal/mergeiterator.go @@ -135,11 +135,12 @@ func (iter *cacheMergeIterator) Value() []byte { // Close implements Iterator func (iter *cacheMergeIterator) Close() error { + err1 := iter.cache.Close() if err := iter.parent.Close(); err != nil { return err } - return iter.cache.Close() + return err1 } // Error returns an error if the cacheMergeIterator is invalid defined by the From 99888ddb1b3fc4043ab06a3debf15854e098959c Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 16 Nov 2022 21:14:28 +0800 Subject: [PATCH 12/19] cleanup with review suggestions --- store/cachekv/internal/btree_test.go | 7 ++-- store/cachekv/internal/memiterator.go | 57 +++++++++++++++++++-------- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/store/cachekv/internal/btree_test.go b/store/cachekv/internal/btree_test.go index 7fcf65423f31..e31a7cdd2782 100644 --- a/store/cachekv/internal/btree_test.go +++ b/store/cachekv/internal/btree_test.go @@ -182,13 +182,14 @@ func TestDBIterator(t *testing.T) { } func verifyIterator(t *testing.T, itr *memIterator, expected []int64, msg string) { - var list []int64 + i := 0 for itr.Valid() { key := itr.Key() - list = append(list, bytes2Int64(key)) + require.Equal(t, expected[i], bytes2Int64(key), "iterator: %d mismatches", i) itr.Next() + i++ } - require.Equal(t, expected, list, msg) + require.Equal(t, i, len(expected), "expected to have fully iterated over all the elements in iter") } func int642Bytes(i int64) []byte { diff --git a/store/cachekv/internal/memiterator.go b/store/cachekv/internal/memiterator.go index 0f31af98e3c3..458425af33cd 100644 --- a/store/cachekv/internal/memiterator.go +++ b/store/cachekv/internal/memiterator.go @@ -2,12 +2,13 @@ package internal import ( "bytes" + "errors" "github.com/cosmos/cosmos-sdk/store/types" "github.com/tidwall/btree" ) -var _ types.Iterator = &memIterator{} +var _ types.Iterator = (*memIterator)(nil) // memIterator iterates over iterKVCache items. // if key is nil, means it was deleted. @@ -45,7 +46,8 @@ func NewMemIterator(start, end []byte, items *BTree, deleted map[string]struct{} valid = iter.Last() } } - return &memIterator{ + + mi := &memIterator{ iter: iter, start: start, end: end, @@ -54,6 +56,12 @@ func NewMemIterator(start, end []byte, items *BTree, deleted map[string]struct{} deleted: deleted, valid: valid, } + + if mi.valid { + mi.valid = mi.keyInRange(mi.Key()) + } + + return mi } func (mi *memIterator) Domain() (start []byte, end []byte) { @@ -66,29 +74,38 @@ func (mi *memIterator) Close() error { } func (mi *memIterator) Error() error { + if !mi.Valid() { + return errors.New("invalid memIterator") + } return nil } func (mi *memIterator) Valid() bool { - if !mi.valid { - return false - } - key := mi.iter.Item().key - if mi.ascending && mi.end != nil && bytes.Compare(key, mi.end) >= 0 { - return false - } - if !mi.ascending && mi.start != nil && bytes.Compare(key, mi.start) < 0 { - return false - } - return true + return mi.valid } func (mi *memIterator) Next() { + mi.assertValid() + if mi.ascending { mi.valid = mi.iter.Next() } else { mi.valid = mi.iter.Prev() } + + if mi.valid { + mi.valid = mi.keyInRange(mi.Key()) + } +} + +func (mi *memIterator) keyInRange(key []byte) bool { + if mi.ascending && mi.end != nil && bytes.Compare(key, mi.end) >= 0 { + return false + } + if !mi.ascending && mi.start != nil && bytes.Compare(key, mi.start) < 0 { + return false + } + return true } func (mi *memIterator) Key() []byte { @@ -103,10 +120,18 @@ func (mi *memIterator) Value() []byte { // If the current key is the same as the last key (and last key is not nil / the start) // then we are calling value on the same thing as last time. // Therefore we don't check the mi.deleted to see if this key is included in there. - reCallingOnOldLastKey := (mi.lastKey != nil) && bytes.Equal(key, mi.lastKey) - if _, ok := mi.deleted[string(key)]; ok && !reCallingOnOldLastKey { - return nil + if _, ok := mi.deleted[string(key)]; ok { + if mi.lastKey == nil || !bytes.Equal(key, mi.lastKey) { + // not re-calling on old last key + return nil + } } mi.lastKey = key return item.value } + +func (mi *memIterator) assertValid() { + if err := mi.Error(); err != nil { + panic(err) + } +} From c5dca14489bf6e9bfa43394f6e61fcd7230c8ff5 Mon Sep 17 00:00:00 2001 From: yihuang Date: Thu, 17 Nov 2022 09:53:33 +0800 Subject: [PATCH 13/19] Update CHANGELOG.md Co-authored-by: Aleksandr Bezobchuk --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4137cb382f5d..2f1be689aa98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,7 +94,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#13802](https://github.com/cosmos/cosmos-sdk/pull/13802) Add --output-document flag to the export CLI command to allow writing genesis state to a file. * [#13794](https://github.com/cosmos/cosmos-sdk/pull/13794) `types/module.Manager` now supports the `cosmossdk.io/core/appmodule.AppModule` API via the new `NewManagerFromMap` constructor. -* [#13881](https://github.com/cosmos/cosmos-sdk/pull/13881) optimize iteration on nested cache context and other operations in general. +* [#13881](https://github.com/cosmos/cosmos-sdk/pull/13881) Optimize iteration on nested cached KV stores and other operations in general. ### State Machine Breaking From 4cd470ef7f484ae76d32f8f1a8ac137e2d1b575a Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 17 Nov 2022 10:14:28 +0800 Subject: [PATCH 14/19] review suggestions --- store/cachekv/benchmark_test.go | 9 ++++----- store/cachekv/internal/btree.go | 14 +++++--------- store/cachekv/internal/btree_test.go | 8 +++----- store/cachekv/internal/memiterator.go | 4 ++-- 4 files changed, 14 insertions(+), 21 deletions(-) diff --git a/store/cachekv/benchmark_test.go b/store/cachekv/benchmark_test.go index 66873f8e047b..2db62ba5d6c6 100644 --- a/store/cachekv/benchmark_test.go +++ b/store/cachekv/benchmark_test.go @@ -34,6 +34,8 @@ func DoBenchmarkDeepContextStack(b *testing.B, depth int) { } store := stack.CurrentContext().KVStore(key) + + b.ResetTimer() for i := 0; i < b.N; i++ { it := store.Iterator(begin, end) it.Valid() @@ -68,6 +70,8 @@ type cachedContext struct { // ContextStack manages the initial context and a stack of cached contexts, // to support the `StateDB.Snapshot` and `StateDB.RevertToSnapshot` methods. +// +// Copied from an old version of ethermint type ContextStack struct { // Context of the initial state before transaction execution. // It's the context used by `StateDB.CommitedState`. @@ -102,8 +106,6 @@ func (cs *ContextStack) IsEmpty() bool { func (cs *ContextStack) Commit() { // commit in order from top to bottom for i := len(cs.cachedContexts) - 1; i >= 0; i-- { - // keep all the cosmos events - cs.initialCtx.EventManager().EmitEvents(cs.cachedContexts[i].ctx.EventManager().Events()) if cs.cachedContexts[i].commit == nil { panic(fmt.Sprintf("commit function at index %d should not be nil", i)) } else { @@ -120,11 +122,8 @@ func (cs *ContextStack) CommitToRevision(target int) error { return fmt.Errorf("snapshot index %d out of bound [%d..%d)", target, 0, len(cs.cachedContexts)) } - targetCtx := cs.cachedContexts[target].ctx // commit in order from top to bottom for i := len(cs.cachedContexts) - 1; i > target; i-- { - // keep all the cosmos events - targetCtx.EventManager().EmitEvents(cs.cachedContexts[i].ctx.EventManager().Events()) if cs.cachedContexts[i].commit == nil { return fmt.Errorf("commit function at index %d should not be nil", i) } diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go index d1060be91328..b563a0a207fe 100644 --- a/store/cachekv/internal/btree.go +++ b/store/cachekv/internal/btree.go @@ -34,11 +34,11 @@ func NewBTree() *BTree { } func (bt *BTree) Set(key, value []byte) { - bt.tree.Set(newPair(key, value)) + bt.tree.Set(newItem(key, value)) } func (bt *BTree) Get(key []byte) []byte { - i, found := bt.tree.Get(newKey(key)) + i, found := bt.tree.Get(newItem(key, nil)) if !found { return nil } @@ -46,7 +46,7 @@ func (bt *BTree) Get(key []byte) []byte { } func (bt *BTree) Delete(key []byte) { - bt.tree.Delete(newKey(key)) + bt.tree.Delete(newItem(key, nil)) } func (bt *BTree) Iterator(start, end []byte) (*memIterator, error) { @@ -74,11 +74,7 @@ func byKeys(a, b item) bool { return bytes.Compare(a.key, b.key) == -1 } -// newPair creates a new pair item. -func newPair(key, value []byte) item { +// newItem creates a new pair item. +func newItem(key, value []byte) item { return item{key: key, value: value} } - -func newKey(key []byte) item { - return item{key: key} -} diff --git a/store/cachekv/internal/btree_test.go b/store/cachekv/internal/btree_test.go index e31a7cdd2782..c957c01261c7 100644 --- a/store/cachekv/internal/btree_test.go +++ b/store/cachekv/internal/btree_test.go @@ -1,9 +1,9 @@ package internal import ( - "encoding/binary" "testing" + sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" ) @@ -193,11 +193,9 @@ func verifyIterator(t *testing.T, itr *memIterator, expected []int64, msg string } func int642Bytes(i int64) []byte { - buf := make([]byte, 8) - binary.BigEndian.PutUint64(buf, uint64(i)) - return buf + return sdk.Uint64ToBigEndian(uint64(i)) } func bytes2Int64(buf []byte) int64 { - return int64(binary.BigEndian.Uint64(buf)) + return int64(sdk.BigEndianToUint64(buf)) } diff --git a/store/cachekv/internal/memiterator.go b/store/cachekv/internal/memiterator.go index 458425af33cd..2bceb8bc77df 100644 --- a/store/cachekv/internal/memiterator.go +++ b/store/cachekv/internal/memiterator.go @@ -29,13 +29,13 @@ func NewMemIterator(start, end []byte, items *BTree, deleted map[string]struct{} var valid bool if ascending { if start != nil { - valid = iter.Seek(newKey(start)) + valid = iter.Seek(newItem(start, nil)) } else { valid = iter.First() } } else { if end != nil { - valid = iter.Seek(newKey(end)) + valid = iter.Seek(newItem(end, nil)) if !valid { valid = iter.Last() } else { From 09d3d788088e0ed56c929ba4ae5633ea048a539f Mon Sep 17 00:00:00 2001 From: yihuang Date: Thu, 17 Nov 2022 11:49:09 +0800 Subject: [PATCH 15/19] Update store/cachekv/internal/btree.go --- store/cachekv/internal/btree.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go index b563a0a207fe..142f754bbd38 100644 --- a/store/cachekv/internal/btree.go +++ b/store/cachekv/internal/btree.go @@ -63,7 +63,7 @@ func (bt *BTree) ReverseIterator(start, end []byte) (*memIterator, error) { return NewMemIterator(start, end, bt, make(map[string]struct{}), false), nil } -// item is a btree.Item with byte slices as keys and values +// item is a btree item with byte slices as keys and values type item struct { key []byte value []byte From 6b3409280f534619c702fb72b42e9695e0d50c7b Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 24 Nov 2022 10:46:08 +0800 Subject: [PATCH 16/19] enable btree lock and close iterator in unit tests --- store/cachekv/internal/btree.go | 2 +- store/cachekv/internal/btree_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go index 142f754bbd38..cb8b486b596a 100644 --- a/store/cachekv/internal/btree.go +++ b/store/cachekv/internal/btree.go @@ -29,7 +29,7 @@ func NewBTree() *BTree { return &BTree{tree: *btree.NewBTreeGOptions(byKeys, btree.Options{ Degree: bTreeDegree, // Contract: cachekv store must not be called concurrently - NoLocks: true, + NoLocks: false, })} } diff --git a/store/cachekv/internal/btree_test.go b/store/cachekv/internal/btree_test.go index c957c01261c7..f85a8bbaf109 100644 --- a/store/cachekv/internal/btree_test.go +++ b/store/cachekv/internal/btree_test.go @@ -190,6 +190,7 @@ func verifyIterator(t *testing.T, itr *memIterator, expected []int64, msg string i++ } require.Equal(t, i, len(expected), "expected to have fully iterated over all the elements in iter") + require.NoError(t, itr.Close()) } func int642Bytes(i int64) []byte { From 4811becfb91018ae9f4ad42fa1e680f58728c6b0 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 24 Nov 2022 10:46:35 +0800 Subject: [PATCH 17/19] close iterator in unit test --- store/cachekv/store_test.go | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/store/cachekv/store_test.go b/store/cachekv/store_test.go index f86522d5d41e..373033fb6384 100644 --- a/store/cachekv/store_test.go +++ b/store/cachekv/store_test.go @@ -120,6 +120,7 @@ func TestCacheKVIteratorBounds(t *testing.T) { i++ } require.Equal(t, nItems, i) + require.NoError(t, itr.Close()) // iterate over none itr = st.Iterator(bz("money"), nil) @@ -128,6 +129,7 @@ func TestCacheKVIteratorBounds(t *testing.T) { i++ } require.Equal(t, 0, i) + require.NoError(t, itr.Close()) // iterate over lower itr = st.Iterator(keyFmt(0), keyFmt(3)) @@ -139,6 +141,7 @@ func TestCacheKVIteratorBounds(t *testing.T) { i++ } require.Equal(t, 3, i) + require.NoError(t, itr.Close()) // iterate over upper itr = st.Iterator(keyFmt(2), keyFmt(4)) @@ -150,6 +153,7 @@ func TestCacheKVIteratorBounds(t *testing.T) { i++ } require.Equal(t, 4, i) + require.NoError(t, itr.Close()) } func TestCacheKVReverseIteratorBounds(t *testing.T) { @@ -171,6 +175,7 @@ func TestCacheKVReverseIteratorBounds(t *testing.T) { i++ } require.Equal(t, nItems, i) + require.NoError(t, itr.Close()) // iterate over none itr = st.ReverseIterator(bz("money"), nil) @@ -179,6 +184,7 @@ func TestCacheKVReverseIteratorBounds(t *testing.T) { i++ } require.Equal(t, 0, i) + require.NoError(t, itr.Close()) // iterate over lower end := 3 @@ -191,6 +197,7 @@ func TestCacheKVReverseIteratorBounds(t *testing.T) { require.Equal(t, valFmt(end-i), v) } require.Equal(t, 3, i) + require.NoError(t, itr.Close()) // iterate over upper end = 4 @@ -203,6 +210,7 @@ func TestCacheKVReverseIteratorBounds(t *testing.T) { require.Equal(t, valFmt(end-i), v) } require.Equal(t, 2, i) + require.NoError(t, itr.Close()) } func TestCacheKVMergeIteratorBasics(t *testing.T) { @@ -347,12 +355,16 @@ func TestCacheKVMergeIteratorChunks(t *testing.T) { func TestCacheKVMergeIteratorDomain(t *testing.T) { st := newCacheKVStore() - start, end := st.Iterator(nil, nil).Domain() + itr := st.Iterator(nil, nil) + start, end := itr.Domain() require.Equal(t, start, end) + require.NoError(t, itr.Close()) - start, end = st.Iterator(keyFmt(40), keyFmt(60)).Domain() + itr = st.Iterator(keyFmt(40), keyFmt(60)) + start, end = itr.Domain() require.Equal(t, keyFmt(40), start) require.Equal(t, keyFmt(60), end) + require.NoError(t, itr.Close()) start, end = st.ReverseIterator(keyFmt(0), keyFmt(80)).Domain() require.Equal(t, keyFmt(0), start) @@ -414,6 +426,7 @@ func TestNilEndIterator(t *testing.T) { } require.Equal(t, SIZE-tt.startIndex, j) + require.NoError(t, itr.Close()) }) } } @@ -500,6 +513,7 @@ func assertIterateDomain(t *testing.T, st types.KVStore, expectedN int) { i++ } require.Equal(t, expectedN, i) + require.NoError(t, itr.Close()) } func assertIterateDomainCheck(t *testing.T, st types.KVStore, mem dbm.DB, r []keyRange) { @@ -531,6 +545,8 @@ func assertIterateDomainCheck(t *testing.T, st types.KVStore, mem dbm.DB, r []ke require.False(t, itr.Valid()) require.False(t, itr2.Valid()) + require.NoError(t, itr.Close()) + require.NoError(t, itr2.Close()) } func assertIterateDomainCompare(t *testing.T, st types.KVStore, mem dbm.DB) { @@ -540,6 +556,8 @@ func assertIterateDomainCompare(t *testing.T, st types.KVStore, mem dbm.DB) { require.NoError(t, err) checkIterators(t, itr, itr2) checkIterators(t, itr2, itr) + require.NoError(t, itr.Close()) + require.NoError(t, itr2.Close()) } func checkIterators(t *testing.T, itr, itr2 types.Iterator) { From b64963af2f8f081960500f48612917fdb933ef50 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 30 Nov 2022 23:33:29 +0800 Subject: [PATCH 18/19] remove the btree lock for now --- store/cachekv/internal/btree.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go index cb8b486b596a..142f754bbd38 100644 --- a/store/cachekv/internal/btree.go +++ b/store/cachekv/internal/btree.go @@ -29,7 +29,7 @@ func NewBTree() *BTree { return &BTree{tree: *btree.NewBTreeGOptions(byKeys, btree.Options{ Degree: bTreeDegree, // Contract: cachekv store must not be called concurrently - NoLocks: false, + NoLocks: true, })} } From 9fea8b4abd1bedcb9f87473118cf3b445acc695b Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 15 Dec 2022 16:03:11 +0800 Subject: [PATCH 19/19] add deadlock unit test case --- store/cachekv/store_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/store/cachekv/store_test.go b/store/cachekv/store_test.go index 373033fb6384..3ef99fd6f144 100644 --- a/store/cachekv/store_test.go +++ b/store/cachekv/store_test.go @@ -431,6 +431,22 @@ func TestNilEndIterator(t *testing.T) { } } +// TestIteratorDeadlock demonstrate the deadlock issue in cache store. +func TestIteratorDeadlock(t *testing.T) { + mem := dbadapter.Store{DB: dbm.NewMemDB()} + store := cachekv.NewStore(mem) + // the channel buffer is 64 and received once, so put at least 66 elements. + for i := 0; i < 66; i++ { + store.Set([]byte(fmt.Sprintf("key%d", i)), []byte{1}) + } + it := store.Iterator(nil, nil) + defer it.Close() + store.Set([]byte("key20"), []byte{1}) + // it'll be blocked here with previous version, or enable lock on btree. + it2 := store.Iterator(nil, nil) + defer it2.Close() +} + //------------------------------------------------------------------------------------------- // do some random ops