-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: add cache to address codec #20122
base: main
Are you sure you want to change the base?
Changes from 3 commits
36ed3fe
23356b3
ac248de
274650b
45a6e11
7ffa4a7
de976c7
37bf7f5
469ac79
a608650
63e2eff
f5ccb72
726d5fc
8ca5ad1
9868c7f
9db0b77
5d5ab90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,23 +3,92 @@ package address | |||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||
"errors" | ||||||||||||||||||||||||||
"strings" | ||||||||||||||||||||||||||
"sync" | ||||||||||||||||||||||||||
"sync/atomic" | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
"github.com/hashicorp/golang-lru/simplelru" | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
"cosmossdk.io/core/address" | ||||||||||||||||||||||||||
errorsmod "cosmossdk.io/errors" | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
"github.com/cosmos/cosmos-sdk/internal/conv" | ||||||||||||||||||||||||||
sdkAddress "github.com/cosmos/cosmos-sdk/types/address" | ||||||||||||||||||||||||||
"github.com/cosmos/cosmos-sdk/types/bech32" | ||||||||||||||||||||||||||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const ( | ||||||||||||||||||||||||||
// TODO: ideally sdk.GetBech32PrefixValAddr("") should be used but currently there's a cyclical import. | ||||||||||||||||||||||||||
// Once globals are deleted the cyclical import won't happen. | ||||||||||||||||||||||||||
prefixValAddr = "valoper" | ||||||||||||||||||||||||||
prefixConsAddr = "valcons" | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// cache variables | ||||||||||||||||||||||||||
var ( | ||||||||||||||||||||||||||
accAddrMu sync.Mutex | ||||||||||||||||||||||||||
accAddrCache *simplelru.LRU | ||||||||||||||||||||||||||
consAddrMu sync.Mutex | ||||||||||||||||||||||||||
consAddrCache *simplelru.LRU | ||||||||||||||||||||||||||
valAddrMu sync.Mutex | ||||||||||||||||||||||||||
valAddrCache *simplelru.LRU | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
isCachingEnabled atomic.Bool | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
func init() { | ||||||||||||||||||||||||||
var err error | ||||||||||||||||||||||||||
isCachingEnabled.Store(true) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// in total the cache size is 61k entries. Key is 32 bytes and value is around 50-70 bytes. | ||||||||||||||||||||||||||
// That will make around 92 * 61k * 2 (LRU) bytes ~ 11 MB | ||||||||||||||||||||||||||
if accAddrCache, err = simplelru.NewLRU(60000, nil); err != nil { | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. personal preference: it would be great to have this configurable so people can trade CPU for memory or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I'm not sure what the best approach is to solve this, as LRUs are currently shared between codecs and defined as globals. |
||||||||||||||||||||||||||
panic(err) | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if consAddrCache, err = simplelru.NewLRU(500, nil); err != nil { | ||||||||||||||||||||||||||
panic(err) | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if valAddrCache, err = simplelru.NewLRU(500, nil); err != nil { | ||||||||||||||||||||||||||
panic(err) | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 do we need 3 different cache types? I was wondering if this is for sharding or pinning maybe? The LRU should keep the frequently used ones in cache anyway? WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bytes of the addresses are the same, but there are three possible outputs (AccAddress, ValAddress, ConsAddress). I see three options here:
I've chosen the first option to maintain consistency with how things were done before. Lines 100 to 111 in 8e60f3b
|
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
type Bech32Codec struct { | ||||||||||||||||||||||||||
Bech32Prefix string | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
var _ address.Codec = &Bech32Codec{} | ||||||||||||||||||||||||||
type cachedBech32Codec struct { | ||||||||||||||||||||||||||
codec Bech32Codec | ||||||||||||||||||||||||||
cache *simplelru.LRU | ||||||||||||||||||||||||||
mu *sync.Mutex | ||||||||||||||||||||||||||
JulianToledano marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
var ( | ||||||||||||||||||||||||||
_ address.Codec = &Bech32Codec{} | ||||||||||||||||||||||||||
_ address.Codec = &cachedBech32Codec{} | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
func NewBech32Codec(prefix string) address.Codec { | ||||||||||||||||||||||||||
return Bech32Codec{prefix} | ||||||||||||||||||||||||||
ac := Bech32Codec{prefix} | ||||||||||||||||||||||||||
if !isCachingEnabled.Load() { | ||||||||||||||||||||||||||
return ac | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
lru := accAddrCache | ||||||||||||||||||||||||||
mu := &accAddrMu | ||||||||||||||||||||||||||
if strings.Contains(prefix, prefixValAddr) { | ||||||||||||||||||||||||||
tac0turtle marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
lru = valAddrCache | ||||||||||||||||||||||||||
mu = &valAddrMu | ||||||||||||||||||||||||||
} else if strings.Contains(prefix, prefixConsAddr) { | ||||||||||||||||||||||||||
JulianToledano marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
lru = consAddrCache | ||||||||||||||||||||||||||
mu = &consAddrMu | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return cachedBech32Codec{ | ||||||||||||||||||||||||||
codec: ac, | ||||||||||||||||||||||||||
cache: lru, | ||||||||||||||||||||||||||
mu: mu, | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// StringToBytes encodes text to bytes | ||||||||||||||||||||||||||
|
@@ -61,3 +130,38 @@ func (bc Bech32Codec) BytesToString(bz []byte) (string, error) { | |||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return text, nil | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
func (cbc cachedBech32Codec) BytesToString(bz []byte) (string, error) { | ||||||||||||||||||||||||||
key := conv.UnsafeBytesToStr(bz) | ||||||||||||||||||||||||||
cbc.mu.Lock() | ||||||||||||||||||||||||||
defer cbc.mu.Unlock() | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if addr, ok := cbc.cache.Get(key); ok { | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is possible but in case the bytes match an existing bech32 address, it can create some trouble. Adding key prefixes may solve this or not sharing the same cache. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See first comment. Currently, there are three different caches, but adding key prefixes may be a solution if we prefer just one. |
||||||||||||||||||||||||||
return addr.(string), nil | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
addr, err := cbc.codec.BytesToString(bz) | ||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||
return "", err | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
cbc.cache.Add(key, addr) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return addr, nil | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
JulianToledano marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can there be more than 1 bech32 prefix per type? The cache is global and may return wrong strings for the byte representation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of the time its gonna be just one bech32 prefix. But there may be uses cases with more than one. For example someone who wants to develop a multi chain client. In those cases this cache may be a problem. I'm more inclined to add the prefix to the key now. Or not share LRUs between codecs at all. 🤔🤔🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is BytesToString for an address as this path may not point to an unique address. A unique cache key prefix for each There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think caches defined prefix should be sufficient, we can concatenate the prefix to the bytes as a key. cosmos-sdk/codec/address/bech32_codec.go Lines 15 to 17 in 5a5c638
E.g. if two address codecs are defined with prefixes
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would work as well. 👍 Keys get bigger but they are unique |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
func (cbc cachedBech32Codec) StringToBytes(text string) ([]byte, error) { | ||||||||||||||||||||||||||
cbc.mu.Lock() | ||||||||||||||||||||||||||
defer cbc.mu.Unlock() | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if addr, ok := cbc.cache.Get(text); ok { | ||||||||||||||||||||||||||
return addr.([]byte), nil | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
addr, err := cbc.codec.StringToBytes(text) | ||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||
return nil, err | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 it can be worth to cache failures, too. Some benchmarks would be interesting. |
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
cbc.cache.Add(text, addr) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return addr, nil | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optimize locking strategy in The current implementation locks the entire method, which could lead to performance bottlenecks. Consider using a more granular locking strategy or other synchronization techniques like
JulianToledano marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
package address | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/hashicorp/golang-lru/simplelru" | ||
"gotest.tools/v3/assert" | ||
JulianToledano marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
"github.com/cosmos/cosmos-sdk/internal/conv" | ||
) | ||
|
||
func TestNewBech32Codec(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
prefix string | ||
lru *simplelru.LRU | ||
address string | ||
}{ | ||
{ | ||
name: "create accounts cached bech32 codec", | ||
prefix: "cosmos", | ||
lru: accAddrCache, | ||
address: "cosmos1p8s0p6gqc6c9gt77lgr2qqujz49huhu6a80smx", | ||
}, | ||
{ | ||
name: "create validator cached bech32 codec", | ||
prefix: "cosmosvaloper", | ||
lru: valAddrCache, | ||
address: "cosmosvaloper1sjllsnramtg3ewxqwwrwjxfgc4n4ef9u2lcnj0", | ||
}, | ||
{ | ||
name: "create consensus cached bech32 codec", | ||
prefix: "cosmosvalcons", | ||
lru: consAddrCache, | ||
address: "cosmosvalcons1ntk8eualewuprz0gamh8hnvcem2nrcdsgz563h", | ||
}, | ||
} | ||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
assert.Equal(t, tt.lru.Len(), 0) | ||
ac := NewBech32Codec(tt.prefix) | ||
cached, ok := ac.(cachedBech32Codec) | ||
assert.Assert(t, ok) | ||
assert.Equal(t, cached.cache, tt.lru) | ||
|
||
addr, err := ac.StringToBytes(tt.address) | ||
assert.NilError(t, err) | ||
assert.Equal(t, tt.lru.Len(), 1) | ||
|
||
cachedAddr, ok := tt.lru.Get(tt.address) | ||
assert.Assert(t, ok) | ||
assert.DeepEqual(t, addr, cachedAddr) | ||
|
||
accAddr, err := ac.BytesToString(addr) | ||
assert.NilError(t, err) | ||
assert.Equal(t, tt.lru.Len(), 2) | ||
|
||
cachedStrAddr, ok := tt.lru.Get(conv.UnsafeBytesToStr(addr)) | ||
assert.Assert(t, ok) | ||
assert.DeepEqual(t, accAddr, cachedStrAddr) | ||
}) | ||
} | ||
} | ||
JulianToledano marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is always true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, followed the same style as in address.go, although I think it doesn't make much sense.