Skip to content
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

Merged
merged 4 commits into from
Feb 27, 2024

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Feb 26, 2024

Problem

getTokenLargestAccounts collects all data in Vec and after that takes only 30 top accounts

Summary of Changes

use BinaryHeap instead of Vec and do not keep more than 30 accounts

based on #34583

Fixes #

@mergify mergify bot added community Community contribution need:merge-assist labels Feb 26, 2024
@mergify mergify bot requested a review from a team February 26, 2024 00:12
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a 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)

rpc/src/rpc.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Outdated Show resolved Hide resolved
};
largest_token_balances.sort_unstable_by(sort_largest);
let new_entry = (amount, address);
if token_balances.len() >= NUM_LARGEST_ACCOUNTS {
Copy link
Contributor

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 CriesofCarrots added the CI Pull Request is ready to enter CI label Feb 27, 2024
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Feb 27, 2024
@fanatid
Copy link
Contributor Author

fanatid commented Feb 27, 2024

@CriesofCarrots updated, can you make a final review, please? 🙂

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@CriesofCarrots CriesofCarrots merged commit 8ad125d into solana-labs:master Feb 27, 2024
37 checks passed
@fanatid fanatid deleted the rpc-gettla-master branch February 27, 2024 16:23
@fanatid
Copy link
Contributor Author

fanatid commented Feb 27, 2024

1.18 backport? 🙂

@CriesofCarrots
Copy link
Contributor

1.18 backport? 🙂

Nope, not for now. Precursor PRs were not backported, plus backports require someone to make a case for them.

jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Feb 27, 2024
* rpc: optimize `getTokenLargestAccounts`

* use tuple instead of struct

* untuple

Co-authored-by: Tyera <[email protected]>

---------

Co-authored-by: Tyera <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants