Stop caching sysvars, instead load them ahead of time by Lichtso · Pull Request #21108 · solana-labs/solana

Lichtso · 2021-11-01T10:12:34Z

Problem

#20881 will require the sysvars to be loaded AOT in order to avoid a dyn Trait after ThisInvokeContext is moved in the program-runtime crate.

Summary of Changes

Use get_program_accounts() in the bank to load the sysvars once per load_and_execute_transactions() instead of loading them on demand in ThisInvokeContext.

Fixes #

jackcmay · 2021-11-01T18:47:03Z

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

Lichtso · 2021-11-01T18:55:54Z

@jackcmay Yes, I was wondering what a better place to load them less often might be. Any idea?

Also, the biggest problem is Accounts::load_by_program() which is using accounts_db.scan_accounts(). That needs an alternative way of knowing / finding the active sysvar ids.

codecov · 2021-11-02T12:20:17Z

Codecov Report

Merging #21108 (37ba0db) into master (9ff92cb) will decrease coverage by 0.0%.
The diff coverage is 81.7%.

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

sakridge · 2021-11-02T17:58:47Z

        self.store_account_and_update_capitalization(pubkey, &new_account);
+
+        // Update the entry in the cache
+        let mut sysvar_cache = self.sysvar_cache.write().unwrap();


any idea of performance difference with this and the current code?

Nope, but good point. Shall I add a performance metric for it and let it run on a test validator?

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?

Not sure what you mean by "outside" of the RwLock.
Otherwise that is what fill_sysvar_cache() is doing.

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.

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.

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.

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.

jackcmay · 2021-11-03T16:33:59Z

        bank.update_rent();
        bank.update_epoch_schedule();
        bank.update_recent_blockhashes();
+        bank.fill_sysvar_cache();


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?

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

Yeah, that locking...

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?

It is specific because the cache is adding a lot ore locks, but yeah, we can optimize that out in another pr

Lichtso · 2021-11-03T22:01:59Z

Ok, I opened a separate follow-up issue.

@jackcmay @sakridge After resolving the merge conflicts and passing the CI tests, can I merge, or do you see further issues?

Pull request has been modified.

…1108)

…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>

#21108) (#22466)" This reverts commit 5d3f3bc.

…a-labs#21108)

… (#22572) * Bump version to v1.8.14 * Stop caching sysvars, instead load them ahead of time (backport #21108) Co-authored-by: Trent Nelson <trent@solana.com>

ryoqun · 2023-02-07T06:26:32Z

        new.update_stake_history(Some(parent_epoch));
        new.update_clock(Some(parent_epoch));
        new.update_fees();
+        new.fill_sysvar_cache();


here back-ref: #30158

Lichtso force-pushed the refactor/load_sysvars_ahead_of_time branch 7 times, most recently from 19d85e6 to 1feab67 Compare November 1, 2021 17:03

Lichtso force-pushed the refactor/load_sysvars_ahead_of_time branch 4 times, most recently from 1580f67 to c1f9663 Compare November 2, 2021 08:17

sakridge reviewed Nov 2, 2021

View reviewed changes

Comment thread runtime/src/bank.rs Outdated

sakridge reviewed Nov 2, 2021

View reviewed changes

Lichtso force-pushed the refactor/load_sysvars_ahead_of_time branch from c1f9663 to 9d6a1aa Compare November 3, 2021 10:13

jackcmay reviewed Nov 3, 2021

View reviewed changes

Lichtso force-pushed the refactor/load_sysvars_ahead_of_time branch from 9d6a1aa to d8bae18 Compare November 3, 2021 17:49

Lichtso mentioned this pull request Nov 3, 2021

Load and store sysvars less often #21158

Closed

Stop caching sysvars, instead load them ahead of time.

37ba0db

jackcmay previously approved these changes Nov 3, 2021

View reviewed changes

Lichtso force-pushed the refactor/load_sysvars_ahead_of_time branch from d8bae18 to 37ba0db Compare November 3, 2021 22:10

Lichtso merged commit 29ad081 into solana-labs:master Nov 4, 2021

Lichtso deleted the refactor/load_sysvars_ahead_of_time branch November 4, 2021 08:48

This was referenced Nov 4, 2021

Replaces MockInvokeContext by ThisInvokeContext in tests #20881

Merged

ABI v2 Proposal #19191

Closed

dankelleher pushed a commit to identity-com/solana that referenced this pull request Nov 24, 2021

Stop caching sysvars, instead load them ahead of time. (solana-labs#2…

c0efcf0

…1108)

frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021

Revert "Stop caching sysvars, instead load them ahead of time. (solan…

a667fcc

…a-labs#21108)" This reverts commit c0efcf0.

jstarry added the v1.8 label Jan 12, 2022

mergify Bot mentioned this pull request Jan 12, 2022

Stop caching sysvars, instead load them ahead of time (backport #21108) #22466

Merged

jstarry added a commit that referenced this pull request Jan 14, 2022

Revert "Stop caching sysvars, instead load them ahead of time (backport

304afd4

#21108) (#22466)" This reverts commit 5d3f3bc.

jstarry added a commit to jstarry/solana that referenced this pull request Jan 19, 2022

Stop caching sysvars, instead load them ahead of time (backport solan…

28d8608

…a-labs#21108)

jstarry added a commit to jstarry/solana that referenced this pull request Jan 19, 2022

Stop caching sysvars, instead load them ahead of time (backport solan…

9cd061f

…a-labs#21108)

ryoqun reviewed Feb 7, 2023

View reviewed changes

ryoqun mentioned this pull request Feb 7, 2023

Add address lookup tables to minimized snapshot #30158

Merged

Conversation

Lichtso commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

jackcmay commented Nov 1, 2021

Uh oh!

Lichtso commented Nov 1, 2021

Uh oh!

codecov Bot commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lichtso commented Nov 3, 2021

Uh oh!

ryoqun Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Lichtso commented Nov 1, 2021 •

edited

Loading

codecov Bot commented Nov 2, 2021 •

edited

Loading

ryoqun Feb 7, 2023 •

edited

Loading