perf: Speedup coins.AmountOf() by removing many regex calls #10021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #10021 +/- ##
=======================================
Coverage 63.56% 63.57%
=======================================
Files 572 572
Lines 53598 53600 +2
=======================================
+ Hits 34072 34075 +3
+ Misses 17582 17581 -1
Partials 1944 1944
|
fedekunze
reviewed
Aug 30, 2021
|
|
||
| // AmountOfNoDenomValidation returns the amount of a denom from coins | ||
| // without validating the denomination. | ||
| func (coins Coins) AmountOfNoDenomValidation(denom string) Int { |
There was a problem hiding this comment.
Suggested change
| func (coins Coins) AmountOfNoDenomValidation(denom string) Int { | |
| func (coins Coins) AmountOfWithoutValidation(denom string) Int { |
| midIdx := len(coins) / 2 // 2:1, 3:1, 4:2 | ||
| coin := coins[midIdx] | ||
| switch { | ||
| case denom < coin.Denom: | ||
| return coins[:midIdx].AmountOf(denom) | ||
| return coins[:midIdx].AmountOfNoDenomValidation(denom) |
There was a problem hiding this comment.
Suggested change
| return coins[:midIdx].AmountOfNoDenomValidation(denom) | |
| return coins[:midIdx].AmountOfWithoutValidation(denom) |
| case denom == coin.Denom: | ||
| return coin.Amount | ||
| default: | ||
| return coins[midIdx+1:].AmountOf(denom) | ||
| return coins[midIdx+1:].AmountOfNoDenomValidation(denom) |
There was a problem hiding this comment.
Suggested change
| return coins[midIdx+1:].AmountOfNoDenomValidation(denom) | |
| return coins[midIdx+1:].AmountOfWithoutValidation(denom) |
|
I personally like the name Granted this clearly won't scale if more validations were required, so happy to change the name per your suggestion if you think its much better. |
alexanderbez
approved these changes
Sep 1, 2021
|
Do we need to backport this to v0.44 or 0.42? |
might as well |
tac0turtle
added
A:automerge
|
@ValarDragon can you update the pr, after which the bot will merge it |
|
Visit https://dashboard.github.orijtech.com?back=0&pr=10021&remote=false&repo=cosmos%2Fcosmos-sdk to see benchmark details. |
|
@Mergifyio backport release/v0.44.x |
mergify bot
pushed a commit
that referenced
this pull request
Sep 16, 2021
(cherry picked from commit ed35bfd) # Conflicts: # CHANGELOG.md
|
Command
|
robert-zaremba
added a commit
that referenced
this pull request
Sep 16, 2021
…10021) (#10166) * perf: Speedup coins.AmountOf() by removing many regex calls (#10021) (cherry picked from commit ed35bfd) # Conflicts: # CHANGELOG.md * fix conflict * fix changelog Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Robert Zaremba <robert@zaremba.ch>
evan-forbes
pushed a commit
to evan-forbes/cosmos-sdk
that referenced
this pull request
Oct 12, 2021
…osmos#10021) (cosmos#10166) * perf: Speedup coins.AmountOf() by removing many regex calls (cosmos#10021) (cherry picked from commit ed35bfd) # Conflicts: # CHANGELOG.md * fix conflict * fix changelog Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Robert Zaremba <robert@zaremba.ch>
evan-forbes
pushed a commit
to evan-forbes/cosmos-sdk
that referenced
this pull request
Nov 1, 2021
…osmos#10021) (cosmos#10166) * perf: Speedup coins.AmountOf() by removing many regex calls (cosmos#10021) (cherry picked from commit ed35bfd) # Conflicts: # CHANGELOG.md * fix conflict * fix changelog Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Robert Zaremba <robert@zaremba.ch>
19 tasks
Eengineer1
pushed a commit
to cheqd/cosmos-sdk
that referenced
this pull request
Aug 26, 2022
…osmos#10021) (cosmos#10166) * perf: Speedup coins.AmountOf() by removing many regex calls (cosmos#10021) (cherry picked from commit ed35bfd) * fix conflict * fix changelog Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Speeds up coins.AmountOf() by removing many regex calls when users hold multiple tokens. I expose the AmountOfNoDenomValidation function publicly, because this is quite useful when you know that the denomination is pre-validated, as we do in usecases within Osmosis where we call this function.
(This function is a notable amount of our current epoch time after CacheKV store fixes at approximately 5%, all spent within the regex matching)
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...
!to 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...
!in the type prefix if API or client breaking change