From 86887a9af49f8c78a1ff7b98f233262d934334e2 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Sun, 12 Dec 2021 01:45:00 -0400 Subject: [PATCH 1/5] Add benchmark for iterators with many deleted items in cache On 1M deletes, an iterator creation took: BenchmarkIteratorOnParentWith1MDeletes-16 1 1112540940 ns/op --- store/cachekv/store_bench_test.go | 46 +++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/store/cachekv/store_bench_test.go b/store/cachekv/store_bench_test.go index 040c4a77c97c..2f75aed5377d 100644 --- a/store/cachekv/store_bench_test.go +++ b/store/cachekv/store_bench_test.go @@ -78,6 +78,48 @@ func benchmarkRandomSet(b *testing.B, keysize int) { for _, k := range keys { kvstore.Set(k, value) } + + iter := kvstore.Iterator(keys[0], keys[b.N]) + defer iter.Close() + + for _ = iter.Key(); iter.Valid(); iter.Next() { + // deadcode elimination stub + sink = iter + } +} + +// Benchmark creating an iterator on a parent with D entries, +// that are all deleted in the cacheKV store. +// We essentially are benchmarking the cacheKV iterator creation & iteration times +// with the number of entries deleted in the parent. +func benchmarkIteratorOnParentWithManyDeletes(b *testing.B, numDeletes int) { + mem := dbadapter.Store{DB: dbm.NewMemDB()} + + // Use a singleton for value, to not waste time computing it + value := randSlice(32) + // Use simple values for keys, pick a random start, + // and take next D keys sequentially after. + startKey := randSlice(32) + keys := generateSequentialKeys(startKey, numDeletes) + for _, k := range keys { + mem.Set(k, value) + } + kvstore := cachekv.NewStore(mem) + // Has to be 1: in this commit due to a bug in the CacheKVStore + for _, k := range keys[1:] { + kvstore.Delete(k) + } + + b.ReportAllocs() + b.ResetTimer() + + iter := kvstore.Iterator(keys[0], keys[b.N]) + defer iter.Close() + + for _ = iter.Key(); iter.Valid(); iter.Next() { + // deadcode elimination stub + sink = iter + } } func BenchmarkBlankParentIteratorNextKeySize32(b *testing.B) { @@ -91,3 +133,7 @@ func BenchmarkBlankParentAppendKeySize32(b *testing.B) { func BenchmarkSetKeySize32(b *testing.B) { benchmarkRandomSet(b, 32) } + +func BenchmarkIteratorOnParentWith1MDeletes(b *testing.B) { + benchmarkIteratorOnParentWithManyDeletes(b, 1_000_000) +} From 58184a45cd140c1aa20d7a6a9a940924b03bf066 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Thu, 9 Dec 2021 01:45:04 -0400 Subject: [PATCH 2/5] Remove duplication of deleted map --- store/cachekv/memiterator.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/store/cachekv/memiterator.go b/store/cachekv/memiterator.go index 0a4bc57a6406..756db7d26e5b 100644 --- a/store/cachekv/memiterator.go +++ b/store/cachekv/memiterator.go @@ -29,15 +29,10 @@ func newMemIterator(start, end []byte, items *dbm.MemDB, deleted map[string]stru panic(err) } - newDeleted := make(map[string]struct{}) - for k, v := range deleted { - newDeleted[k] = v - } - return &memIterator{ Iterator: iter, - deleted: newDeleted, + deleted: deleted, } } From e9d7d7845f30cf382c0ee6ad044f9f40abc7cf2a Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Thu, 9 Dec 2021 03:00:16 -0400 Subject: [PATCH 3/5] Fix the subtle memiterator bug added by last change, due to API design issues --- store/cachekv/memiterator.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/store/cachekv/memiterator.go b/store/cachekv/memiterator.go index 756db7d26e5b..04df40ff56aa 100644 --- a/store/cachekv/memiterator.go +++ b/store/cachekv/memiterator.go @@ -1,6 +1,8 @@ package cachekv import ( + "bytes" + dbm "github.com/tendermint/tm-db" "github.com/cosmos/cosmos-sdk/store/types" @@ -12,6 +14,7 @@ import ( type memIterator struct { types.Iterator + lastKey []byte deleted map[string]struct{} } @@ -32,14 +35,22 @@ func newMemIterator(start, end []byte, items *dbm.MemDB, deleted map[string]stru return &memIterator{ Iterator: iter, + lastKey: nil, deleted: deleted, } } func (mi *memIterator) Value() []byte { key := mi.Iterator.Key() - if _, ok := mi.deleted[string(key)]; ok { + // 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) + // 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 } + mi.lastKey = key return mi.Iterator.Value() } From 4fa86c6478e2667a58a8b4f57e1963a14095f7b7 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Sun, 12 Dec 2021 01:49:30 -0400 Subject: [PATCH 4/5] Correct comment --- store/cachekv/store_bench_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/cachekv/store_bench_test.go b/store/cachekv/store_bench_test.go index 2f75aed5377d..9a857abed9e5 100644 --- a/store/cachekv/store_bench_test.go +++ b/store/cachekv/store_bench_test.go @@ -105,7 +105,7 @@ func benchmarkIteratorOnParentWithManyDeletes(b *testing.B, numDeletes int) { mem.Set(k, value) } kvstore := cachekv.NewStore(mem) - // Has to be 1: in this commit due to a bug in the CacheKVStore + // Has to be 1: due to a bug in the CacheKVStore for _, k := range keys[1:] { kvstore.Delete(k) } From c14a9f088d2df959ffa1286a0f730eaedf47a235 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Sun, 12 Dec 2021 02:12:22 -0400 Subject: [PATCH 5/5] Add changelog & explain issue --- CHANGELOG.md | 1 + store/cachekv/store_bench_test.go | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bf2e26dac68..fd7058d72e04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -149,6 +149,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#10468](https://github.com/cosmos/cosmos-sdk/pull/10468) Allow futureOps to queue additional operations in simulations * [\#10625](https://github.com/cosmos/cosmos-sdk/pull/10625) Add `--fee-payer` CLI flag * (cli) [\#10683](https://github.com/cosmos/cosmos-sdk/pull/10683) In CLI, allow 1 SIGN_MODE_DIRECT signer in transactions with multiple signers. +* (store) [\#10741](https://github.com/cosmos/cosmos-sdk/pull/10741) Significantly speedup iterator creation after delete heavy workloads. Significantly improves IBC migration times. ### Bug Fixes diff --git a/store/cachekv/store_bench_test.go b/store/cachekv/store_bench_test.go index 9a857abed9e5..88c86eff564a 100644 --- a/store/cachekv/store_bench_test.go +++ b/store/cachekv/store_bench_test.go @@ -101,11 +101,16 @@ func benchmarkIteratorOnParentWithManyDeletes(b *testing.B, numDeletes int) { // and take next D keys sequentially after. startKey := randSlice(32) keys := generateSequentialKeys(startKey, numDeletes) + // setup parent db with D keys. for _, k := range keys { mem.Set(k, value) } kvstore := cachekv.NewStore(mem) - // Has to be 1: due to a bug in the CacheKVStore + // Delete all keys from the cache KV store. + // The keys[1:] is to keep at least one entry in parent, due to a bug in the SDK iterator design. + // Essentially the iterator will never be valid, in that it should never run. + // However, this is incompatible with the for loop structure the SDK uses, hence + // causes a panic. Thus we do keys[1:]. for _, k := range keys[1:] { kvstore.Delete(k) }