From 5a6b8467b02809f244eb1f8e53c6208620e2426f Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Mon, 1 Mar 2021 01:55:44 -0800 Subject: [PATCH] Backport genesis speed ups to 0.41.x (#8722) From: #8719 Fixes: #7766 Co-authored-by: Emmanuel T Odeke Co-authored-by: Alessio Treglia Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> --- CHANGELOG.md | 8 +++- store/cachekv/store.go | 38 ++++++++++++++++-- x/bank/types/balance.go | 45 +++++++++++++++++---- x/bank/types/balance_test.go | 78 ++++++++++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 277f93aafeef..af9efd92ba8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,13 @@ 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 + +* [\#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 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 +}