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

add state while scanning #25418

Merged
merged 1 commit into from May 31, 2022
Merged

Conversation

jeffwashington
Copy link
Contributor

@jeffwashington jeffwashington commented May 20, 2022

Problem

Aiming towards efficiency and simplicity wrt scanning billions of accounts in multiple passes.

Having a struct can reduce the code that runs per append vec and more importantly per account.
We can also add filtering later, reducing the work to skip accounts that don't match the current pass.

Summary of Changes

Fixes #

@jeffwashington jeffwashington added the CI Pull Request is ready to enter CI label May 20, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label May 20, 2022
@jeffwashington jeffwashington force-pushed the may45_3_2 branch 4 times, most recently from 510bbc6 to 3d692fd Compare May 23, 2022 17:39
@jeffwashington jeffwashington marked this pull request as ready for review May 23, 2022 17:41
@jeffwashington jeffwashington force-pushed the may45_3_2 branch 6 times, most recently from a2b5f02 to b0e5339 Compare May 24, 2022 14:55
HaoranYi
HaoranYi previously approved these changes May 24, 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. Thanks!

self.mismatch_found.fetch_add(1, Ordering::Relaxed);
}
}
if self.accum.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

could use init_accum instead.

brooksprumo
brooksprumo previously approved these changes May 24, 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.

I think this looks good. The problem description makes sense, but I don't fully understand all the code. Glad there was another set of eyes on it as well.

Comment on lines +1675 to +1677
if self.accum.is_empty() {
self.accum.append(&mut vec![Vec::new(); self.range]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this call self.init_accum(self.range) instead?

@mergify mergify bot dismissed stale reviews from brooksprumo and HaoranYi May 31, 2022 14:47

Pull request has been modified.

@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #25418 (17e305e) into master (02b26dd) will decrease coverage by 0.0%.
The diff coverage is 77.8%.

@@            Coverage Diff            @@
##           master   #25418     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         621      621             
  Lines      170297   170393     +96     
=========================================
+ Hits       139917   139946     +29     
- Misses      30380    30447     +67     

@jeffwashington jeffwashington merged commit 5994b4f into solana-labs:master May 31, 2022
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 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants