Optimize max_flushed_root update in flushing roots #4320

HaoranYi · 2025-01-07T16:19:05Z

Problem

When updating max flushed root in accounts index, we don't need to update it for ever flushed root in the loop. We can update it with the last flushed root.

split from #4304

Summary of Changes

update max flushed root in account index with the last flushed root.

Fixes #

HaoranYi · 2025-01-07T16:21:48Z

accounts-db/src/accounts_db.rs

@@ -6295,33 +6295,35 @@ impl AccountsDb {
        });

        // Always flush up to `requested_flush_root`, which is necessary for things like snapshotting.
-        let cached_roots: BTreeSet<Slot> = self.accounts_cache.clear_roots(requested_flush_root);
+        let flushed_roots: BTreeSet<Slot> = self.accounts_cache.clear_roots(requested_flush_root);


rename: cached_roots -> flushed_roots for clarity.

HaoranYi · 2025-01-07T16:28:03Z

probably won't matter much in actual performance. But it is still nice to have.

brooksprumo

One thing I thought of, where dooes the accounts_cache's max_flush_root get used? Anywhere in the foreground/transaction processing? That's the only place I can think of where delaying when set_max_flush_root() may make a difference.

HaoranYi · 2025-01-07T21:50:58Z

One thing I thought of, where dooes the accounts_cache's max_flush_root get used? Anywhere in the foreground/transaction processing? That's the only place I can think of where delaying when set_max_flush_root() may make a difference.

There are 3 usages of max_flushed_root.

add_root()
flush_accounts_cache()
handle_snapshot_request()

2 and 3 are from the same account background thread. So this change should not
affect them.

1 is used from bank thread.

We can prove that any call of slot for add_root() will be greater than max_flushed_root.

Proof:

root will be strictly increasing when we call add_root().
max_flushed_root is computed from the split from w_maybe_flushed_roots
'w_maybe_flushed_roots' is populated by add_root().

Claim: Any slot 'S' that called on add_root() must be greater than the max slot in w_maybe_flushed_roots.
This is because 'w_maybe_flushed_roots' only contains previous slots, which are less than current slot 'S'.

Claim: max_flushed_root must be less than max slot in w_maybe_flushed_roots because of (2).

Therefore, S > max('w_maybe_flushed_roots') >= max_flushed_root.

Q.E.D

Therefore, 1 won't be affected by this change too.

brooksprumo

HaoranYi · 2025-01-09T00:14:49Z

I think current code is good under the aggressive flush condition.

Imaging if slot X is flushed before it rooted. What the current code does
is to remove it from the dashmap and create a storage for slot X. Note that
We don't update 'max_flushed_root' or 'maybe_unflushed_roots'. So logically
from the point view of 'max_flushed_root', this slot X is not flushed.

When X is rooted, then we will added to maybe_unflushed_roots. By the time
flush see X again, it will find out that it doesn't exist in dashmap -
already flushed, and don't need to flush. But we still need to update
max_flushed_root in the 'logical' sense. So logically max_flushed_root is
correct.

In essence, 'max_flushed_root' is tracking the max flush root 'logically' from
the request flush slot, not the actual slots flushed physically, which could
be due to by aggressive flush.

Because max_flushed_root is tracking the flushed_root "logically", the flush
of unrooted storage by aggressive flush doesn't matter here. As what have
proved in the PR, the optimization is correct in logical flushed_root sense. Therefore, I
think the optimization good.

@jeffwashington

HaoranYi · 2025-01-09T14:26:02Z

accounts-db/src/accounts_cache.rs

-        if root > max_flushed_root || (root == max_flushed_root && root == 0) {
-            self.maybe_unflushed_roots.write().unwrap().insert(root);
-        }
+        self.maybe_unflushed_roots.write().unwrap().insert(root);


Since we can prove that slot > max_flush_root, we don't need to check root > max_flush_root.
And since we remove the slot > max_flush_root check, there is no need to special case root == 0 for genesis block too.

we can prove that slot > max_flush_root

Is this because roots are monotonically increasing?

yes, exactly!

jeffwashington

lgtm
I believe I understand what is going on here and this looks correct.

carllin

Looks good, been a while since I've been in this area :)

carllin · 2025-01-10T06:08:20Z

accounts-db/src/accounts_db.rs

+        // Note that self.flush_slot_cache_with_clean() can return None if the
+        // slot is already been flushed. This can happen if the cache is
+        // overwhelmed and we flushed some yet to be rooted frozen slots.
+        // However, Independent of whether the last slot was actually flushed


nit Independent -> independent

yeah. I will change it in another pr so that people don't need to re-approve this one again.

carllin · 2025-01-10T06:11:32Z

accounts-db/src/accounts_cache.rs

-        if root > max_flushed_root || (root == max_flushed_root && root == 0) {
-            self.maybe_unflushed_roots.write().unwrap().insert(root);
-        }
+        self.maybe_unflushed_roots.write().unwrap().insert(root);


we can prove that slot > max_flush_root

Is this because roots are monotonically increasing?

HaoranYi changed the title ~~opt: fetch_max flush root~~ Optimize max_flushed_root update in flushing roots Jan 7, 2025

HaoranYi commented Jan 7, 2025

View reviewed changes

HaoranYi requested review from brooksprumo, jeffwashington and carllin January 7, 2025 16:23

brooksprumo reviewed Jan 7, 2025

View reviewed changes

optimize acchounts_cache max_root tracking in flush_roots

0352210

HaoranYi force-pushed the fetch_max branch from b8e0239 to 0352210 Compare January 7, 2025 17:50

HaoranYi requested a review from brooksprumo January 8, 2025 15:18

brooksprumo previously approved these changes Jan 8, 2025

View reviewed changes

better comments

1d6c78e

HaoranYi dismissed brooksprumo’s stale review via 1d6c78e January 9, 2025 00:16

remove max_root check

599f093

HaoranYi commented Jan 9, 2025

View reviewed changes

jeffwashington approved these changes Jan 9, 2025

View reviewed changes

carllin approved these changes Jan 10, 2025

View reviewed changes

HaoranYi merged commit cb9451c into anza-xyz:master Jan 10, 2025
40 checks passed

HaoranYi deleted the fetch_max branch January 10, 2025 16:07

HaoranYi mentioned this pull request Jan 10, 2025

chore: fix comments #4395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize max_flushed_root update in flushing roots #4320

Optimize max_flushed_root update in flushing roots #4320

HaoranYi commented Jan 7, 2025 •

edited

Loading

HaoranYi Jan 7, 2025

HaoranYi commented Jan 7, 2025

brooksprumo left a comment

HaoranYi commented Jan 7, 2025

brooksprumo left a comment

HaoranYi commented Jan 9, 2025 •

edited

Loading

HaoranYi Jan 9, 2025

carllin Jan 10, 2025

HaoranYi Jan 10, 2025

jeffwashington left a comment

carllin left a comment

carllin Jan 10, 2025

HaoranYi Jan 10, 2025

carllin Jan 10, 2025

Optimize max_flushed_root update in flushing roots #4320

Optimize max_flushed_root update in flushing roots #4320

Conversation

HaoranYi commented Jan 7, 2025 • edited Loading

Problem

Summary of Changes

HaoranYi Jan 7, 2025

Choose a reason for hiding this comment

HaoranYi commented Jan 7, 2025

brooksprumo left a comment

Choose a reason for hiding this comment

HaoranYi commented Jan 7, 2025

brooksprumo left a comment

Choose a reason for hiding this comment

HaoranYi commented Jan 9, 2025 • edited Loading

HaoranYi Jan 9, 2025

Choose a reason for hiding this comment

carllin Jan 10, 2025

Choose a reason for hiding this comment

HaoranYi Jan 10, 2025

Choose a reason for hiding this comment

jeffwashington left a comment

Choose a reason for hiding this comment

carllin left a comment

Choose a reason for hiding this comment

carllin Jan 10, 2025

Choose a reason for hiding this comment

HaoranYi Jan 10, 2025

Choose a reason for hiding this comment

carllin Jan 10, 2025

Choose a reason for hiding this comment

HaoranYi commented Jan 7, 2025 •

edited

Loading

HaoranYi commented Jan 9, 2025 •

edited

Loading