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
Stop caching sysvars, instead load them ahead of time #21108
Stop caching sysvars, instead load them ahead of time #21108
Conversation
19d85e6
to
1feab67
Compare
With this change all transactions will load all the sysvars even the transaction does not need them. Some of these sysvars could probably be loaded/cached once per slot |
@jackcmay Yes, I was wondering what a better place to load them less often might be. Any idea? Also, the biggest problem is |
1580f67
to
c1f9663
Compare
Codecov Report
@@ Coverage Diff @@
## master #21108 +/- ##
=========================================
- Coverage 81.5% 81.5% -0.1%
=========================================
Files 499 499
Lines 140213 140288 +75
=========================================
+ Hits 114319 114335 +16
- Misses 25894 25953 +59 |
@@ -1768,6 +1776,14 @@ impl Bank { | |||
} | |||
|
|||
self.store_account_and_update_capitalization(pubkey, &new_account); | |||
|
|||
// Update the entry in the cache | |||
let mut sysvar_cache = self.sysvar_cache.write().unwrap(); |
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.
any idea of performance difference with this and the current code?
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.
Nope, but good point. Shall I add a performance metric for it and let it run on a test validator?
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.
Maybe we don't need this to be generic, we update all the sysvars during bank init, maybe add all the relevant sysvars in one action outside of the rw lock?
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.
Not sure what you mean by "outside" of the RwLock
.
Otherwise that is what fill_sysvar_cache()
is doing.
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.
In new_with_paths
for example, we store away sysvars (which updates the cache), and then we again update the cache (and read accounts) with some of the same sysvars in fill_sysvar_cache
.
@sakridge I'm also wondering why in general we update the sysvars during bank creation, why not create bank-specific updated ones, cache them, use the cache to get all sysvars, and then later store them back during bank freeze.
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.
why in general we update the sysvars during bank creation, why not create bank-specific updated ones, cache them, use the cache to get all sysvars, and then later store them back during bank freeze.
That could be an option. I think it was just more straightforward to store them in accountsdb and load it like other accounts. It also improves as accountsdb improves with in-memory caching and flushing to disk and things like that.
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 open a separate issue for that and handle it in a different PR?
Because this PR is already the third level of recursion, so to speak, for me.
@sakridge Coming back to the question of the performance: It might be hard to measure the time spent there directly, but we could count how often it used to load the accounts and how often it updates them now. And then at least have that as a comparison.
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.
Ok. Yea if it's difficult then it's in the noise and we don't need to worry about it. Just wondering what the cost is relative to other bank creation functions are. I wrote this benchmark that just creates banks: #20361
Also, just running the bench-tps performance test and seeing that it doesn't affect it, although ideally I think we might want to see more transactions which touch the sysvars.
c1f9663
to
9d6a1aa
Compare
@@ -1260,6 +1263,7 @@ impl Bank { | |||
bank.update_rent(); | |||
bank.update_epoch_schedule(); | |||
bank.update_recent_blockhashes(); | |||
bank.fill_sysvar_cache(); |
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.
Here, we are saving the accounts to the db in the "update" calls, and then immediately reloading them to populate the cache. Each of these operations requires an accountsdb lock. Can we instead combine these into a more efficient operation?
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.
fill_sysvar_cache()
only tries to load them if they are not found in the cache already. So the fill happens after all sysvar updates deliberately.
if !sysvar_cache.iter().any(|(key, _data)| key == id) {
So the inefficiency I can see here is:
- The linear search in the
sysvar_cache
- And that each
update_sysvar_account()
call reacquires the locks
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.
Yeah, that locking...
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.
I agree that it probably can be optimized. But that is not a change specific to this PR. So same question as above:
Can we open a separate issue for that and handle it in a different PR?
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.
It is specific because the cache is adding a lot ore locks, but yeah, we can optimize that out in another pr
9d6a1aa
to
d8bae18
Compare
d8bae18
to
37ba0db
Compare
Pull request has been modified.
…a-labs#21108)" This reverts commit c0efcf0.
(cherry picked from commit 29ad081) # Conflicts: # programs/bpf/tests/programs.rs # programs/bpf_loader/src/syscalls.rs # programs/stake/src/stake_instruction.rs # runtime/src/bank.rs # runtime/src/message_processor.rs # sdk/program/src/sysvar/mod.rs # sdk/src/process_instruction.rs
… (#22466) * Stop caching sysvars, instead load them ahead of time. (#21108) (cherry picked from commit 29ad081) # Conflicts: # programs/bpf/tests/programs.rs # programs/bpf_loader/src/syscalls.rs # programs/stake/src/stake_instruction.rs # runtime/src/bank.rs # runtime/src/message_processor.rs # sdk/program/src/sysvar/mod.rs # sdk/src/process_instruction.rs * resolve conflicts Co-authored-by: Alexander Meißner <AlexanderMeissner@gmx.net> Co-authored-by: Justin Starry <justin@solana.com>
@@ -1473,6 +1479,7 @@ impl Bank { | |||
new.update_stake_history(Some(parent_epoch)); | |||
new.update_clock(Some(parent_epoch)); | |||
new.update_fees(); | |||
new.fill_sysvar_cache(); |
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.
here back-ref: #30158
Problem
#20881 will require the
sysvars
to be loaded AOT in order to avoid adyn Trait
afterThisInvokeContext
is moved in the program-runtime crate.Summary of Changes
Use
get_program_accounts()
in the bank to load thesysvars
once perload_and_execute_transactions()
instead of loading them on demand inThisInvokeContext
.Fixes #