diff --git a/CHANGELOG.md b/CHANGELOG.md index b3aa1dc82b22..a46de7009880 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/bank) [\#8614](https://github.com/cosmos/cosmos-sdk/issues/8614) Add `Name` and `Symbol` fields to denom metadata * (x/auth) [\#8522](https://github.com/cosmos/cosmos-sdk/pull/8522) Allow to query all stored accounts * (crypto/types) [\#8600](https://github.com/cosmos/cosmos-sdk/pull/8600) `CompactBitArray`: optimize the `NumTrueBitsBefore` method and add an `Equal` method. +* (store/cachekv), (x/bank/types) [\#8719](https://github.com/cosmos/cosmos-sdk/pull/8719) algorithmically fix pathologically slow code 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 +}