-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: Speedup coins.Sort() when coins is of length 1 #18875
Conversation
Coins.Sort() performs a heap allocation, even though sort.Sort() is in place. This is because the compiler is doing {something} called runtime.convTSlice which internally makes a copy of the entire slice. We should ideally find a solution that avoids this malloc for every Coins.Sort() of all sizes, but just eliminating it for slices of length <= 1 is already a big win, as Sort's are used throughout the Add and Sub logic.
Warning Rate Limit Exceeded@ValarDragon has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 13 minutes and 59 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. 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 optimizes the sorting process for coin collections. It introduces a conditional check to skip sorting when the collection contains one or zero elements, thereby saving computational resources by avoiding unnecessary operations on already sorted or singleton sets. 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 X ? TipsChat with CodeRabbit Bot (
|
This comment has been minimized.
This comment has been minimized.
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.
Ty!
(cherry picked from commit bd04173) # Conflicts: # CHANGELOG.md
(cherry picked from commit bd04173) # Conflicts: # CHANGELOG.md
#18877) Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Julien Robert <[email protected]>
#18878) Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Julien Robert <[email protected]>
…#18875) (cosmos#18878) Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Julien Robert <[email protected]>
…#18875) (cosmos#18878) Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Coins.Sort() performs a heap allocation, even though sort.Sort() is in place.
This is because the compiler is doing {something} called runtime.convTSlice which internally makes a copy of the entire slice.
We should ideally find a solution that avoids this malloc for every Coins.Sort() of all sizes, but just eliminating it for slices of length <= 1 is already a big win, as Sort's are used throughout the Add and Sub logic.
(At least its a 2 second CPU savings win in Osmosis epoch code)
Author Checklist
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers 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...