## Description
Closes: #10072
Significantly speeds up all the GasKvStore and CacheKVStore operations. Now Get/Set for these stores no longer even appears on my Osmosis benchmarks, saving ~8% of those benchmark's times. (Only one of the internal methods for setCacheValue appear -- will try to separately get a PR for reducing those memory allocations if #10026 is merged)
Talked to @alexanderbez about this on Discord, and he seemed in agreement with the approach of removing telemetry from these stores.
This does technically change telemetry, but I don't know if this is / should be considered a breaking change?
---
### 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
- [ ] 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)
- [ ] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification - is there something I should be updating?
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed
### 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)
## Description
So we don't burn un-necessary CPU in case of dirty store/delete.
```
name old time/op new time/op delta
CacheKVStoreIterator500-8 23.3µs ± 2% 23.4µs ± 1% ~ (p=0.151 n=5+5)
CacheKVStoreIterator1000-8 47.0µs ± 2% 46.3µs ± 0% -1.50% (p=0.008 n=5+5)
CacheKVStoreIterator10000-8 462µs ± 3% 458µs ± 1% ~ (p=0.690 n=5+5)
CacheKVStoreIterator50000-8 2.63ms ± 5% 2.51ms ± 2% -4.63% (p=0.032 n=5+5)
CacheKVStoreIterator100000-8 8.09ms ±21% 6.98ms ± 4% ~ (p=0.151 n=5+5)
CacheKVStoreGetNoKeyFound-8 393ns ± 2% 397ns ± 2% ~ (p=0.421 n=5+5)
CacheKVStoreGetKeyFound-8 269ns ± 5% 268ns ± 3% ~ (p=1.000 n=5+5)
```
Fixes#9991
---
### 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
- [ ] 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)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed
### 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)
* StoreKVPair protobuf message definition and generated go types
* store WriteListener
* update MultiStore, CacheWrap, CacheWrapper interfaces
* adjust KVStores to fit new CacheWrapper interface
* new ListenKVStore
* adjust multistores to fit new MultiStore interface and enable wrapping returned KVStores with the new ListenKVStore
* typo fixes in adr
* ListenKV Store test
* update server mock KVStore and MultiStore
* multistore unit test; fix multistore constructor
* update changelog
* fix bug identified in CI
* improve codecov, minor fixes/adjustments
* review fixes
* review updates; flip set to delete in KVStorePair, updated proto-docs from running 'make proto-gen'
Reduces CPU burn by using a typed List to avoid the expensive type
assertions from using an interface. This is the only option for now
until Go makes generics generally available.
The change brings time spent on the time assertion cummulatively to:
580ms down from 6.88s
Explanation:
The type assertions were showing up heavily in profiles:
* Before this commit
```shell
Total: 42.18s
ROUTINE ======================== github.com/cosmos/cosmos-sdk/store/cachekv.newMemIterator
in /Users/emmanuelodeke/go/src/github.com/cosmos/cosmos-sdk/store/cachekv/memiterator.go
14.01s 18.87s (flat, cum) 44.74% of Total
. . 17: items []*kv.Pair
. . 18: ascending bool
. . 19:}
. . 20:
. . 21:func newMemIterator(start, end []byte, items *list.List, ascending bool) *memIterator {
. 620ms 22: itemsInDomain := make([]*kv.Pair, 0, items.Len())
. . 23:
. . 24: var entered bool
. . 25:
510ms 870ms 26: for e := items.Front(); e != nil; e = e.Next() {
6.85s 6.88s 27: item := e.Value.(*kv.Pair)
5.71s 8.19s 28: if !dbm.IsKeyInDomain(item.Key, start, end) {
120ms 120ms 29: if entered {
. . 30: break
. . 31: }
. . 32:
. . 33: continue
. . 34: }
. . 35:
820ms 980ms 36: itemsInDomain = append(itemsInDomain, item)
. . 37: entered = true
. . 38: }
. . 39:
. 1.21s 40: return &memIterator{
. . 41: start: start,
. . 42: end: end,
. . 43: items: itemsInDomain,
. . 44: ascending: ascending,
. . 45: }
```
and given that the list only uses that type, it is only right to lift the
code from container/list.List, and only modify Element.Value's type.
For emphasis, the code is basically just a retrofit of
container/list/list.go but with a typing, and we'll keep it as is
until perhaps Go1.17 or Go1.18 or when everyone uses Go1.17+ after
generics have landed.
* After this commit
```shell
Total: 45.25s
ROUTINE ======================== github.com/cosmos/cosmos-sdk/store/cachekv.newMemIterator
in /Users/emmanuelodeke/go/src/github.com/cosmos/cosmos-sdk/store/cachekv/memiterator.go
4.84s 6.77s (flat, cum) 14.96% of Total
. . 16: items []*kv.Pair
. . 17: ascending bool
. . 18:}
. . 19:
. . 20:func newMemIterator(start, end []byte, items *kv.List, ascending bool) *memIterator {
. 330ms 21: itemsInDomain := make([]*kv.Pair, 0, items.Len())
. . 22:
. . 23: var entered bool
. . 24:
60ms 160ms 25: for e := items.Front(); e != nil; e = e.Next() {
580ms 580ms 26: item := e.Value
3.68s 4.78s 27: if !dbm.IsKeyInDomain(item.Key, start, end) {
80ms 80ms 28: if entered {
. . 29: break
. . 30: }
. . 31:
. . 32: continue
. . 33: }
. . 34:
440ms 580ms 35: itemsInDomain = append(itemsInDomain, item)
. . 36: entered = true
. . 37: }
. . 38:
. 260ms 39: return &memIterator{
. . 40: start: start,
. . 41: end: end,
. . 42: items: itemsInDomain,
. . 43: ascending: ascending,
. . 44: }
```
Fixes#8810
After continuously profiling InitGensis with 100K accounts, it showed
pathologically slow code, that was the result of a couple of patterns:
* Unconditional and not always necessary map lookups
* O(n^2) sdk.AccAddressFromBech32 retrievals when the code is expensive,
during a quicksort
The remedy involved 4 parts:
* O(n) sdk.AccAddressFromBech32 invocations, down from O(n^2) in the quicksort
* Only doing map lookups when the domain key check has passed
* Using a black magic compiler technique of the map clearing idiom
* Zero allocation []byte<->string conversion
With 100K accounts, this brings InitGenesis down to ~6min, instead of
20+min, it reduces the sort code from ~7sec down to 50ms.
Also some simple benchmark reflect the change:
```shell
name old time/op new time/op delta
SanitizeBalances500-8 19.3ms ±10% 1.5ms ± 5% -92.46% (p=0.000 n=20+20)
SanitizeBalances1000-8 41.9ms ± 8% 3.0ms ±12% -92.92% (p=0.000 n=20+20)
name old alloc/op new alloc/op delta
SanitizeBalances500-8 9.05MB ± 6% 0.56MB ± 0% -93.76% (p=0.000 n=20+18)
SanitizeBalances1000-8 20.2MB ± 3% 1.1MB ± 0% -94.37% (p=0.000 n=20+19)
name old allocs/op new allocs/op delta
SanitizeBalances500-8 72.4k ± 6% 4.5k ± 0% -93.76% (p=0.000 n=20+20)
SanitizeBalances1000-8 162k ± 3% 9k ± 0% -94.40% (p=0.000 n=20+20)
```
The CPU profiles show the radical change as per
https://github.com/cosmos/cosmos-sdk/issues/7766#issuecomment-786671734
Later on, we shall do more profiling and fixes but for now this brings
down the run-time for InitGenesis.
Fixes#7766
Co-authored-by: Alessio Treglia <alessio@tendermint.com>
With this change, we'll get details on the number of
allocations performed by code. Later on when we have
continuous benchmarking infrastructure, this change
will prove useful to flag regressions.
Fixes#8459
Co-authored-by: Alessio Treglia <alessio@tendermint.com>
* enable the wsl linter
Fix various wsl-related warnings.
x/ibc/04-channel/keeper/handshake.go: fix missing return statement in ChanOpenTry().
* goimports -w files
* remove unknown linter references
* run make format
* Revert "run make format"
This reverts commit f810b62b9e4993f08506663d4e5f2ec2228a9863.
* run make format
* lint: various linting fixs
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* more linting
* more linting fixes
* more errchecking
* comment out errcheck for now
* undo error check
* address some comments
* remore require error
* change delete to batch delete
Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
* Bump Tendermint version to v0.33.0
* Deprecate old cmn package with new packages
* Update update DB APIs
* More DB updates
* Bump IAVL to v0.13.0
* Handle error returned by iavl.NewMutableTree
* Fix some IAVL stuffs
* Update IAVL
* More updates
* Passing tests
* Fix unit tests
Co-authored-by: Jack Zampolin <jack.zampolin@gmail.com>
Dev Ojha
Cuong Manh Le
Ian Norden
Emmanuel T Odeke
Marko
Robert Zaremba
Aaron Craelius
billy rennekamp
Adam Bozanich
Alexander Bezobchuk
Alessio Treglia
Joon