From 92b248bdfc3d8eac549a80357c3e48fd1c0564a0 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Sat, 27 Feb 2021 07:26:22 -0800 Subject: [PATCH 1/3] store/cachekv, x/bank/types: algorithmically fix pathologically slow code (#8719) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 9 ++++- store/cachekv/store.go | 38 ++++++++++++++++-- x/bank/types/balance.go | 45 +++++++++++++++++---- x/bank/types/balance_test.go | 78 ++++++++++++++++++++++++++++++++++++ 4 files changed, 159 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 277f93aafeef..21dab9b6a65e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,10 +36,17 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] -**IMPORTANT:** Due to a bug in the `v0.41.x` series with how evidence handles validator consensus addresses [\#8461](https://github.com/cosmos/cosmos-sdk/issues/8461), SDK based chains that are not using the de$ +**IMPORTANT**: Due to a bug in the v0.41.x series with how evidence handles validator consensus addresses #8461, SDK based chains that are not using the default bech32 prefix (cosmos, aka all chains except for the Cosmos Hub) should not use this release or any release in the v0.41.x series. Please see #8668 for tracking & timeline for the v0.42.0 release, which will include a fix for this issue. + + +###Improvements + +* (store/cachekv), (x/bank/types) #8719 algorithmically fix pathologically slow code + ## [v0.41.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.41.3) - 2021-02-18 + ### Bug Fixes * [\#8617](https://github.com/cosmos/cosmos-sdk/pull/8617) Fix build failures caused by a small API breakage introduced in tendermint v0.34.7. diff --git a/store/cachekv/store.go b/store/cachekv/store.go index b2e394e95f3a..42c94370b8af 100644 --- a/store/cachekv/store.go +++ b/store/cachekv/store.go @@ -4,9 +4,11 @@ import ( "bytes" "container/list" "io" + "reflect" "sort" "sync" "time" + "unsafe" dbm "github.com/tendermint/tm-db" @@ -177,18 +179,48 @@ func (store *Store) iterator(start, end []byte, ascending bool) types.Iterator { return newCacheMergeIterator(parent, cache, ascending) } +// strToByte is meant to make a zero allocation conversion +// from string -> []byte to speed up operations, it is not meant +// to be used generally, but for a specific pattern to check for available +// keys within a domain. +func strToByte(s string) []byte { + var b []byte + hdr := (*reflect.SliceHeader)(unsafe.Pointer(&b)) + hdr.Cap = len(s) + hdr.Len = len(s) + hdr.Data = (*reflect.StringHeader)(unsafe.Pointer(&s)).Data + return b +} + +// byteSliceToStr is meant to make a zero allocation conversion +// from []byte -> string to speed up operations, it is not meant +// to be used generally, but for a specific pattern to delete keys +// from a map. +func byteSliceToStr(b []byte) string { + hdr := (*reflect.StringHeader)(unsafe.Pointer(&b)) + return *(*string)(unsafe.Pointer(hdr)) +} + // Constructs a slice of dirty items, to use w/ memIterator. func (store *Store) dirtyItems(start, end []byte) { unsorted := make([]*kv.Pair, 0) + n := len(store.unsortedCache) for key := range store.unsortedCache { - cacheValue := store.cache[key] - - if dbm.IsKeyInDomain([]byte(key), start, end) { + if dbm.IsKeyInDomain(strToByte(key), start, end) { + cacheValue := store.cache[key] unsorted = append(unsorted, &kv.Pair{Key: []byte(key), Value: cacheValue.value}) + } + } + if len(unsorted) == n { // This pattern allows the Go compiler to emit the map clearing idiom for the entire map. + for key := range store.unsortedCache { delete(store.unsortedCache, key) } + } else { // Otherwise, normally delete the unsorted keys from the map. + for _, kv := range unsorted { + delete(store.unsortedCache, byteSliceToStr(kv.Key)) + } } sort.Slice(unsorted, func(i, j int) bool { diff --git a/x/bank/types/balance.go b/x/bank/types/balance.go index 6150679023b8..7c017ee90051 100644 --- a/x/bank/types/balance.go +++ b/x/bank/types/balance.go @@ -62,16 +62,47 @@ func (b Balance) Validate() error { // SanitizeGenesisBalances sorts addresses and coin sets. func SanitizeGenesisBalances(balances []Balance) []Balance { - sort.Slice(balances, func(i, j int) bool { - addr1, _ := sdk.AccAddressFromBech32(balances[i].Address) - addr2, _ := sdk.AccAddressFromBech32(balances[j].Address) - return bytes.Compare(addr1.Bytes(), addr2.Bytes()) < 0 - }) - + // Given that this function sorts balances, using the standard library's + // Quicksort based algorithms, we have algorithmic complexities of: + // * Best case: O(nlogn) + // * Worst case: O(n^2) + // The comparator used MUST be cheap to use lest we incur expenses like we had + // before whereby sdk.AccAddressFromBech32, which is a very expensive operation + // compared n * n elements yet discarded computations each time, as per: + // https://github.com/cosmos/cosmos-sdk/issues/7766#issuecomment-786671734 + // with this change the first step is to extract out and singly produce the values + // that'll be used for comparisons and keep them cheap and fast. + + // 1. Retrieve the byte equivalents for each Balance's address and maintain a mapping of + // its Balance, as the mapper will be used in sorting. + type addrToBalance struct { + // We use a pointer here to avoid averse effects of value copying + // wasting RAM all around. + balance *Balance + accAddr []byte + } + adL := make([]*addrToBalance, 0, len(balances)) for _, balance := range balances { - balance.Coins = balance.Coins.Sort() + balance := balance + addr, _ := sdk.AccAddressFromBech32(balance.Address) + adL = append(adL, &addrToBalance{ + balance: &balance, + accAddr: []byte(addr), + }) } + // 2. Sort with the cheap mapping, using the mapper's + // already accAddr bytes values which is a cheaper operation. + sort.Slice(adL, func(i, j int) bool { + ai, aj := adL[i], adL[j] + return bytes.Compare(ai.accAddr, aj.accAddr) < 0 + }) + + // 3. To complete the sorting, we have to now just insert + // back the balances in the order returned by the sort. + for i, ad := range adL { + balances[i] = *ad.balance + } return balances } diff --git a/x/bank/types/balance_test.go b/x/bank/types/balance_test.go index 3987f5189ef0..9736c847baa0 100644 --- a/x/bank/types/balance_test.go +++ b/x/bank/types/balance_test.go @@ -1,10 +1,12 @@ package types_test import ( + "bytes" "testing" "github.com/stretchr/testify/require" + "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" sdk "github.com/cosmos/cosmos-sdk/types" bank "github.com/cosmos/cosmos-sdk/x/bank/types" ) @@ -101,3 +103,79 @@ func TestBalance_GetAddress(t *testing.T) { }) } } + +func TestSanitizeBalances(t *testing.T) { + // 1. Generate balances + tokens := sdk.TokensFromConsensusPower(81) + coin := sdk.NewCoin("benchcoin", tokens) + coins := sdk.Coins{coin} + addrs, _ := makeRandomAddressesAndPublicKeys(20) + + var balances []bank.Balance + for _, addr := range addrs { + balances = append(balances, bank.Balance{ + Address: addr.String(), + Coins: coins, + }) + } + // 2. Sort the values. + sorted := bank.SanitizeGenesisBalances(balances) + + // 3. Compare and ensure that all the values are sorted in ascending order. + // Invariant after sorting: + // a[i] <= a[i+1...n] + for i := 0; i < len(sorted); i++ { + ai := sorted[i] + // Ensure that every single value that comes after i is less than it. + for j := i + 1; j < len(sorted); j++ { + aj := sorted[j] + + if got := bytes.Compare(ai.GetAddress(), aj.GetAddress()); got > 0 { + t.Errorf("Balance(%d) > Balance(%d)", i, j) + } + } + } +} + +func makeRandomAddressesAndPublicKeys(n int) (accL []sdk.AccAddress, pkL []*ed25519.PubKey) { + for i := 0; i < n; i++ { + pk := ed25519.GenPrivKey().PubKey().(*ed25519.PubKey) + pkL = append(pkL, pk) + accL = append(accL, sdk.AccAddress(pk.Address())) + } + return accL, pkL +} + +var sink, revert interface{} + +func BenchmarkSanitizeBalances500(b *testing.B) { + benchmarkSanitizeBalances(b, 500) +} + +func BenchmarkSanitizeBalances1000(b *testing.B) { + benchmarkSanitizeBalances(b, 1000) +} + +func benchmarkSanitizeBalances(b *testing.B, nAddresses int) { + b.ReportAllocs() + tokens := sdk.TokensFromConsensusPower(81) + coin := sdk.NewCoin("benchcoin", tokens) + coins := sdk.Coins{coin} + addrs, _ := makeRandomAddressesAndPublicKeys(nAddresses) + + b.ResetTimer() + for i := 0; i < b.N; i++ { + var balances []bank.Balance + for _, addr := range addrs { + balances = append(balances, bank.Balance{ + Address: addr.String(), + Coins: coins, + }) + } + sink = bank.SanitizeGenesisBalances(balances) + } + if sink == nil { + b.Fatal("Benchmark did not run") + } + sink = revert +} From 044992b1cf76440a683e7d7e857cd26c4e1e3d45 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Sat, 27 Feb 2021 22:43:59 +0000 Subject: [PATCH 2/3] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21dab9b6a65e..61d4ed1633af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ###Improvements -* (store/cachekv), (x/bank/types) #8719 algorithmically fix pathologically slow code +* [\#8719](https://github.com/cosmos/cosmos-sdk/pull/8719) (store/cachekv), (x/bank/types) algorithmically fix pathologically slow code ## [v0.41.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.41.3) - 2021-02-18 From adf3b1a11f55996ae8bce9319c1d293cb31777dc Mon Sep 17 00:00:00 2001 From: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Date: Sun, 28 Feb 2021 09:43:39 -0300 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61d4ed1633af..af9efd92ba8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,7 +46,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [v0.41.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.41.3) - 2021-02-18 - ### Bug Fixes * [\#8617](https://github.com/cosmos/cosmos-sdk/pull/8617) Fix build failures caused by a small API breakage introduced in tendermint v0.34.7.