Olympus Mons Cherry-Picks (#2)

* IAVL * perf: store/cachekv: Correct the naming and implementation for existing benchmarks (cosmos#10116) ## Description Cref discussion in cosmos#10026, updates the CacheKV Benchmark naming and implementation to correspond to whats actually going on, and remove many irrelevant/incorrect components from being in the benchmarks timing. Basically the old Benchmark's iterator creation was very flawed, and never hit the complex case, only repeatedly performing the best case performance of the iterator. Instead it really benchmarked iterator set and next operations. This PR splits out the benchmarks for those two accordingly. --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [x] added a changelog entry to `CHANGELOG.md` - N/A imo - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed - lint failure unrelated ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) * store cosmos#10486 * supply cosmos#10393 * supply cosmos#10393 * telemetry cosmos#10334 * module account Co-authored-by: Marko <[email protected]> Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Emmanuel T Odeke <[email protected]> Co-authored-by: likhita-809 <[email protected]>
tharsis · Nov 15, 2021 · 8692c10 · 8692c10
1 parent e443fb7
commit 8692c10
Show file tree

Hide file tree

Showing 13 changed files with 169 additions and 265 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -37,9 +37,13 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
-### Bug Fixes
+### Improvements
 
-* [\#10414](https://github.com/cosmos/cosmos-sdk/pull/10414) Use `sdk.GetConfig().GetFullBIP44Path()` instead `sdk.FullFundraiserPath` to generate key
+* [\#10077](https://github.com/cosmos/cosmos-sdk/pull/10077),[\#10334](https://github.com/cosmos/cosmos-sdk/pull/10334) Remove telemetry on `GasKV` and `CacheKV` store operations, significantly improving their performance.
+* [\#10486](https://github.com/cosmos/cosmos-sdk/pull/10486) store/cachekv's `Store.Write` conservatively looks up keys, but also uses the [map clearing idiom](https://bencher.orijtech.com/perfclinic/mapclearing/) to reduce the RAM usage, CPU time usage, and garbage collection pressure from clearing maps, instead of allocating new maps.
+* [\#10393](https://github.com/cosmos/cosmos-sdk/pull/"10393") Add `HasSupply` method to bank keeper to ensure that input denom actually exists on chain.
+
+### Bug Fixes
 
 ## [v0.44.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.44.3) - 2021-10-21
 

diff --git a/docs/core/telemetry.md b/docs/core/telemetry.md
@@ -131,14 +131,6 @@ The following examples expose too much cardinality and may not even prove to be
 | `store_iavl_delete`             | Duration of an IAVL `Store#Delete` call                                                   | ms              | summary |
 | `store_iavl_commit`             | Duration of an IAVL `Store#Commit` call                                                   | ms              | summary |
 | `store_iavl_query`              | Duration of an IAVL `Store#Query` call                                                    | ms              | summary |
-| `store_gaskv_get`               | Duration of a GasKV `Store#Get` call                                                      | ms              | summary |
-| `store_gaskv_set`               | Duration of a GasKV `Store#Set` call                                                      | ms              | summary |
-| `store_gaskv_has`               | Duration of a GasKV `Store#Has` call                                                      | ms              | summary |
-| `store_gaskv_delete`            | Duration of a GasKV `Store#Delete` call                                                   | ms              | summary |
-| `store_cachekv_get`             | Duration of a CacheKV `Store#Get` call                                                    | ms              | summary |
-| `store_cachekv_set`             | Duration of a CacheKV `Store#Set` call                                                    | ms              | summary |
-| `store_cachekv_write`           | Duration of a CacheKV `Store#Write` call                                                  | ms              | summary |
-| `store_cachekv_delete`          | Duration of a CacheKV `Store#Delete` call                                                 | ms              | summary |
 
 ## Next {hide}
 

diff --git a/docs/ru/readme.md b/docs/ru/readme.md
@@ -1,11 +1,3 @@
----
-parent:
-  order: false
----
+# Cosmos SDK Documentation (Russian)
 
-# RU
-
-::: warning
-**DEPRECATED**
-This documentation is not complete and it's outdated. Please use the English version.
-:::
+A Russian translation of the Cosmos SDK documentation is not available for this version. If you would like to help with translating, please see [Internationalization](https://github.com/cosmos/cosmos-sdk/blob/master/docs/DOCS_README.md#internationalization). A `v0.39` version of the documentation can be found [here](https://github.com/cosmos/cosmos-sdk/tree/v0.39.3/docs/ru).
diff --git a/go.mod b/go.mod
@@ -11,7 +11,7 @@ require (
 	github.com/coinbase/rosetta-sdk-go v0.6.10
 	github.com/confio/ics23/go v0.6.6
 	github.com/cosmos/go-bip39 v1.0.0
-	github.com/cosmos/iavl v0.17.1
+	github.com/cosmos/iavl v0.17.2
 	github.com/cosmos/ledger-cosmos-go v0.11.1
 	github.com/enigmampc/btcutil v1.0.3-0.20200723161021-e2fb6adb2a25
 	github.com/gogo/gateway v1.1.0
@@ -49,9 +49,9 @@ require (
 	github.com/tendermint/go-amino v0.16.0
 	github.com/tendermint/tendermint v0.34.14
 	github.com/tendermint/tm-db v0.6.4
-	golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a
-	google.golang.org/genproto v0.0.0-20210602131652-f16073e35f0c
-	google.golang.org/grpc v1.40.0
+	golang.org/x/crypto v0.0.0-20210817164053-32db794688a5
+	google.golang.org/genproto v0.0.0-20210828152312-66f60bf46e71
+	google.golang.org/grpc v1.42.0
 	google.golang.org/protobuf v1.27.1
 	gopkg.in/yaml.v2 v2.4.0
 )

diff --git a/go.sum b/go.sum
diff --git a/store/cachekv/bench_helper_test.go b/store/cachekv/bench_helper_test.go
@@ -0,0 +1,44 @@
+package cachekv_test
+
+import "crypto/rand"
+
+func randSlice(sliceSize int) []byte {
+	bz := make([]byte, sliceSize)
+	_, _ = rand.Read(bz)
+	return bz
+}
+
+func incrementByteSlice(bz []byte) {
+	for index := len(bz) - 1; index >= 0; index-- {
+		if bz[index] < 255 {
+			bz[index]++
+			break
+		} else {
+			bz[index] = 0
+		}
+	}
+}
+
+// Generate many keys starting at startKey, and are in sequential order
+func generateSequentialKeys(startKey []byte, numKeys int) [][]byte {
+	toReturn := make([][]byte, 0, numKeys)
+	cur := make([]byte, len(startKey))
+	copy(cur, startKey)
+	for i := 0; i < numKeys; i++ {
+		newKey := make([]byte, len(startKey))
+		copy(newKey, cur)
+		toReturn = append(toReturn, newKey)
+		incrementByteSlice(cur)
+	}
+	return toReturn
+}
+
+// Generate many random, unsorted keys
+func generateRandomKeys(keySize int, numKeys int) [][]byte {
+	toReturn := make([][]byte, 0, numKeys)
+	for i := 0; i < numKeys; i++ {
+		newKey := randSlice(keySize)
+		toReturn = append(toReturn, newKey)
+	}
+	return toReturn
+}
diff --git a/store/cachekv/store.go b/store/cachekv/store.go
@@ -5,15 +5,13 @@ import (
 	"io"
 	"sort"
 	"sync"
-	"time"
 
 	dbm "github.com/tendermint/tm-db"
 
 	"github.com/cosmos/cosmos-sdk/internal/conv"
 	"github.com/cosmos/cosmos-sdk/store/listenkv"
 	"github.com/cosmos/cosmos-sdk/store/tracekv"
 	"github.com/cosmos/cosmos-sdk/store/types"
-	"github.com/cosmos/cosmos-sdk/telemetry"
 	"github.com/cosmos/cosmos-sdk/types/kv"
 )
 
@@ -91,7 +89,6 @@ func (store *Store) Has(key []byte) bool {
 func (store *Store) Delete(key []byte) {
 	store.mtx.Lock()
 	defer store.mtx.Unlock()
-	defer telemetry.MeasureSince(time.Now(), "store", "cachekv", "delete")
 
 	types.AssertValidKey(key)
 	store.setCacheValue(key, nil, true, true)
@@ -101,7 +98,6 @@ func (store *Store) Delete(key []byte) {
 func (store *Store) Write() {
 	store.mtx.Lock()
 	defer store.mtx.Unlock()
-	defer telemetry.MeasureSince(time.Now(), "store", "cachekv", "write")
 
 	// We need a copy of all of the keys.
 	// Not the best, but probably not a bottleneck depending.
@@ -118,22 +114,34 @@ func (store *Store) Write() {
 	// TODO: Consider allowing usage of Batch, which would allow the write to
 	// at least happen atomically.
 	for _, key := range keys {
-		cacheValue := store.cache[key]
-
-		switch {
-		case store.isDeleted(key):
+		if store.isDeleted(key) {
+			// We use []byte(key) instead of conv.UnsafeStrToBytes because we cannot
+			// be sure if the underlying store might do a save with the byteslice or
+			// not. Once we get confirmation that .Delete is guaranteed not to
+			// save the byteslice, then we can assume only a read-only copy is sufficient.
 			store.parent.Delete([]byte(key))
-		case cacheValue.value == nil:
-			// Skip, it already doesn't exist in parent.
-		default:
+			continue
+		}
+
+		cacheValue := store.cache[key]
+		if cacheValue.value != nil {
+			// It already exists in the parent, hence delete it.
 			store.parent.Set([]byte(key), cacheValue.value)
 		}
 	}
 
-	// Clear the cache
-	store.cache = make(map[string]*cValue)
-	store.deleted = make(map[string]struct{})
-	store.unsortedCache = make(map[string]struct{})
+	// Clear the cache using the map clearing idiom
+	// and not allocating fresh objects.
+	// Please see https://bencher.orijtech.com/perfclinic/mapclearing/
+	for key := range store.cache {
+		delete(store.cache, key)
+	}
+	for key := range store.deleted {
+		delete(store.deleted, key)
+	}
+	for key := range store.unsortedCache {
+		delete(store.unsortedCache, key)
+	}
 	store.sortedCache = dbm.NewMemDB()
 }
 

diff --git a/store/cachekv/store_bench_test.go b/store/cachekv/store_bench_test.go
@@ -1,8 +1,6 @@
 package cachekv_test
 
 import (
-	"crypto/rand"
-	"sort"
 	"testing"
 
 	dbm "github.com/tendermint/tm-db"
@@ -11,37 +9,85 @@ import (
 	"github.com/cosmos/cosmos-sdk/store/dbadapter"
 )
 
-func benchmarkCacheKVStoreIterator(numKVs int, b *testing.B) {
+var sink interface{}
+
+const defaultValueSizeBz = 1 << 12
+
+// This benchmark measures the time of iterator.Next() when the parent store is blank
+func benchmarkBlankParentIteratorNext(b *testing.B, keysize int) {
+	mem := dbadapter.Store{DB: dbm.NewMemDB()}
+	kvstore := cachekv.NewStore(mem)
+	// Use a singleton for value, to not waste time computing it
+	value := randSlice(defaultValueSizeBz)
+	// Use simple values for keys, pick a random start,
+	// and take next b.N keys sequentially after.]
+	startKey := randSlice(32)
+
+	// Add 1 to avoid issues when b.N = 1
+	keys := generateSequentialKeys(startKey, b.N+1)
+	for _, k := range keys {
+		kvstore.Set(k, value)
+	}
+
 	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
+	}
+}
+
+// Benchmark setting New keys to a store, where the new keys are in sequence.
+func benchmarkBlankParentAppend(b *testing.B, keysize int) {
 	mem := dbadapter.Store{DB: dbm.NewMemDB()}
-	cstore := cachekv.NewStore(mem)
-	keys := make([]string, numKVs)
+	kvstore := cachekv.NewStore(mem)
 
-	for i := 0; i < numKVs; i++ {
-		key := make([]byte, 32)
-		value := make([]byte, 32)
+	// 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 b.N keys sequentially after.
+	startKey := randSlice(32)
 
-		_, _ = rand.Read(key)
-		_, _ = rand.Read(value)
+	keys := generateSequentialKeys(startKey, b.N)
 
-		keys[i] = string(key)
-		cstore.Set(key, value)
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	for _, k := range keys {
+		kvstore.Set(k, value)
 	}
+}
 
-	sort.Strings(keys)
+// Benchmark setting New keys to a store, where the new keys are random.
+// the speed of this function does not depend on the values in the parent store
+func benchmarkRandomSet(b *testing.B, keysize int) {
+	mem := dbadapter.Store{DB: dbm.NewMemDB()}
+	kvstore := cachekv.NewStore(mem)
 
-	for n := 0; n < b.N; n++ {
-		iter := cstore.Iterator([]byte(keys[0]), []byte(keys[numKVs-1]))
+	// Use a singleton for value, to not waste time computing it
+	value := randSlice(defaultValueSizeBz)
+	keys := generateRandomKeys(keysize, b.N)
 
-		for _ = iter.Key(); iter.Valid(); iter.Next() {
-		}
+	b.ReportAllocs()
+	b.ResetTimer()
 
-		iter.Close()
+	for _, k := range keys {
+		kvstore.Set(k, value)
 	}
 }
 
-func BenchmarkCacheKVStoreIterator500(b *testing.B)    { benchmarkCacheKVStoreIterator(500, b) }
-func BenchmarkCacheKVStoreIterator1000(b *testing.B)   { benchmarkCacheKVStoreIterator(1000, b) }
-func BenchmarkCacheKVStoreIterator10000(b *testing.B)  { benchmarkCacheKVStoreIterator(10000, b) }
-func BenchmarkCacheKVStoreIterator50000(b *testing.B)  { benchmarkCacheKVStoreIterator(50000, b) }
-func BenchmarkCacheKVStoreIterator100000(b *testing.B) { benchmarkCacheKVStoreIterator(100000, b) }
+func BenchmarkBlankParentIteratorNextKeySize32(b *testing.B) {
+	benchmarkBlankParentIteratorNext(b, 32)
+}
+
+func BenchmarkBlankParentAppendKeySize32(b *testing.B) {
+	benchmarkBlankParentAppend(b, 32)
+}
+
+func BenchmarkSetKeySize32(b *testing.B) {
+	benchmarkRandomSet(b, 32)
+}
diff --git a/store/gaskv/store.go b/store/gaskv/store.go
@@ -2,10 +2,8 @@ package gaskv
 
 import (
 	"io"
-	"time"
 
 	"github.com/cosmos/cosmos-sdk/store/types"
-	"github.com/cosmos/cosmos-sdk/telemetry"
 )
 
 var _ types.KVStore = &Store{}
@@ -56,14 +54,12 @@ func (gs *Store) Set(key []byte, value []byte) {
 
 // Implements KVStore.
 func (gs *Store) Has(key []byte) bool {
-	defer telemetry.MeasureSince(time.Now(), "store", "gaskv", "has")
 	gs.gasMeter.ConsumeGas(gs.gasConfig.HasCost, types.GasHasDesc)
 	return gs.parent.Has(key)
 }
 
 // Implements KVStore.
 func (gs *Store) Delete(key []byte) {
-	defer telemetry.MeasureSince(time.Now(), "store", "gaskv", "delete")
 	// charge gas to prevent certain attack vectors even though space is being freed
 	gs.gasMeter.ConsumeGas(gs.gasConfig.DeleteCost, types.GasDeleteDesc)
 	gs.parent.Delete(key)