-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
rpc: optimize getTokenLargestAccounts
#35315
Conversation
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.
Thanks for picking this up!
What you've written seems okay, but I'd generally prefer to follow the existing pattern, unless there's a reason not to that I'm missing (see comment)
}; | ||
largest_token_balances.sort_unstable_by(sort_largest); | ||
let new_entry = (amount, address); | ||
if token_balances.len() >= NUM_LARGEST_ACCOUNTS { |
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.
Technically, ==
should be sufficient here, but I suppose there's no harm in >=
Co-authored-by: Tyera <[email protected]>
@CriesofCarrots updated, can you make a final review, please? 🙂 |
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!
1.18 backport? 🙂 |
Nope, not for now. Precursor PRs were not backported, plus backports require someone to make a case for them. |
* rpc: optimize `getTokenLargestAccounts` * use tuple instead of struct * untuple Co-authored-by: Tyera <[email protected]> --------- Co-authored-by: Tyera <[email protected]>
Problem
getTokenLargestAccounts
collects all data inVec
and after that takes only 30 top accountsSummary of Changes
use
BinaryHeap
instead ofVec
and do not keep more than 30 accountsbased on #34583
Fixes #