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

Stop caching sysvars, instead load them ahead of time #21108

Merged
Lichtso merged 1 commit into from
Nov 4, 2021
Merged

Stop caching sysvars, instead load them ahead of time #21108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions programs/bpf/tests/programs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1400,9 +1400,9 @@ fn assert_instruction_count() {
("solana_bpf_rust_noop", 480),
("solana_bpf_rust_param_passing", 146),
("solana_bpf_rust_rand", 487),
("solana_bpf_rust_sanity", 8406),
("solana_bpf_rust_sanity", 8454),
("solana_bpf_rust_secp256k1_recover", 25216),
("solana_bpf_rust_sha", 30704),
("solana_bpf_rust_sha", 30692),
]);
}

Expand Down
24 changes: 8 additions & 16 deletions programs/bpf_loader/src/syscalls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3284,10 +3284,8 @@ mod tests {
let mut invoke_context = MockInvokeContext::new(&Pubkey::default(), vec![]);
let mut data = vec![];
bincode::serialize_into(&mut data, &src_clock).unwrap();
invoke_context
.get_sysvars()
.borrow_mut()
.push((sysvar::clock::id(), Some(Rc::new(data))));
let sysvars = &[(sysvar::clock::id(), data)];
invoke_context.sysvars = sysvars;

let mut syscall = SyscallGetClockSysvar {
invoke_context: Rc::new(RefCell::new(&mut invoke_context)),
Expand Down Expand Up @@ -3330,10 +3328,8 @@ mod tests {
let mut invoke_context = MockInvokeContext::new(&Pubkey::default(), vec![]);
let mut data = vec![];
bincode::serialize_into(&mut data, &src_epochschedule).unwrap();
invoke_context
.get_sysvars()
.borrow_mut()
.push((sysvar::epoch_schedule::id(), Some(Rc::new(data))));
let sysvars = &[(sysvar::epoch_schedule::id(), data)];
invoke_context.sysvars = sysvars;

let mut syscall = SyscallGetEpochScheduleSysvar {
invoke_context: Rc::new(RefCell::new(&mut invoke_context)),
Expand Down Expand Up @@ -3383,10 +3379,8 @@ mod tests {
let mut invoke_context = MockInvokeContext::new(&Pubkey::default(), vec![]);
let mut data = vec![];
bincode::serialize_into(&mut data, &src_fees).unwrap();
invoke_context
.get_sysvars()
.borrow_mut()
.push((sysvar::fees::id(), Some(Rc::new(data))));
let sysvars = &[(sysvar::fees::id(), data)];
invoke_context.sysvars = sysvars;

let mut syscall = SyscallGetFeesSysvar {
invoke_context: Rc::new(RefCell::new(&mut invoke_context)),
Expand Down Expand Up @@ -3427,10 +3421,8 @@ mod tests {
let mut invoke_context = MockInvokeContext::new(&Pubkey::default(), vec![]);
let mut data = vec![];
bincode::serialize_into(&mut data, &src_rent).unwrap();
invoke_context
.get_sysvars()
.borrow_mut()
.push((sysvar::rent::id(), Some(Rc::new(data))));
let sysvars = &[(sysvar::rent::id(), data)];
invoke_context.sysvars = sysvars;

let mut syscall = SyscallGetRentSysvar {
invoke_context: Rc::new(RefCell::new(&mut invoke_context)),
Expand Down
14 changes: 5 additions & 9 deletions programs/stake/src/stake_instruction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ mod tests {
},
sysvar::{stake_history::StakeHistory, Sysvar},
};
use std::{cell::RefCell, rc::Rc, str::FromStr};
use std::{cell::RefCell, str::FromStr};

fn create_default_account() -> RefCell<AccountSharedData> {
RefCell::new(AccountSharedData::default())
Expand Down Expand Up @@ -444,10 +444,8 @@ mod tests {
);
let mut data = Vec::with_capacity(sysvar::clock::Clock::size_of());
bincode::serialize_into(&mut data, &sysvar::clock::Clock::default()).unwrap();
invoke_context
.get_sysvars()
.borrow_mut()
.push((sysvar::clock::id(), Some(Rc::new(data))));
let sysvars = &[(sysvar::clock::id(), data)];
invoke_context.sysvars = sysvars;
super::process_instruction(1, &instruction.data, &mut invoke_context)
}
}
Expand Down Expand Up @@ -1102,10 +1100,8 @@ mod tests {
MockInvokeContext::new(&id(), create_keyed_accounts_unified(&keyed_accounts));
let mut data = Vec::with_capacity(sysvar::clock::Clock::size_of());
bincode::serialize_into(&mut data, &sysvar::clock::Clock::default()).unwrap();
invoke_context
.get_sysvars()
.borrow_mut()
.push((sysvar::clock::id(), Some(Rc::new(data))));
let sysvars = &[(sysvar::clock::id(), data)];
invoke_context.sysvars = sysvars;

assert_eq!(
super::process_instruction(
Expand Down
30 changes: 28 additions & 2 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1006,6 +1006,8 @@ pub struct Bank {
vote_only_bank: bool,

pub cost_tracker: RwLock<CostTracker>,

sysvar_cache: RwLock<Vec<(Pubkey, Vec<u8>)>>,
}

impl Default for BlockhashQueue {
Expand Down Expand Up @@ -1145,6 +1147,7 @@ impl Bank {
freeze_started: AtomicBool::default(),
vote_only_bank: false,
cost_tracker: RwLock::<CostTracker>::default(),
sysvar_cache: RwLock::new(Vec::new()),
}
}

Expand Down Expand Up @@ -1254,6 +1257,7 @@ impl Bank {
bank.update_rent();
bank.update_epoch_schedule();
bank.update_recent_blockhashes();
bank.fill_sysvar_cache();
Copy link
Copy Markdown
Contributor

@jackcmay jackcmay Nov 3, 2021

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?

Copy link
Copy Markdown
Contributor Author

@Lichtso Lichtso Nov 3, 2021

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

Copy link
Copy Markdown
Contributor

@jackcmay jackcmay Nov 3, 2021

Choose a reason for hiding this comment

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

Yeah, that locking...

Copy link
Copy Markdown
Contributor Author

@Lichtso Lichtso Nov 3, 2021

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?

Copy link
Copy Markdown
Contributor

@jackcmay jackcmay Nov 3, 2021

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

bank
}

Expand Down Expand Up @@ -1402,6 +1406,7 @@ impl Bank {
)),
freeze_started: AtomicBool::new(false),
cost_tracker: RwLock::new(CostTracker::default()),
sysvar_cache: RwLock::new(Vec::new()),
};

datapoint_info!(
Expand Down Expand Up @@ -1458,6 +1463,7 @@ impl Bank {
new.update_stake_history(Some(parent_epoch));
new.update_clock(Some(parent_epoch));
new.update_fees();
new.fill_sysvar_cache();

return new;
}
Expand All @@ -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();
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

here back-ref: #30158

new
}

Expand Down Expand Up @@ -1589,6 +1596,7 @@ impl Bank {
freeze_started: AtomicBool::new(fields.hash != Hash::default()),
vote_only_bank: false,
cost_tracker: RwLock::new(CostTracker::default()),
sysvar_cache: RwLock::new(Vec::new()),
};
bank.finish_init(
genesis_config,
Expand Down Expand Up @@ -1767,6 +1775,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();
Copy link
Copy Markdown
Contributor

@sakridge sakridge Nov 2, 2021

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?

Copy link
Copy Markdown
Contributor Author

@Lichtso Lichtso Nov 2, 2021

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?

Copy link
Copy Markdown
Contributor

@jackcmay jackcmay Nov 2, 2021

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?

Copy link
Copy Markdown
Contributor Author

@Lichtso Lichtso Nov 2, 2021

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.

Copy link
Copy Markdown
Contributor

@jackcmay jackcmay Nov 2, 2021

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.

Copy link
Copy Markdown
Contributor

@sakridge sakridge Nov 3, 2021

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.

Copy link
Copy Markdown
Contributor Author

@Lichtso Lichtso Nov 3, 2021

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.

Copy link
Copy Markdown
Contributor

@sakridge sakridge Nov 3, 2021

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.

if let Some(position) = sysvar_cache.iter().position(|(id, _data)| id == pubkey) {
sysvar_cache[position].1 = new_account.data().to_vec();
} else {
sysvar_cache.push((*pubkey, new_account.data().to_vec()));
}
}

fn inherit_specially_retained_account_fields(
Expand Down Expand Up @@ -1904,6 +1920,17 @@ impl Bank {
});
}

fn fill_sysvar_cache(&mut self) {
let mut sysvar_cache = self.sysvar_cache.write().unwrap();
for id in sysvar::ALL_IDS.iter() {
if !sysvar_cache.iter().any(|(key, _data)| key == id) {
if let Some(account) = self.get_account_with_fixed_root(id) {
sysvar_cache.push((*id, account.data().to_vec()));
}
}
}
}

pub fn get_slot_history(&self) -> SlotHistory {
from_account(&self.get_account(&sysvar::slot_history::id()).unwrap()).unwrap()
}
Expand Down Expand Up @@ -3858,8 +3885,7 @@ impl Bank {
compute_budget,
compute_meter,
&mut timings.details,
self.rc.accounts.clone(),
&self.ancestors,
&*self.sysvar_cache.read().unwrap(),
blockhash,
lamports_per_signature,
);
Expand Down
Loading