Skip to content

Commit

Permalink
SVM: remove usage of program cache from load_program_with_pubkey() (a…
Browse files Browse the repository at this point in the history
…nza-xyz#1582)

* SVM: remove usage of program cache from Bank::load_program()

* return value instead of reference from get_environments_for_epoch()
pgarg66 authored Jun 4, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent e9d5e69 commit 9830fb6
Showing 6 changed files with 37 additions and 71 deletions.
8 changes: 4 additions & 4 deletions program-runtime/src/loaded_programs.rs
Original file line number Diff line number Diff line change
@@ -707,7 +707,7 @@ impl ProgramCacheForTxBatch {
Self {
entries: HashMap::new(),
slot,
environments: cache.get_environments_for_epoch(epoch).clone(),
environments: cache.get_environments_for_epoch(epoch),
upcoming_environments: cache.get_upcoming_environments_for_epoch(epoch),
latest_root_epoch: cache.latest_root_epoch,
hit_max_limit: false,
@@ -805,13 +805,13 @@ impl<FG: ForkGraph> ProgramCache<FG> {
}

/// Returns the current environments depending on the given epoch
pub fn get_environments_for_epoch(&self, epoch: Epoch) -> &ProgramRuntimeEnvironments {
pub fn get_environments_for_epoch(&self, epoch: Epoch) -> ProgramRuntimeEnvironments {
if epoch != self.latest_root_epoch {
if let Some(upcoming_environments) = self.upcoming_environments.as_ref() {
return upcoming_environments;
return upcoming_environments.clone();
}
}
&self.environments
self.environments.clone()
}

/// Returns the upcoming environments depending on the given epoch
7 changes: 4 additions & 3 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
@@ -6746,13 +6746,14 @@ impl Bank {
reload: bool,
effective_epoch: Epoch,
) -> Option<Arc<ProgramCacheEntry>> {
let program_cache = self.transaction_processor.program_cache.read().unwrap();
let environments = self
.transaction_processor
.get_environments_for_epoch(effective_epoch);
load_program_with_pubkey(
self,
&program_cache,
&environments,
pubkey,
self.slot(),
effective_epoch,
self.epoch_schedule(),
reload,
)
6 changes: 2 additions & 4 deletions runtime/src/bank/tests.rs
Original file line number Diff line number Diff line change
@@ -12006,16 +12006,14 @@ fn test_feature_activation_loaded_programs_cache_preparation_phase() {
.read()
.unwrap()
.get_environments_for_epoch(0)
.program_runtime_v1
.clone();
.program_runtime_v1;
let upcoming_env = bank
.transaction_processor
.program_cache
.read()
.unwrap()
.get_environments_for_epoch(1)
.program_runtime_v1
.clone();
.program_runtime_v1;

// Advance the bank to recompile the program.
{
66 changes: 15 additions & 51 deletions svm/src/program_loader.rs
Original file line number Diff line number Diff line change
@@ -2,8 +2,8 @@ use {
crate::transaction_processing_callback::TransactionProcessingCallback,
solana_program_runtime::{
loaded_programs::{
ForkGraph, LoadProgramMetrics, ProgramCache, ProgramCacheEntry, ProgramCacheEntryOwner,
ProgramCacheEntryType, ProgramRuntimeEnvironment, DELAY_VISIBILITY_SLOT_OFFSET,
LoadProgramMetrics, ProgramCacheEntry, ProgramCacheEntryOwner, ProgramCacheEntryType,
ProgramRuntimeEnvironment, ProgramRuntimeEnvironments, DELAY_VISIBILITY_SLOT_OFFSET,
},
timings::ExecuteDetailsTimings,
},
@@ -12,7 +12,7 @@ use {
account_utils::StateMut,
bpf_loader, bpf_loader_deprecated,
bpf_loader_upgradeable::UpgradeableLoaderState,
clock::{Epoch, Slot},
clock::Slot,
epoch_schedule::EpochSchedule,
instruction::InstructionError,
loader_v4::{self, LoaderV4State, LoaderV4Status},
@@ -121,16 +121,14 @@ pub(crate) fn load_program_accounts<CB: TransactionProcessingCallback>(
/// If the account doesn't exist it returns `None`. If the account does exist, it must be a program
/// account (belong to one of the program loaders). Returns `Some(InvalidAccountData)` if the program
/// account is `Closed`, contains invalid data or any of the programdata accounts are invalid.
pub fn load_program_with_pubkey<CB: TransactionProcessingCallback, FG: ForkGraph>(
pub fn load_program_with_pubkey<CB: TransactionProcessingCallback>(
callbacks: &CB,
program_cache: &ProgramCache<FG>,
environments: &ProgramRuntimeEnvironments,
pubkey: &Pubkey,
slot: Slot,
effective_epoch: Epoch,
_epoch_schedule: &EpochSchedule,
reload: bool,
) -> Option<Arc<ProgramCacheEntry>> {
let environments = program_cache.get_environments_for_epoch(effective_epoch);
let mut load_program_metrics = LoadProgramMetrics {
program_id: pubkey.to_string(),
..LoadProgramMetrics::default()
@@ -227,7 +225,7 @@ mod tests {
super::*,
crate::transaction_processor::TransactionBatchProcessor,
solana_program_runtime::{
loaded_programs::{BlockRelation, ProgramRuntimeEnvironments},
loaded_programs::{BlockRelation, ForkGraph, ProgramRuntimeEnvironments},
solana_rbpf::program::BuiltinProgram,
},
solana_sdk::{
@@ -507,14 +505,12 @@ mod tests {
let mock_bank = MockBankCallback::default();
let key = Pubkey::new_unique();
let batch_processor = TransactionBatchProcessor::<TestForkGraph>::default();
let program_cache = batch_processor.program_cache.read().unwrap();

let result = load_program_with_pubkey(
&mock_bank,
&program_cache,
&batch_processor.get_environments_for_epoch(50),
&key,
500,
50,
&batch_processor.epoch_schedule,
false,
);
@@ -533,14 +529,11 @@ mod tests {
.borrow_mut()
.insert(key, account_data.clone());

let program_cache = batch_processor.program_cache.read().unwrap();

let result = load_program_with_pubkey(
&mock_bank,
&program_cache,
&batch_processor.get_environments_for_epoch(20),
&key,
0, // Slot 0
20,
&batch_processor.epoch_schedule,
false,
);
@@ -550,11 +543,7 @@ mod tests {
ProgramCacheEntryOwner::LoaderV4,
ProgramCacheEntryType::FailedVerification(
batch_processor
.program_cache
.read()
.unwrap()
.get_environments_for_epoch(20)
.clone()
.program_runtime_v1,
),
);
@@ -573,15 +562,12 @@ mod tests {
.borrow_mut()
.insert(key, account_data.clone());

let program_cache = batch_processor.program_cache.read().unwrap();

// This should return an error
let result = load_program_with_pubkey(
&mock_bank,
&program_cache,
&batch_processor.get_environments_for_epoch(20),
&key,
200,
20,
&batch_processor.epoch_schedule,
false,
);
@@ -590,11 +576,7 @@ mod tests {
ProgramCacheEntryOwner::LoaderV2,
ProgramCacheEntryType::FailedVerification(
batch_processor
.program_cache
.read()
.unwrap()
.get_environments_for_epoch(20)
.clone()
.program_runtime_v1,
),
);
@@ -610,10 +592,9 @@ mod tests {

let result = load_program_with_pubkey(
&mock_bank,
&program_cache,
&batch_processor.get_environments_for_epoch(20),
&key,
200,
20,
&batch_processor.epoch_schedule,
false,
);
@@ -638,7 +619,6 @@ mod tests {
let key2 = Pubkey::new_unique();
let mock_bank = MockBankCallback::default();
let batch_processor = TransactionBatchProcessor::<TestForkGraph>::default();
let program_cache = batch_processor.program_cache.read().unwrap();

let mut account_data = AccountSharedData::default();
account_data.set_owner(bpf_loader_upgradeable::id());
@@ -666,10 +646,9 @@ mod tests {
// This should return an error
let result = load_program_with_pubkey(
&mock_bank,
&program_cache,
&batch_processor.get_environments_for_epoch(0),
&key1,
0,
0,
&batch_processor.epoch_schedule,
false,
);
@@ -678,11 +657,7 @@ mod tests {
ProgramCacheEntryOwner::LoaderV3,
ProgramCacheEntryType::FailedVerification(
batch_processor
.program_cache
.read()
.unwrap()
.get_environments_for_epoch(0)
.clone()
.program_runtime_v1,
),
);
@@ -708,10 +683,9 @@ mod tests {

let result = load_program_with_pubkey(
&mock_bank,
&program_cache,
&batch_processor.get_environments_for_epoch(20),
&key1,
200,
20,
&batch_processor.epoch_schedule,
false,
);
@@ -740,7 +714,6 @@ mod tests {
let mut account_data = AccountSharedData::default();
account_data.set_owner(loader_v4::id());
let batch_processor = TransactionBatchProcessor::<TestForkGraph>::default();
let program_cache = batch_processor.program_cache.read().unwrap();

let loader_data = LoaderV4State {
slot: 0,
@@ -760,10 +733,9 @@ mod tests {

let result = load_program_with_pubkey(
&mock_bank,
&program_cache,
&batch_processor.get_environments_for_epoch(0),
&key,
0,
0,
&batch_processor.epoch_schedule,
false,
);
@@ -772,11 +744,7 @@ mod tests {
ProgramCacheEntryOwner::LoaderV4,
ProgramCacheEntryType::FailedVerification(
batch_processor
.program_cache
.read()
.unwrap()
.get_environments_for_epoch(0)
.clone()
.program_runtime_v1,
),
);
@@ -798,10 +766,9 @@ mod tests {

let result = load_program_with_pubkey(
&mock_bank,
&program_cache,
&batch_processor.get_environments_for_epoch(20),
&key,
200,
20,
&batch_processor.epoch_schedule,
false,
);
@@ -845,15 +812,12 @@ mod tests {
.borrow_mut()
.insert(key, account_data.clone());

let program_cache = batch_processor.program_cache.read().unwrap();

for is_upcoming_env in [false, true] {
let result = load_program_with_pubkey(
&mock_bank,
&program_cache,
&batch_processor.get_environments_for_epoch(is_upcoming_env as u64),
&key,
200,
is_upcoming_env as u64,
&batch_processor.epoch_schedule,
false,
)
18 changes: 11 additions & 7 deletions svm/src/transaction_processor.rs
Original file line number Diff line number Diff line change
@@ -22,7 +22,7 @@ use {
invoke_context::{EnvironmentConfig, InvokeContext},
loaded_programs::{
ForkGraph, ProgramCache, ProgramCacheEntry, ProgramCacheForTxBatch,
ProgramCacheMatchCriteria,
ProgramCacheMatchCriteria, ProgramRuntimeEnvironments,
},
log_collector::LogCollector,
sysvar_cache::SysvarCache,
@@ -191,6 +191,14 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
}
}

/// Returns the current environments depending on the given epoch
pub fn get_environments_for_epoch(&self, epoch: Epoch) -> ProgramRuntimeEnvironments {
self.program_cache
.read()
.unwrap()
.get_environments_for_epoch(epoch)
}

/// Main entrypoint to the SVM.
pub fn load_and_execute_sanitized_transactions<CB: TransactionProcessingCallback>(
&self,
@@ -416,10 +424,9 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
// Load, verify and compile one program.
let program = load_program_with_pubkey(
callback,
&program_cache,
&self.get_environments_for_epoch(self.epoch),
&key,
self.slot,
self.epoch,
&self.epoch_schedule,
false,
)
@@ -481,17 +488,14 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
if let Some((key, program_to_recompile)) = program_cache.programs_to_recompile.pop() {
let effective_epoch = program_cache.latest_root_epoch.saturating_add(1);
drop(program_cache);
let program_cache_read = self.program_cache.read().unwrap();
if let Some(recompiled) = load_program_with_pubkey(
callbacks,
&program_cache_read,
&self.get_environments_for_epoch(effective_epoch),
&key,
self.slot,
effective_epoch,
&self.epoch_schedule,
false,
) {
drop(program_cache_read);
recompiled
.tx_usage_counter
.fetch_add(program_to_recompile.tx_usage_counter.load(Relaxed), Relaxed);
3 changes: 1 addition & 2 deletions svm/tests/conformance.rs
Original file line number Diff line number Diff line change
@@ -439,10 +439,9 @@ fn execute_fixture_as_instr(

let loaded_program = program_loader::load_program_with_pubkey(
mock_bank,
&batch_processor.program_cache.read().unwrap(),
&batch_processor.get_environments_for_epoch(2),
&program_id,
42,
2,
&batch_processor.epoch_schedule,
false,
)

0 comments on commit 9830fb6

Please sign in to comment.