-
Notifications
You must be signed in to change notification settings - Fork 238
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
Add default value for loaded-accounts-data-size #1355
base: master
Are you sure you want to change the base?
Add default value for loaded-accounts-data-size #1355
Conversation
e979dfd
to
f41a7b0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1355 +/- ##
=========================================
- Coverage 82.1% 82.1% -0.1%
=========================================
Files 886 886
Lines 236417 236424 +7
=========================================
- Hits 194266 194253 -13
- Misses 42151 42171 +20 |
using mod ledger-tool replayed several random mnb ledgers, the max loaded account size a transaction had is about 10MB, which is below Adding loaded_accounts_data_size, per transaction, to metrics seems too much in general, but perhaps can run it on canary? Or maybe @brooksprumo already have something like that collected? @bw-solana @apfitzge for visibility |
@KirillLykov has mentioned some data for understanding distribution of account sizes, but not sure if this was for all accounts or distribution for which ones are being accessed by transactions. If we don't have it, I would be in favor of some histogram for storing this information and maybe reporting per slot (mean, median, max) |
I have data for all the accounts sizes on mainnet, so I cannot say how much data one txs uses at max. |
Assuming accounts size data @KirillLykov collected were directly from mnb accounts db, then mean could be severely skewed by abandoned small accounts (eg accounts created by not used). Still very helpful info tho, thanks @KirillLykov |
The data I've published in the slack chat is obtained from the large file which has pubkeys for accounts (thanks @grooviegermanikus for collecting), so it might be possible to extract information about accounts last time have been used. If you are interested in this data, I can share it with you. |
I added histogram for loaded accounts size per tx, as @bw-solana suggested early, testing against an random mnb snapshot, gives this (in bytes):
mean + 1*stddev = 1,158,729; This doesn't line up with @KirillLykov per account data:
with 64 max accounts per tx, (350 + 6,000) * 64 = 406,400. I'd think this is mainly due to accounts actively used tend to have larger than average accounts size. |
Some more data point: for 432167 slots 251851770 to 252288093
for 9763 slots 245285309 to 245295300
|
f41a7b0
to
e5fad37
Compare
Given the data @KirillLykov published on slack, and histogram from mainnet-beta ledger (above), I'd like to propose the default to be somewhere at 95 percentile: 2MiB. With this default limit, we'd expect about 5-10% transactions to fail due to I prepared this write-up to facilitate dev-comm. |
6bf9c1a
to
d57dfd0
Compare
The problem statement states that there should be a reasonable default that is lesser than MAX, but I am not able to infer what specific problem this change solves. I looked in the attached write-up and found the comment:
Is that the root cause for the change? To more efficiently pack blocks that may currently be under-packed? |
Sort of. Right now, it doesn't matter because we don't factor in data bytes to block limits. |
Correct. To add more color, large default value will "over reserve" block space during packing (here), which could prevent other banking threads to schedule more transactions, especially close to the end of slot, potentially under pack block. |
// ComputeBudgetDetails does not concern with loaded_accounts_data_size_limit, hence safe | ||
// to hardcode its feature gate `default_loaded_accounts_data_size_limit` as activated | ||
let use_default_loaded_accounts_data_size = true; | ||
let compute_budget_limits = process_compute_budget_instructions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we change this variable to compute_budget_details
?
ComputeBudgetLimits
does in fact deal with the loaded account size; it's ComputeBudgetDetails
that doesn't.
(though this naming is confusing since I'd maybe intuitively think Details
has more details?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea think ..._deails
might be more confusing. process_compute_budget_instructions(...) -> Result< ComputeBudgetLimits, ...>
, ...._limits
reflects better that those are the limits we got from processing instructions.
… of set_loaded_accounts_data_size_limit
replace default() with new_with(feature_gate_status); add public getter with feature_gate status and making consts private;
plumb feature-gate status to process_compute_budget_instructions()
507ef56
to
8f9ea1e
Compare
Problem
Compute budget instruction
set_loaded_accounts_data_size_limit
was introduced since v1.16, transaction sender can use it to set limits of accounts to load for transaction. It usesMAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES = 64MiB
as default value ifset_loaded_accounts_data_size_limit
is absent from transaction during initial rollout, so no known transactions in mnb would fail without adapting the new instruction. It should use a reasonable default value that is lesser than MAX, as users start to adapting this instruction.Summary of Changes
DEFAULT_LOADED_ACCOUNTS_DATA_SIZE_BYTES
in absence ofset_loaded_accounts_data_size_limit
Feature Gate Issue: #1568