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

accounts-db: unpack_archive: unpack accounts straight into their final destination #289

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

alessandrod
Copy link

We used to unpack accounts into account_path/accounts/ then rename to account_path/. We now unpack them into their final destination directly and avoid the rename syscall. I've added a test and verified that it behaves the same with both old and new impl.

I also removed an extra parsing/iteration over entry paths which was unnecessary.

See discord conversation for context https://discord.com/channels/428295358100013066/838890116386521088/1217386980292427786

@alessandrod alessandrod force-pushed the unpack-accounts-no-rename branch 3 times, most recently from 8a446e5 to c9ae0cd Compare March 18, 2024 08:07
@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 94.59459% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (67c3bff) to head (9d3c754).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #289   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         837      837           
  Lines      226874   226891   +17     
=======================================
+ Hits       185870   185884   +14     
- Misses      41004    41007    +3     

apfitzge
apfitzge previously approved these changes Mar 18, 2024
Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm - left a few suggestions to avoid pattern of (manual check, unwrap/index). I think the suggestions should not add additional overhead, but can be left to a follow-up PR if you prefer.

accounts-db/src/hardened_unpack.rs Outdated Show resolved Hide resolved
accounts-db/src/hardened_unpack.rs Outdated Show resolved Hide resolved
@alessandrod
Copy link
Author

lgtm - left a few suggestions to avoid pattern of (manual check, unwrap/index). I think the suggestions should not add additional overhead, but can be left to a follow-up PR if you prefer.

Thanks for the review! Applied both suggestions.

@apfitzge
Copy link

Thanks for the review! Applied both suggestions.

Aghh, looks like clippy didn't like the match I provided and asked you to do a let else

@alessandrod alessandrod force-pushed the unpack-accounts-no-rename branch from dc517e5 to a2f4ec0 Compare March 18, 2024 22:09
@alessandrod
Copy link
Author

Thanks for the review! Applied both suggestions.

Aghh, looks like clippy didn't like the match I provided and asked you to do a let else

Oops, fixed

@alessandrod alessandrod force-pushed the unpack-accounts-no-rename branch from a2f4ec0 to 9d3c754 Compare March 19, 2024 03:19
We used to do a iterator.clone().any(...) followed by
iterator.collect(). Merge the two and avoid an extra iteration and
re-parsing of the path.
…l destination

We used to unpack accounts into account_path/accounts/<account> then
rename to account_path/<account>. We now unpack them into their final
destination directly and avoid the rename syscall.
@alessandrod alessandrod force-pushed the unpack-accounts-no-rename branch from 9d3c754 to 399e17f Compare March 19, 2024 20:43
@alessandrod alessandrod merged commit 8df80d9 into anza-xyz:master Mar 19, 2024
35 checks passed
@alessandrod alessandrod deleted the unpack-accounts-no-rename branch March 19, 2024 22:39
codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
…l destination (anza-xyz#289)

* accounts-db: unpack_archive: avoid extra iteration on each path

We used to do a iterator.clone().any(...) followed by
iterator.collect(). Merge the two and avoid an extra iteration and
re-parsing of the path.

* accounts-db: unpack_archive: unpack accounts straight into their final destination

We used to unpack accounts into account_path/accounts/<account> then
rename to account_path/<account>. We now unpack them into their final
destination directly and avoid the rename syscall.
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.

3 participants