perf: add cache to address codec #20122
Conversation
|
Warning Rate Limit Exceeded@JulianToledano has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 29 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe update primarily introduces caching for Bech32 address conversions in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
codec/address/bech32_codec.go
Outdated
| 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 { | ||
| return addr.(string), nil | ||
| } | ||
|
|
||
| addr, err := cbc.codec.BytesToString(bz) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| cbc.cache.Add(key, addr) | ||
|
|
||
| return addr, nil | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
| cbc.cache.Add(text, addr) | ||
|
|
||
| return addr, nil | ||
| } |
There was a problem hiding this comment.
Optimize locking strategy in cachedBech32Codec methods.
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 sync.Map which is optimized for the use case where the entry for a given key is only written once but read many times.
There was a problem hiding this comment.
Good start! 🏃
I added some thoughts and ideas although I have only limited context on this feature.
When it comes to caching, I also have telemetry on my list to learn about the utilization. This may be overkill but just sharing this collector as example.
| } | ||
| if valAddrCache, err = simplelru.NewLRU(500, nil); err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
🤔 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.
The bytes of the addresses are the same, but there are three possible outputs (AccAddress, ValAddress, ConsAddress). I see three options here:
- Use three different caches.
- Add the prefix of the address codec to the key
- Store a map in the LRU with prefix as keys (e.g.,
map["cosmos"] = "cosmos1dr3...")
I've chosen the first option to maintain consistency with how things were done before.
Lines 100 to 111 in 8e60f3b
|
|
||
| // 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.
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.
Agreed. I'm not sure what the best approach is to solve this, as LRUs are currently shared between codecs and defined as globals.
|
|
||
| func init() { | ||
| var err error | ||
| isCachingEnabled.Store(true) |
There was a problem hiding this comment.
Yeah, followed the same style as in address.go, although I think it doesn't make much sense.
|
|
||
| addr, err := cbc.codec.StringToBytes(text) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🤔 it can be worth to cache failures, too. Some benchmarks would be interesting.
| cbc.mu.Lock() | ||
| defer cbc.mu.Unlock() | ||
|
|
||
| if addr, ok := cbc.cache.Get(key); ok { |
There was a problem hiding this comment.
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.
See first comment. Currently, there are three different caches, but adding key prefixes may be a solution if we prefer just one.
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
codec/address/bech32_codec.go (1)
39-54: Consider consolidating cache types or making cache configuration more flexible, as multiple cache types might not be necessary and could complicate maintenance.
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- codec/address/bech32_codec.go (2 hunks)
- codec/address/bech32_codec_test.go (1 hunks)
Additional Context Used
Path-based Instructions (2)
codec/address/bech32_codec_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"codec/address/bech32_codec.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (6)
codec/address/bech32_codec_test.go (2)
17-66: The unit tests inTestNewBech32Codeceffectively validate the caching mechanism and address conversions. Good coverage of different prefixes and cache interactions.
70-92: The tests inTestMultipleBech32Codeccorrectly ensure that separate caches for different codecs do not interfere with each other and validate the caching behavior.codec/address/bech32_codec.go (4)
39-53: Initialization of caches and mutexes is correctly implemented, ensuring separate caches for different address types and appropriate error handling.
69-91: TheNewBech32Codecfunction is well-implemented, correctly handling the selection of caches and mutexes based on the prefix and the caching condition.
Line range hint
91-133: MethodsStringToBytesandBytesToStringinBech32Codecare correctly implemented with comprehensive error handling and validation.
134-168: Methods incachedBech32Codecare correctly implemented with thread safety and caching. Consider optimizing the locking strategy to improve performance, as discussed in previous comments.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- codec/address/bech32_codec_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- codec/address/bech32_codec_test.go
Description
ref:
#13140
#7448
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
Summary by CodeRabbit
NewSimAppfunction to use specific prefixes.