Implementing an Account Storage Reader to stream Account Storages during snapshots and testing by roryharr · Pull Request #5950 · anza-xyz/agave

roryharr · 2025-04-24T00:51:18Z

Problem

Account archive internals are exposed to the runtime library
Dead accounts need to be filtered when being written to storage during snapshot.

Summary of Changes

Implemented an account storage reader to convert an account storage into a class that supports the trait read.
Modified paths that stream account storages to use the new type.

Fixes #

roryharr · 2025-04-24T00:52:15Z

First Graph: Just this change (no dead accounts)

Snapshot time, orange and purple are just reference machines.
Blue is the dev box that is running both new snapshot code and old snapshot code.
At 22:00 the old snapshot code is running. At 3:00, 9:00 and 14:00 the new snapshot code is running. No Performance Difference.

Second Graph: Dead_Accounts code transitioning from old method of filter to new method of filtering

Transition from old method to new method occurs ~1830. Snapshot time goes from 1.70b us (~28min) to 1.16b us (~20min)

codecov-commenter · 2025-04-24T01:46:59Z

Codecov Report

Attention: Patch coverage is 97.36842% with 9 lines in your changes missing coverage. Please review.

Project coverage is 82.9%. Comparing base (c27b011) to head (8b3c3d8).
Report is 86 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #5950    +/-   ##
========================================
  Coverage    82.9%    82.9%            
========================================
  Files         842      843     +1     
  Lines      377804   378161   +357     
========================================
+ Hits       313223   313637   +414     
+ Misses      64581    64524    -57

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

brooksprumo

Now that this PR has dead accounts stuff in it, the PR is getting quite involved. It may actually make sense to do the AccountStorageReader on its own, without any dead accounts stuff. (I haven't tried implementing that, so not sure exactly what it looks like though.)

…ing snapshots and testing

- Added Unit Tests for New Module - Moved dead accounts filtering to accounts_db - Removed unwraps

Cleaned up various uneeded references

Fixed reopen test hook to use new reference

roryharr · 2025-05-14T19:57:20Z

Now that this PR has dead accounts stuff in it, the PR is getting quite involved. It may actually make sense to do the AccountStorageReader on its own, without any dead accounts stuff. (I haven't tried implementing that, so not sure exactly what it looks like though.)

I had missed this comment at the time.
The reason to add the dead accounts stuff is that this was largely untestable without it and the intent was unclear...

Modifying test flow to increase coverage

brooksprumo

I need to do another pass over the tests, but I think this looks good.

Co-authored-by: Brooks <brooks@prumo.org>

brooksprumo

Tests are nicely done!

brooksprumo · 2025-05-17T19:49:24Z

Also, how's the perf of this code? Does it impact snapshot at all, given that we aren't populating the obsolete accounts yet?

roryharr · 2025-05-23T17:46:58Z

Also, how's the perf of this code? Does it impact snapshot at all, given that we aren't populating the obsolete accounts yet?

I'll get a re-run today with no dead accounts. Previously when I tested it it had no impact on perf without dead accounts, and actually improved things slightly with lots of dead accounts due to saving less data.

Using randomly generated seeds for testing. Logging seeds for reproduciability Updated test to ensure last account being invalidated is always one of the test scenarios

roryharr · 2025-05-24T00:41:37Z

Also, how's the perf of this code? Does it impact snapshot at all, given that we aren't populating the obsolete accounts yet?

I'll get a re-run today with no dead accounts. Previously when I tested it it had no impact on perf without dead accounts, and actually improved things slightly with lots of dead accounts due to saving less data.

Reconfirmed, no performance degredation

Co-authored-by: Brooks <brooks@prumo.org>

brooksprumo

Great work!

…ing snapshots and testing (anza-xyz#5950) * Implementing an Account Storage Reader to stream Account Storages during snapshots and testing This will be used with the obsolete account feature to remove obsolete entires during snapshots.

roryharr commented Apr 24, 2025

View reviewed changes

Comment thread accounts-db/src/accounts_db.rs Outdated

roryharr changed the title ~~Implementing an Account Storage Reader to stream Account Storages durng snapshots and testing~~ Implementing an Account Storage Reader to stream Account Storages during snapshots and testing Apr 24, 2025

roryharr requested review from brooksprumo and jeffwashington April 25, 2025 18:53

roryharr commented Apr 25, 2025

View reviewed changes

Comment thread runtime/src/serde_snapshot/tests.rs

roryharr force-pushed the implement_account_storage_reader branch 2 times, most recently from 6495364 to 0a1b346 Compare April 25, 2025 18:59

brooksprumo reviewed Apr 25, 2025

View reviewed changes

roryharr requested a review from brooksprumo April 29, 2025 06:54

roryharr marked this pull request as ready for review April 29, 2025 06:54

brooksprumo reviewed Apr 30, 2025

View reviewed changes

roryharr added 6 commits May 9, 2025 10:44

Implementing an Account Storage Reader to stream Account Storages dur…

821a3f7

…ing snapshots and testing

Only reading to correct file size

c1bda73

Updated code based on feedback

c158d6d

- Added Unit Tests for New Module - Moved dead accounts filtering to accounts_db - Removed unwraps

Rust FMT on Tests

f47868e

Added test support for MMap/FileIO and Tiered Storage

778573d

Cleaned up various uneeded references

Rebasing on top of new API

6b049cb

roryharr force-pushed the implement_account_storage_reader branch from f80903e to 6b049cb Compare May 9, 2025 21:39

roryharr requested a review from brooksprumo May 12, 2025 17:27

brooksprumo reviewed May 13, 2025

View reviewed changes

Comment thread accounts-db/src/accounts_db.rs Outdated

Comment thread accounts-db/src/accounts_db.rs Outdated

Comment thread accounts-db/src/accounts_db.rs Outdated

Comment thread accounts-db/src/account_storage_reader.rs Outdated

Comment thread accounts-db/src/account_storage_reader.rs

brooksprumo self-requested a review May 13, 2025 15:10

brooksprumo reviewed May 13, 2025

View reviewed changes

Comment thread accounts-db/src/account_storage_reader.rs Outdated

Comment thread accounts-db/src/account_storage_reader.rs Outdated

Comment thread accounts-db/src/account_storage_reader.rs Outdated

brooksprumo self-requested a review May 13, 2025 17:35

Resolving Review feedback

bdef51f

Fixed reopen test hook to use new reference

roryharr commented May 13, 2025

View reviewed changes

Comment thread accounts-db/src/account_storage_reader.rs

Adding assert to ensure file is opened in the proper method

dad87e7

roryharr force-pushed the implement_account_storage_reader branch from 5bc04a6 to dad87e7 Compare May 13, 2025 22:48

roryharr added 3 commits May 14, 2025 13:56

Adding assert for Tiered Storage

6cac7e9

Modifying test flow to increase coverage

Updating naming from dead accounts to obsolete accounts

cddda28

Ran format

9ceb76b

brooksprumo reviewed May 16, 2025

View reviewed changes

Comment thread accounts-db/src/account_storage_reader.rs Outdated

Comment thread accounts-db/src/account_storage_reader.rs Outdated

brooksprumo self-requested a review May 16, 2025 19:29

roryharr and others added 2 commits May 16, 2025 16:30

Update accounts-db/src/account_storage_reader.rs

3017c6b

Co-authored-by: Brooks <brooks@prumo.org>

Remove unnecessary path qualifications

35086a1

brooksprumo reviewed May 17, 2025

View reviewed changes

Fixing the private key allocation

52f3c36

Using randomly generated seeds for testing. Logging seeds for reproduciability Updated test to ensure last account being invalidated is always one of the test scenarios

roryharr requested a review from brooksprumo May 24, 2025 00:41

brooksprumo reviewed May 25, 2025

View reviewed changes

Comment thread accounts-db/src/account_storage_reader.rs Outdated

Comment thread accounts-db/src/account_storage_reader.rs Outdated

Comment thread accounts-db/src/account_storage_reader.rs Outdated

roryharr and others added 3 commits May 26, 2025 09:59

Update accounts-db/src/account_storage_reader.rs

053996e

Co-authored-by: Brooks <brooks@prumo.org>

Using into iter to avoid clone

a6c909c

Cargo Format

8b3c3d8

brooksprumo approved these changes May 26, 2025

View reviewed changes

roryharr merged commit 060ff07 into anza-xyz:master May 27, 2025
47 checks passed

Conversation

roryharr commented Apr 24, 2025

Problem

Summary of Changes

Uh oh!

Uh oh!

roryharr commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brooksprumo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roryharr commented May 14, 2025

Uh oh!

brooksprumo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

brooksprumo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brooksprumo commented May 17, 2025

Uh oh!

roryharr commented May 23, 2025

Uh oh!

roryharr commented May 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brooksprumo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

roryharr commented Apr 24, 2025 •

edited

Loading

codecov-commenter commented Apr 24, 2025 •

edited

Loading