-
Notifications
You must be signed in to change notification settings - Fork 273
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: Remove unneeded GetKey calls to the LRU cache #890
Conversation
WalkthroughThe recent modifications enhance the efficiency and readability of the caching mechanism within 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 (
|
@Mergifyio backport release/v1.x.x |
✅ Backports have been created
|
Co-authored-by: Marko <marbar3778@yahoo.com> (cherry picked from commit 4d6fc7b)
@ValarDragon this isn't fully the correct optimization despite eliminating the extra call to node.Key() while then also keep the original intended optimization or zero byteslice->string conversion in the case of already existing keys when looking im the map. In short it now it incurs the byteslice->string conversion in all cases, but could have been faster if it had been: key := node.GetKey()
if value, ok := c.dict[string(key)]; ok {
...
}
c.dict[string(key)] = value |
if elem, exists := c.dict[string(key)]; exists { | ||
return c.remove(elem) | ||
keyS := string(key) | ||
if elem, exists := c.dict[keyS]; exists { |
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.
This undoes the zero byteslice->string Go map lookup optimization.
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.
Oh wow, didn't know about this compiler optimization.
…zation This change brings back an optimization that go innocently removed while trying to reduce the number of invocations to node.GetKey() in PR cosmos#890. The Go compiler considers the special case of lookups per: value, ok := map[string(byteslice)] or value := map[string(byteslice)] and performs a zero byteslice->string conversion because it knows that there is no storing in the map hence a read-only usage of the byteslice's memory.
Remove some extra
GetKey
calls within the LRU cache code. GetKey calls took 1.5s on an Osmosis 2000 block long block sync. This PR should shave off .45s of that.Though ideally we get the LRU code rethought / adapted more, to make it generally lower overhead. (Its currently 13.5 s on a 2000 block sync!)