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

Optimize max_flushed_root update in flushing roots #4320

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HaoranYi
Copy link

@HaoranYi HaoranYi commented Jan 7, 2025

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 HaoranYi changed the title opt: fetch_max flush root Optimize max_flushed_root update in flushing roots Jan 7, 2025
@@ -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);
Copy link
Author

Choose a reason for hiding this comment

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

rename: cached_roots -> flushed_roots for clarity.

@HaoranYi
Copy link
Author

HaoranYi commented Jan 7, 2025

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

Copy link

@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.

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
Copy link
Author

HaoranYi commented Jan 7, 2025

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.

  1. add_root()
  2. flush_accounts_cache()
  3. 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:

  1. root will be strictly increasing when we call add_root().
  2. max_flushed_root is computed from the split from w_maybe_flushed_roots
  3. '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.

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.

2 participants