Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

remove par_iter on update index below threshold #25699

Merged
merged 1 commit into from
Jun 1, 2022

Conversation

jeffwashington
Copy link
Contributor

@jeffwashington jeffwashington commented Jun 1, 2022

Problem

Improve perf for common call with low # accounts to update.
See #24692

The largest call I saw on a mnb snapshot and brief ledger replay was 302 accounts being updated at a time.

Summary of Changes

Fixes #

})
.flatten()
.collect::<Vec<_>>()
let threshold = 1;
Copy link
Contributor

@HaoranYi HaoranYi Jun 1, 2022

Choose a reason for hiding this comment

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

make it const? hoping that the compiler can const propagating and inline lambda for the special case of 1.

HaoranYi
HaoranYi previously approved these changes Jun 1, 2022
Copy link
Contributor

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm

@jeffwashington
Copy link
Contributor Author

data on my mnb run for all # accounts > 1% of the total:

# accounts | COUNT | % | cumulative %
-- | -- | -- | --
1 | 504160 | 61% | 61%
4 | 97797 | 12% | 73%
2 | 56329 | 7% | 80%
6 | 53812 | 7% | 86%
8 | 23438 | 3% | 89%
10 | 16963 | 2% | 91%
9 | 12817 | 2% | 93%
12 | 11043 | 1% | 94%
14 | 6911 | 1% | 95%
16 | 5323 | 1% | 96%

@jeffwashington jeffwashington marked this pull request as ready for review June 1, 2022 18:24
@jeffwashington
Copy link
Contributor Author

jeffwashington commented Jun 1, 2022

Note that any individual pubkey update index call could be a page fault currently. With a threshold of 1, we are clearly better not adding parallelism. It gets more complicated to know with len > 1.

@jeffwashington
Copy link
Contributor Author

To get to a cumulative ~1% of calls, we have to include all calls with len > 64.
Here are the largest 10 len values:


len | COUNT of len | % | cumulative %
-- | -- | -- | --
302 | 15 | 0% | 0%
264 | 15 | 0% | 0%
262 | 1 | 0% | 0%
232 | 9 | 0% | 0%
230 | 2 | 0% | 0%
196 | 15 | 0% | 0%
192 | 1 | 0% | 0%
190 | 1 | 0% | 0%
188 | 11 | 0% | 0%
186 | 3 | 0% | 0%

brooksprumo
brooksprumo previously approved these changes Jun 1, 2022
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Looks good to me as well.

@mergify mergify bot dismissed stale reviews from brooksprumo and HaoranYi June 1, 2022 19:34

Pull request has been modified.

@jeffwashington
Copy link
Contributor Author

doh. my metrics were from testnet...

@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #25699 (25c6cdf) into master (3ed2e0c) will increase coverage by 6.6%.
The diff coverage is 53.5%.

@@             Coverage Diff             @@
##           master   #25699       +/-   ##
===========================================
+ Coverage    75.4%    82.1%     +6.6%     
===========================================
  Files          40      621      +581     
  Lines        2345   170485   +168140     
  Branches      338        0      -338     
===========================================
+ Hits         1769   139978   +138209     
- Misses        459    30507    +30048     
+ Partials      117        0      -117     

@jeffwashington jeffwashington merged commit 149a54b into solana-labs:master Jun 1, 2022
mergify bot pushed a commit that referenced this pull request Jun 1, 2022
mergify bot added a commit that referenced this pull request Jun 2, 2022
(cherry picked from commit 149a54b)

Co-authored-by: Jeff Washington (jwash) <[email protected]>
jeffwashington added a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
jeffwashington added a commit to jeffwashington/solana that referenced this pull request Jun 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants