-
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: Refactor Coins/Validate to avoid unnecessary map #14163
Conversation
and add a few tests
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 for adding this!
I think its helpful, NewCoins is called a lot in testing within Osmosis (and at epoch). Lowering the heap pressure here is v valuable!
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.
thank you for the submission. code cleanup is welcome.
I tried fixing gofumpt formatting but when I run
|
Can we get a quick changelog entry before the merge? |
I can write one if you want. But you have to stop the merge machinery. Will have to check how this is working here. |
Alright, hope this works. Let me know if you need anything else. |
* Refactor (coins Coins) Validate() to avoid unnecessary map and add a few tests * Add CHANGELOG entry Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit 9fd1825) # Conflicts: # CHANGELOG.md
…) (#14182) Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
and add a few tests.
I came across this code when cheking some stuff for CosmWasm. While reading the implementation I though the map there is unnecessary. The moment you know a list is sorted, you only need to check that element
n
andn-1
are not equal for everyn > 0
to find duplicates.I did not do any performance tests here but being able to avoid a hashmap entirely should be helpful if this ends up in some hot path of some codebase.
I would not ship this in a patch release since it most likely creates some unsorted errors in places where you had duplicate errors before. But the docs and tests do not seem to guarantee one error over the other.
Hope this diff does not feel spammy or over optimizing. @ValarDragon said this change matters so I hope it has some value.
Description
Closes: #XXXX
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.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...
!
in the type prefix if API or client breaking change