Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Adds transaction metrics for accounts & data loaded/stored#34718

Closed
brooksprumo wants to merge 1 commit into
solana-labs:masterfrom
brooksprumo:tx-metrics-load-store-bytes
Closed

Adds transaction metrics for accounts & data loaded/stored#34718
brooksprumo wants to merge 1 commit into
solana-labs:masterfrom
brooksprumo:tx-metrics-load-store-bytes

Conversation

@brooksprumo
Copy link
Copy Markdown
Contributor

@brooksprumo brooksprumo commented Jan 9, 2024

Problem

While processing transactions, we track metrics related to how long it takes to load and store accounts, but not the number of accounts1 nor the amount of account data.

Summary of Changes

Add metrics for the number of accounts and amount of account data loaded and stored during transaction processing.

Here's an example of the new metrics:

number of accounts
number of accounts

account data
account data

Footnotes

  1. Caveat: We do track the number of accounts that are passed to message processing, "execute_details_total_account_count", which matches the number of accounts loaded. So this PR adds a currently-redundant datapoint for number of loaded accounts. I can remove that if desired. Is there something similar on the store-side? I.e. a number-of-modified-accounts-to-be-stored?

@brooksprumo brooksprumo self-assigned this Jan 9, 2024
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 9, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (c6c2340) 81.6% compared to head (001acc4) 81.6%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34718   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         830      830           
  Lines      224837   224866   +29     
=======================================
+ Hits       183505   183564   +59     
+ Misses      41332    41302   -30     

@brooksprumo brooksprumo marked this pull request as ready for review January 10, 2024 13:32
Comment thread runtime/src/bank.rs Outdated
Copy link
Copy Markdown
Contributor

@alessandrod alessandrod Jan 11, 2024

Choose a reason for hiding this comment

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

nit: I'd prefer to avoid the extra loop and instead compute the value in load_accounts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Fixed in 6948ff3.

Copy link
Copy Markdown
Contributor Author

@brooksprumo brooksprumo Jan 12, 2024

Choose a reason for hiding this comment

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

And changed in 006ff6c.

Comment thread accounts-db/src/accounts.rs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is a fn maybe in append vec or accounts that tells you the total bytes, including owner, etc. This is only summing data len(), which is different than bytes stored.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a good point. I was originally intending to track only the account data, but maybe tracking all the loaded/stored bytes would be better? Or maybe do both? Wdyt?

Copy link
Copy Markdown
Contributor Author

@brooksprumo brooksprumo Jan 30, 2024

Choose a reason for hiding this comment

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

Thinking about this more, I actually do want the account data size for the datapoint itself. I've renamed the datapoint to hopefully be more clear that this is the account data and not all the bytes.

We'll also now have the number of accounts stored, so the metadata bytes could be added to someone's query, if they'd like.

@jeffwashington jeffwashington self-requested a review January 11, 2024 19:55
@brooksprumo brooksprumo force-pushed the tx-metrics-load-store-bytes branch from 5c1757c to 006ff6c Compare January 12, 2024 00:20
@github-actions github-actions Bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Jan 26, 2024
@brooksprumo brooksprumo force-pushed the tx-metrics-load-store-bytes branch from 006ff6c to 58267ae Compare January 30, 2024 18:08
@brooksprumo brooksprumo changed the title Adds transaction metrics for number of bytes loaded and stored Adds transaction metrics for accounts & data loaded/stored Jan 30, 2024
@brooksprumo brooksprumo force-pushed the tx-metrics-load-store-bytes branch from 58267ae to 001acc4 Compare January 30, 2024 18:39
@brooksprumo
Copy link
Copy Markdown
Contributor Author

Rebased to address merge conflicts.

StoredAccountsInfo {
count: accounts_to_store.len() as u64,
data_bytes: accounts_to_store
.iter()
Copy link
Copy Markdown
Contributor

@jeffwashington jeffwashington Feb 13, 2024

Choose a reason for hiding this comment

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

we iterate these and sum data len here and accumulate a stat:

self.stats
.store_total_data
.fetch_add(total_data as u64, Ordering::Relaxed);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah nice. I'd like to get these stats per slot though. IIUC, this stat will be per second.

Also maybe getting the stored amount per slot is not actually needed? It does feel nice to exactly correlate any replay load/store time increases to the number of accounts or the amount of data read/written though.

@willhickey
Copy link
Copy Markdown
Contributor

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants