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

Clean up and fix PER stats #4898

Merged
merged 3 commits into from
Feb 12, 2025
Merged

Clean up and fix PER stats #4898

merged 3 commits into from
Feb 12, 2025

Conversation

HaoranYi
Copy link

@HaoranYi HaoranYi commented Feb 10, 2025

Problem

Now we enabled PER and deleted the old ER code path. There are a few stats that are missing from the PER code path.

load_vote_and_stake_accounts_us, hash reward partition timing and vote_accounts_cache_miss_count stats are not hooked up in PER code path.

Summary of Changes

  • Remove stale PER stat - load_vote_and_stake_accounts_us (no longer applicable)
  • fix hash reward partition timing stat
  • add back vote_accounts_cache_miss_count

Fixes #

@t-nelson
Copy link

are there equivalents to some of these in another datapoint?

@HaoranYi
Copy link
Author

HaoranYi commented Feb 11, 2025

are there equivalents to some of these in another datapoint?

load_vote_and_stake_accounts_us, vote_accounts_cache_miss_count are the stats
for old reward code before PER. These logics has been removed.

hash_partition_rewards_us was not poulated. Now this is fixed.

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.

Adding back in the hash_partition_rewards_us looks good to me.

For load_vote_and_stake_accounts_us and vote_accounts_cache_miss_count, can those be re-added too?

Looks like the get_vote_account lambda in calculate_stake_vote_rewards() could track the vote accounts cache misses.

I see that we get the stake accounts from the stakes cache, so that part is effectively 0. It looks like we could grab the vote account loading time though.

@HaoranYi
Copy link
Author

HaoranYi commented Feb 11, 2025

vote_accounts_cache_miss_count should always be zero. That stat was added originally to assert that we cache all the vote accounts. It is mainly used to debug, not a meaningful stat for performance. but i can add it back along with the vote/stake account loading time.

@brooksprumo
Copy link

brooksprumo commented Feb 11, 2025

vote_accounts_cache_miss_count should always be zero. That stat was added originally to assert that we cache all the vote accounts.

Yeah, that's what I gathered from the code too. Should we assert the vote accounts are in the cache now, and not allow a fallback to loading from accounts-db?

And for this PR, if we do want to assert the vote accounts are in the cache, then I agree it makes sense to remove these two metrics (load_vote_and_stake_accounts_us and vote_accounts_cache_miss_count).

@HaoranYi
Copy link
Author

HaoranYi commented Feb 11, 2025

I pushed a commit to add back vote_accounts_cache_miss_count in this PR. We could change it into an assert and get rid of the stat and account's db load fallback in a future PR.

I think load_vote_and_stake_accounts_us is no longer applicable. We load them from the cache. And that timing is already covered in measure_stake_rewards_us. So I keep it deleted.

@HaoranYi HaoranYi requested a review from brooksprumo February 11, 2025 16:47
@HaoranYi HaoranYi changed the title clean up PER stats Clean up and fix PER stats Feb 11, 2025
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.

:shipit:

Copy link

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

hash_partition_rewards_us was not poulated. Now this is fixed.

ah ok, this is the one i was looking for

@HaoranYi HaoranYi merged commit a7df2d5 into anza-xyz:master Feb 12, 2025
47 checks passed
@HaoranYi HaoranYi deleted the rm_stat branch February 12, 2025 15:47
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