clean scan optimization: scan disk index only for zero lamport #2879

HaoranYi · 2024-09-09T19:38:51Z

Problem

Generally, we don't need to scan disk index for clean because disk index only contains single ref single entry account index, which is nearly always "alive" and shouldn't be cleaned, except for one special case - single ref zero accounts (single ref zero accounts need to be cleaned).

Currently, we scan disk index for every candidate, which can be optimized to only scan disk index for zeros.

Summary of Changes

optimize clean scan.

add is_zero mark on cleaning info.
only scan disk index for is_zero candidate.

Fixes #

accounts-db/src/accounts_db.rs

Co-authored-by: Brooks <[email protected]>

brooksprumo · 2024-09-25T14:33:28Z

accounts-db/src/accounts_db.rs

-                            .accounts
-                            .scan_pubkeys(|pubkey| self.insert_pubkey(&candidates, *pubkey));
+
+                        store.accounts.scan_index(|index| {


Ah, I missed this the first time. We're using scan_index() now, instead of scan_pubkeys(). I think this is fine. With AppendVecs this is basically no additional cost. With Tiered Storage, it shouldn't be much additional work to get the is-zero-lamports information (esp once tiered storage has the lamports optimizations).

yeah. we need to read the accounts meta. but it shouldn't be expensive.

accounts-db/src/append_vec.rs

brooksprumo · 2024-09-25T14:37:06Z

accounts-db/src/accounts_db.rs

+                        if candidate_info.might_contain_zero_lamport_entry {
+                            ScanFilter::All
+                        } else {
+                            self.scan_filter_for_shrinking


Can you remind me where self.scan_filter_for_shrinking is set (and what the value is)?

https://github.com/anza-xyz/agave/blob/master/validator/src/main.rs#L1274-L1285

It is set by the CLI.
default is "all", so no impact if not passed by CLI.

Maybe we should rename the CLI in a future PR, as it is not just for shrinking but also for cleaning.

brooksprumo · 2024-09-25T15:22:50Z

accounts-db/src/accounts_db.rs

+                            let is_zero_lamport = index.index_info.lamports == 0;
+                            insert_candidate(pubkey, is_zero_lamport);


As we're scanning here, what happens in the case where this index_info is not zero lamport, but there is another index entry for this pubkey that is zero lamport? If we end up with setting might_contain_zero_lamport_entry to false, will that prevent the later scan from looking on disk and finding the other zero lamport entry?

No. if it was true, then it will still be true, I think. We are oring is_zero_lamport.

candidates_bin .entry(pubkey) .or_default() .might_contain_zero_lamport_entry |= is_zero_lamport;

Yes, I understand if a different entry for the same pubkey sets might_contain_zero_lamport_entry to true, then other falses will not reset the field to false.

My question is, when looking at the dirty stores, what if there's a pubkey A in dirty storage slot 100, with non-zero lamports. Also assume pubkey A is in non dirty storage slot 7 with zero lamports. It looks to me that we'd only look at storage 100 and not storage 7, so the clean candidate would say "nope, no zero lamport entries here" and then the scan filter would not look on disk for the other index entries.

I think after typing that out, I can see why it won't happen. If we have a dirty storage at slot 100, then the index entry for pubkey A must be in the in-memory index, as the slot-list will be 2 in this example. So only looking in-mem is safe, and we don't need to look on disk.

Does that sound right?

Ah. I see what you mean.
Yes, you are correct. If there are other instances of the account outside of the stores that we scan, then disk index is irrelevant because it will always be in memory.

jeffwashington · 2024-09-27T19:19:10Z

i think this is correct. I need to look at this again monday. Do you have a machine monitoring this? Do we have a metric that shows us the difference in clean disk index loads?

HaoranYi · 2024-09-27T20:28:36Z

Yes, it is running on dev12 (3gB7) since yesterday.
I am starting a base line master for comparison now. I will report the result soon.

HaoranYi · 2024-09-28T19:01:38Z

clean scan time stats
red: master
blue: this PR.

It looks like blue is lower and better.

Note that from 8:00-11:00CST we don't have any metrics recorded because the certificate of solama metrics is expired.

jeffwashington · 2024-09-30T19:10:35Z

accounts-db/src/accounts_db.rs

@@ -3226,7 +3249,7 @@ impl AccountsDb {
                    let is_candidate_for_clean =
                        max_slot_inclusive >= *slot && latest_full_snapshot_slot >= *slot;
                    if is_candidate_for_clean {
-                        self.insert_pubkey(&candidates, *pubkey);
+                        insert_candidate(*pubkey, true);


why is this 'true'?

ah, because we are iterating zero_lamport_accounts_to_purge_after_full_snapshot

yes. exactly!

jeffwashington · 2024-09-30T19:36:57Z

@HaoranYi can you please show metric comparison for:
scan_missing on clean_accounts?
Should be close to 0 on normal machines and a lot higher on your machine.

HaoranYi · 2024-09-30T20:18:46Z

@HaoranYi can you please show metric comparison for: scan_missing on clean_accounts? Should be close to 0 on normal machines and a lot higher on your machine.

blue: this PR.
red: master

Yes, missing is much higher with this PR. But on normal machine, missing is small but not close to zero (between 5-6K per minute).

HaoranYi · 2024-09-30T20:40:54Z

I think it is not zero because we are adding dead store back to "dirty stores"
at the end of cleaning-shrinking cycle.

agave/accounts-db/src/accounts_db.rs

Line 8113 in 489f483

dead_slots.insert(*slot);

And those dead storages are not dropped until next clean starts (This is
required and is correct). When next clean starts, all the keys in those dead
stores are populated as candidates, which could be missing from the index.
That's why "missing" is not zero.

jeffwashington · 2024-09-30T21:05:13Z

@HaoranYi can you please show metric comparison for: scan_missing on clean_accounts? Should be close to 0 on normal machines and a lot higher on your machine.

blue: this PR. red: master

Yes, missing is much higher with this PR. But on normal machine, missing is small but not close to zero (between 5-6K per minute).

ok, this graph shows the savings. This many disk lookups are avoided to get the same results.

jeffwashington

lgtm

jeffwashington · 2024-09-30T21:06:22Z

accounts-db/src/accounts_db.rs

+                        if candidate_info.might_contain_zero_lamport_entry {
+                            ScanFilter::All
+                        } else {
+                            self.scan_filter_for_shrinking


note that until we change the default of this cli arg, we'll have no impact on clean (or shrink) due to this filtering scan thing. default is ScanFilter::All

So, once this goes in, we need long term testing of setting scan filter to abnormal only.

HaoranYi · 2024-10-03T14:08:03Z

This has been running fine on mainnet for a week. So merge it.

…xyz#2879) * clean scan optimization * fix rebase conflicts * Update accounts-db/src/accounts_db.rs Co-authored-by: Brooks <[email protected]> * Update accounts-db/src/accounts_db.rs Co-authored-by: Brooks <[email protected]> * Update accounts-db/src/accounts_db.rs Co-authored-by: Brooks <[email protected]> * Update accounts-db/src/accounts_db.rs Co-authored-by: Brooks <[email protected]> * review update * revert ZeroLamport trait for IndexInfoInner --------- Co-authored-by: HaoranYi <[email protected]> Co-authored-by: Brooks <[email protected]>

HaoranYi marked this pull request as draft September 9, 2024 19:39

HaoranYi force-pushed the accounts-db/clean_scan_opt branch 3 times, most recently from 5f2c5ff to f557e79 Compare September 10, 2024 22:08

clean scan optimization

5cda868

HaoranYi force-pushed the accounts-db/clean_scan_opt branch from f557e79 to 5cda868 Compare September 24, 2024 20:41

fix rebase conflicts

26f77fb

HaoranYi marked this pull request as ready for review September 24, 2024 21:01

HaoranYi requested review from brooksprumo and jeffwashington September 25, 2024 13:08

HaoranYi changed the title ~~clean scan optimization~~ clean scan optimization: scan disk index only for zeros Sep 25, 2024

brooksprumo reviewed Sep 25, 2024

View reviewed changes

HaoranYi and others added 5 commits September 25, 2024 09:04

Update accounts-db/src/accounts_db.rs

84263bf

Co-authored-by: Brooks <[email protected]>

Update accounts-db/src/accounts_db.rs

07ae15d

Co-authored-by: Brooks <[email protected]>

Update accounts-db/src/accounts_db.rs

d245937

Co-authored-by: Brooks <[email protected]>

Update accounts-db/src/accounts_db.rs

a3333ab

Co-authored-by: Brooks <[email protected]>

review update

e45cf24

HaoranYi requested a review from brooksprumo September 25, 2024 14:26

brooksprumo reviewed Sep 25, 2024

View reviewed changes

revert ZeroLamport trait for IndexInfoInner

9f676c4

brooksprumo reviewed Sep 25, 2024

View reviewed changes

HaoranYi requested a review from brooksprumo September 25, 2024 16:08

HaoranYi changed the title ~~clean scan optimization: scan disk index only for zeros~~ clean scan optimization: scan disk index only for zero lamport Sep 26, 2024

jeffwashington reviewed Sep 30, 2024

View reviewed changes

jeffwashington self-requested a review September 30, 2024 20:17

jeffwashington approved these changes Sep 30, 2024

View reviewed changes

jeffwashington reviewed Sep 30, 2024

View reviewed changes

HaoranYi merged commit b23e636 into anza-xyz:master Oct 3, 2024
40 checks passed

HaoranYi deleted the accounts-db/clean_scan_opt branch October 3, 2024 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean scan optimization: scan disk index only for zero lamport #2879

clean scan optimization: scan disk index only for zero lamport #2879

HaoranYi commented Sep 9, 2024 •

edited

Loading

brooksprumo Sep 25, 2024

HaoranYi Sep 25, 2024

brooksprumo Sep 25, 2024

HaoranYi Sep 25, 2024 •

edited

Loading

brooksprumo Sep 25, 2024

HaoranYi Sep 25, 2024 •

edited

Loading

brooksprumo Sep 25, 2024

HaoranYi Sep 25, 2024

jeffwashington commented Sep 27, 2024

HaoranYi commented Sep 27, 2024

HaoranYi commented Sep 28, 2024 •

edited

Loading

jeffwashington Sep 30, 2024

jeffwashington Sep 30, 2024

HaoranYi Sep 30, 2024

jeffwashington commented Sep 30, 2024

HaoranYi commented Sep 30, 2024

HaoranYi commented Sep 30, 2024

jeffwashington commented Sep 30, 2024

jeffwashington left a comment

jeffwashington Sep 30, 2024 •

edited

Loading

HaoranYi commented Oct 3, 2024

		let is_zero_lamport = index.index_info.lamports == 0;
		insert_candidate(pubkey, is_zero_lamport);

clean scan optimization: scan disk index only for zero lamport #2879

clean scan optimization: scan disk index only for zero lamport #2879

Conversation

HaoranYi commented Sep 9, 2024 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoranYi Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoranYi Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffwashington commented Sep 27, 2024

HaoranYi commented Sep 27, 2024

HaoranYi commented Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffwashington commented Sep 30, 2024

HaoranYi commented Sep 30, 2024

HaoranYi commented Sep 30, 2024

jeffwashington commented Sep 30, 2024

jeffwashington left a comment

Choose a reason for hiding this comment

jeffwashington Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

HaoranYi commented Oct 3, 2024

HaoranYi commented Sep 9, 2024 •

edited

Loading

HaoranYi Sep 25, 2024 •

edited

Loading

HaoranYi Sep 25, 2024 •

edited

Loading

HaoranYi commented Sep 28, 2024 •

edited

Loading

jeffwashington Sep 30, 2024 •

edited

Loading