Rework and optimize AccountIndexIterator

[HaoranYi]

Problem

AccountIndexIterator has several issues.

1. The current implementation is unnecessarily complex, make it hard to maintain and read.
2. It has discrepancy between the sorted and unsorted iterator behavior.
3. It is not optimized, resulting redundant bin map loading/filtering/sorting operations.

Therefore, we would like to rework and optimize the code.

Summary of Changes

1. Bin Map Ownership and Abstraction:

• Bin map management functions have been relocated from AccountIndexIterator to AccountIndex. This reflects the appropriate ownership of bin maps by AccountIndex.
• The previous reliance on u32::max as a proxy for the actual bin map count has been eliminated, preventing potential inaccuracies and errors. The iterator layer should not be responsible for knowing the total number of binmaps.

2. hold_range_in_memory Relocation:

• The hold_range_in_memory function has been moved from AccountIndexIterator to AccountIndex, aligning its location with the appropriate level of abstraction.

3. Iterator Algorithm Refactoring and Unification:

• The iterator algorithm has been completely rewritten to eliminate discrepancies between sorted and unsorted iterator behaviors.
• The previous unsorted iterator's memory-intensive approach has been replaced with a batch-processing strategy, mirroring the sorted iterator's behavior.
• Redundant bin map loading, filtering, and sorting operations have been eliminated. Each bin map is now processed at most once, significantly improving performance.
• The code has been simplified, removing unnecessary complexity (e.g., break/goto statements), resulting in a more readable and maintainable implementation.
• The sorted and unsorted iterators are now unified in their behavior, both returning items in batches.

4. Interface Improvements:

• The iterator interface has been refined to accept pubkey ranges by reference, eliminating unnecessary cloning and improving efficiency.

5. Additional tests are added.

Fixes #

[HaoranYi]

I have been running this PR for the past 3 days on mainnet with out any issues.

[HaoranYi]

I pushed a commit to fix the conflicts after rebasing to master.

[brooksprumo]

I kinda feel that the sorted vs unsorted iterators are fundamentally different enough where we could (should?) have two separate iterator types and implementations. Wdyt?

[HaoranYi]

I kinda feel that the sorted vs unsorted iterators are fundamentally different enough where we could (should?) have two separate iterator types and implementations. Wdyt?

yes. i think it is good idea.
i will refactor them into two iterators.

[HaoranYi]

@brooksprumo can you help to review this PR again?

[brooksprumo]

@brooksprumo can you help to review this PR again?

Sure, will do!

[brooksprumo]

I need to go through the core of the implementation still. There's some code organization that is making the review a bit challenging for me in Github. I'll pull the code locally and see if that helps.

[HaoranYi]

@brooksprumo I pushed a commit to implement sorted/unsorted iterator in a different way.
Instead of using an inner class, I used trait with default implementation to share the common code. And I also get rid of the enum wapper, added a new test for unsorted iterator.

Can you take a look again? thanks!

[brooksprumo]

@brooksprumo I pushed a commit to implement sorted/unsorted iterator in a different way. Instead of using an inner class, I used trait with default implementation to share the common code. And I also get rid of the enum wapper, added a new test for unsorted iterator.

I like this direction. Can we go one step further and remove AccountsIndexIteratorCommon too? I'm not seeing a lot of benefit here, since both the sorted and unsorted iterators already implement std::iter::Iterator, which should allow them to work interchangeably by the caller. Maybe if there were more shared code then there would be more use default impl within the trait and thus make it more useful? I think mainly I wonder, if we fully duplicate all the code between the two iterators, how much is actually different? I'm thinking maybe not a lot? Wdyt?

[HaoranYi]

ok. pushed another change to remove the trait and unshare the common code between two types of iterators.
I keep the refactor of clone_bound and bin_from_bound out of the class method, so that these two can still be shared.

let me know wdyt.

[brooksprumo]

Wow, ok, I've spent a lot of time staring at all this. Sorry for all the refactor/nit questions/comments. Let me know your thoughts on how to proceed. Some things of the code currently in master feel broken and I'm not sure if/when/where to fix it all.

[HaoranYi]

pushed one more commit to save two copies of pubkey in hold_range_in memory.

After inlining the hold_range_in memory, we already have a ref of Range. so we can use as_ref to avoid the clone of the pubkeys.

[HaoranYi]

one more change on the iterator to take ref of pubkey range instead of copy.

[HaoranYi]

After #4729 is merged, we can optimize the account index iterator more by using the lowest_pubkey and highest_pubkey range on the account's index map.

[jeffwashington]

Sorry - I'm missing context. What is the problem? Soon, we'll stop iterating during rent collection. Who else does these index scans?

[jeffwashington]

Sorry - I'm missing context. What is the problem? Soon, we'll stop iterating during rent collection. Who else does these index scans?

On mnb activation schedule: skip rewrites is #3 and stop collecting rent is #4.
However, it looks like we are still scanning/loading all accounts during rent collection even with the activation of these 2 features. Is that accurate?

[HaoranYi]

Sorry - I'm missing context. What is the problem? Soon, we'll stop iterating during rent collection. Who else does these index scans?

On mnb activation schedule: skip rewrites is #3 and stop collecting rent is #4. However, it looks like we are still scanning/loading all accounts during rent collection even with the activation of these 2 features. Is that accurate?

Yes, it is correct.
Rent scan may have a impact on a corner case where we could update rent_epoch. If its rent_epoch is not already rent exempted, rent scan will update the rent_epoch to rent exempted . That's why we are still required to do rent scan even though we don't collect any rent.

There is another SIMD, which exclude rent_epoch field changes in rent scan. When that SIMD be active, we can then safely disable rent scan.

The account index iterator is used for account range scan, which is used by RPC method to scan accounts from index. We have received complains from rpc admins about account's scan is slow.

[HaoranYi]

These are the relevant SIMDS for disable rent scan.
solana-foundation/solana-improvement-documents#230
solana-foundation/solana-improvement-documents#175

[HaoranYi]

@brooksprumo Iast week, I rework the PR in a different direction.
More details can be found in this comment
Can you review it again?

[brooksprumo]

Can the PR description be updated with the new direction this PR has gone?

[HaoranYi]

Can the PR description be updated with the new direction this PR has gone?

done.

[brooksprumo]

@HaoranYi Are you running this current version on mnb? Can you share relevant perf info, please?

[HaoranYi]

@HaoranYi Are you running this current version on mnb? Can you share relevant perf info, please?

I have started running this pr on shared-dev11. will report the perf info soon.

[brooksprumo]

This looks right to me!

[HaoranYi]

I have been running and monitoring this PR on dev11. It has been running for 1 epoch and all major matrices are normal.