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: performance fix for getProgramAccounts #19941

Merged
merged 2 commits into from
Sep 16, 2021

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Sep 16, 2021

Problem

The result accounts were pushed into a fresh vector one-by-one, which produced significant slowdowns for very large responses (visible when exceeding thousands of accounts).

Summary of Changes

Pre-allocate the right capacity - the result length is known.

The accounts were gradually pushed into a vector, which produced
significant slowdowns for very large responses.
@mergify mergify bot added the community Community contribution label Sep 16, 2021
@mergify mergify bot requested a review from a team September 16, 2021 13:39
@ckamm
Copy link
Contributor Author

ckamm commented Sep 16, 2021

Optionally these could also be rewritten to use iterators, like

           keyed_accounts
                .into_iter()
                .map(|(pubkey, account)| {
                    Ok(RpcKeyedAccount {
                         pubkey: pubkey.to_string(),
                         account: encode_account(&account, &pubkey, encoding, data_slice_config)?,
                    })
                })
                .collect::<Result<Vec<_>>>()?

@jstarry
Copy link
Member

jstarry commented Sep 16, 2021

Optionally these could also be rewritten to use iterators, like

           keyed_accounts
                .into_iter()
                .map(|(pubkey, account)| {
                    Ok(RpcKeyedAccount {
                         pubkey: pubkey.to_string(),
                         account: encode_account(&account, &pubkey, encoding, data_slice_config)?,
                    })
                })
                .collect::<Result<Vec<_>>>()?

I typically prefer this approach, do you mind updating?

@ckamm
Copy link
Contributor Author

ckamm commented Sep 16, 2021

@jstarry Not at all, that's what I did initially, then undid to keep the patch solely about the performance aspect. :)

@jstarry
Copy link
Member

jstarry commented Sep 16, 2021

lgtm once the checks are fixed, thanks for this!

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.

Looks like you just need to remove the Ok(..?) wrappers in two of your iterators, then gtg.
Thanks for this!

@mergify mergify bot requested a review from a team September 16, 2021 15:17
@ckamm
Copy link
Contributor Author

ckamm commented Sep 16, 2021

Looks like you just need to remove the Ok(..?) wrappers in two of your iterators, then gtg.

Could you elaborate? I'm intentionally making the map() produce an iterator over Result<_>, so I can deal with the potential error inside the closure. Is there a better way to write it?

@jstarry
Copy link
Member

jstarry commented Sep 16, 2021

Screen Shot 2021-09-16 at 11 07 35 AM

@brianlong
Copy link
Contributor

When this is ready, please prepare a version of v1.6.25 with these changes for staging & user testing. Apps are eager to test this out on the large mainnet account set for Serum Program V3.

@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Sep 16, 2021
@mergify mergify bot requested a review from a team September 16, 2021 16:28
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 again!

@mergify mergify bot requested a review from a team September 16, 2021 17:18
@codecov
Copy link

codecov bot commented Sep 16, 2021

Codecov Report

Merging #19941 (36681ce) into master (28673d5) will decrease coverage by 0.0%.
The diff coverage is 61.8%.

@@            Coverage Diff            @@
##           master   #19941     +/-   ##
=========================================
- Coverage    82.6%    82.6%   -0.1%     
=========================================
  Files         478      478             
  Lines      133443   133545    +102     
=========================================
- Hits       110345   110343      -2     
- Misses      23098    23202    +104     

@mergify mergify bot merged commit f1bbf1d into solana-labs:master Sep 16, 2021
mergify bot pushed a commit that referenced this pull request Sep 16, 2021
* rpc: performance fix for getProgramAccounts

The accounts were gradually pushed into a vector, which produced
significant slowdowns for very large responses.

* rpc: rewrite loops using iterators

Co-authored-by: Christian Kamm <[email protected]>
(cherry picked from commit f1bbf1d)

# Conflicts:
#	core/src/rpc.rs
mergify bot pushed a commit that referenced this pull request Sep 16, 2021
* rpc: performance fix for getProgramAccounts

The accounts were gradually pushed into a vector, which produced
significant slowdowns for very large responses.

* rpc: rewrite loops using iterators

Co-authored-by: Christian Kamm <[email protected]>
(cherry picked from commit f1bbf1d)
mergify bot added a commit that referenced this pull request Sep 16, 2021
* rpc: performance fix for getProgramAccounts (#19941)

* rpc: performance fix for getProgramAccounts

The accounts were gradually pushed into a vector, which produced
significant slowdowns for very large responses.

* rpc: rewrite loops using iterators

Co-authored-by: Christian Kamm <[email protected]>
(cherry picked from commit f1bbf1d)

# Conflicts:
#	core/src/rpc.rs

* fix conflicts

* Fix conflicts

Co-authored-by: Christian Kamm <[email protected]>
Co-authored-by: Justin Starry <[email protected]>
Co-authored-by: Tyera Eulberg <[email protected]>
mergify bot added a commit that referenced this pull request Sep 16, 2021
* rpc: performance fix for getProgramAccounts

The accounts were gradually pushed into a vector, which produced
significant slowdowns for very large responses.

* rpc: rewrite loops using iterators

Co-authored-by: Christian Kamm <[email protected]>
(cherry picked from commit f1bbf1d)

Co-authored-by: Christian Kamm <[email protected]>
@ckamm ckamm deleted the ckamm/perf-getAccounts branch September 17, 2021 05:35
dankelleher pushed a commit to identity-com/solana that referenced this pull request Nov 24, 2021
* rpc: performance fix for getProgramAccounts

The accounts were gradually pushed into a vector, which produced
significant slowdowns for very large responses.

* rpc: rewrite loops using iterators

Co-authored-by: Christian Kamm <[email protected]>
frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants