Skip to content

Commit 5db07ad

Browse files
LichtsoLichtso
authored
Revert "refactor: non-functional program-runtime changes for clarity (anza-xyz#10510)" This reverts commit ad004b7.
1 parent cd9665e commit 5db07ad

File tree

3 files changed

+91
-95
lines changed

3 files changed

+91
-95
lines changed

‎program-runtime/src/loaded_programs.rs‎

Lines changed: 19 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -849,7 +849,7 @@ impl<FG: ForkGraph> ProgramCache<FG> {
849849
match &mut self.index {
850850
IndexImplementation::V1 { entries, .. } => {
851851
let slot_versions = &mut entries.entry(key).or_default();
852-
let insertion_point = slot_versions.binary_search_by(|at| {
852+
match slot_versions.binary_search_by(|at| {
853853
at.effective_slot
854854
.cmp(&entry.effective_slot)
855855
.then(at.deployment_slot.cmp(&entry.deployment_slot))
@@ -866,8 +866,7 @@ impl<FG: ForkGraph> ProgramCache<FG> {
866866
entry.program.get_environment(),
867867
)),
868868
)
869-
});
870-
match insertion_point {
869+
}) {
871870
Ok(index) => {
872871
let existing = slot_versions.get_mut(index).unwrap();
873872
match (&existing.program, &entry.program) {
@@ -905,8 +904,7 @@ impl<FG: ForkGraph> ProgramCache<FG> {
905904
slot_versions.insert(index, Arc::clone(&entry));
906905
}
907906
}
908-
// Remove existing entries in the same deployment slot unless they are for a different
909-
// environment.
907+
// Remove existing entries in the same deployment slot unless they are for a different environment.
910908
// This overwrites the current status of a program in program management instructions.
911909
slot_versions.retain(|existing| {
912910
existing.deployment_slot != entry.deployment_slot
@@ -971,13 +969,12 @@ impl<FG: ForkGraph> ProgramCache<FG> {
971969
first_ancestor_env = entry.program.get_environment();
972970
return true;
973971
}
974-
// Do not prune the entry if the runtime environment of the entry is
975-
// different than the entry that was previously found (stored in
976-
// first_ancestor_env). Different environment indicates that this entry
977-
// might belong to an older epoch that had a different environment (e.g.
978-
// different feature set). Once the root moves to the new/current epoch,
979-
// the entry will get pruned. But, until then the entry might still be
980-
// getting used by an older slot.
972+
// Do not prune the entry if the runtime environment of the entry is different
973+
// than the entry that was previously found (stored in first_ancestor_env).
974+
// Different environment indicates that this entry might belong to an older
975+
// epoch that had a different environment (e.g. different feature set).
976+
// Once the root moves to the new/current epoch, the entry will get pruned.
977+
// But, until then the entry might still be getting used by an older slot.
981978
if let Some(entry_env) = entry.program.get_environment()
982979
&& let Some(env) = first_ancestor_env
983980
&& !Arc::ptr_eq(entry_env, env)
@@ -1061,36 +1058,28 @@ impl<FG: ForkGraph> ProgramCache<FG> {
10611058
if let Some(second_level) = entries.get(key) {
10621059
let mut filter_by_deployment_slot = None;
10631060
for entry in second_level.iter().rev() {
1064-
let required_deployment_slot =
1065-
filter_by_deployment_slot.unwrap_or(entry.deployment_slot);
1066-
if required_deployment_slot != entry.deployment_slot {
1061+
if filter_by_deployment_slot
1062+
.map(|slot| slot != entry.deployment_slot)
1063+
.unwrap_or(false)
1064+
{
10671065
continue;
10681066
}
1069-
let entry_in_same_branch = entry.deployment_slot
1070-
<= self.latest_root_slot
1067+
if entry.deployment_slot <= self.latest_root_slot
10711068
|| matches!(
10721069
locked_fork_graph.relationship(
10731070
entry.deployment_slot,
10741071
loaded_programs_for_tx_batch.slot
10751072
),
10761073
BlockRelation::Equal | BlockRelation::Ancestor
1077-
);
1078-
if entry_in_same_branch {
1079-
let entry_is_effective =
1080-
loaded_programs_for_tx_batch.slot >= entry.effective_slot;
1081-
let entry_to_return = if entry_is_effective {
1074+
)
1075+
{
1076+
let entry_to_return = if loaded_programs_for_tx_batch.slot
1077+
>= entry.effective_slot
1078+
{
10821079
if !Self::matches_environment(
10831080
entry,
10841081
program_runtime_environments_for_execution,
10851082
) {
1086-
// We found an entry that would work, had its environment matched
1087-
// the one we're planning to use for this slot.
1088-
//
1089-
// At this point we know that whatever the "current version" of
1090-
// program is, it must have had a deployment slot equal to the
1091-
// program we're looking at in this iteration. We just have to find
1092-
// one with the correct environment and can skip entries for any
1093-
// other deployment slot while searching further.
10941083
filter_by_deployment_slot = filter_by_deployment_slot
10951084
.or(Some(entry.deployment_slot));
10961085
continue;

‎svm/src/program_loader.rs‎

Lines changed: 70 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use {
88
solana_loader_v4_interface::state::{LoaderV4State, LoaderV4Status},
99
solana_program_runtime::loaded_programs::{
1010
DELAY_VISIBILITY_SLOT_OFFSET, ProgramCacheEntry, ProgramCacheEntryOwner,
11-
ProgramCacheEntryType, ProgramRuntimeEnvironments,
11+
ProgramCacheEntryType, ProgramRuntimeEnvironment, ProgramRuntimeEnvironments,
1212
},
1313
solana_pubkey::Pubkey,
1414
solana_sdk_ids::{bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable, loader_v4},
@@ -27,6 +27,26 @@ pub(crate) enum ProgramAccountLoadResult {
2727
ProgramOfLoaderV4(AccountSharedData, Slot),
2828
}
2929

30+
pub(crate) fn load_program_from_bytes(
31+
#[cfg(feature = "metrics")] load_program_metrics: &mut LoadProgramMetrics,
32+
programdata: &[u8],
33+
loader_key: &Pubkey,
34+
account_size: usize,
35+
deployment_slot: Slot,
36+
program_runtime_environment: ProgramRuntimeEnvironment,
37+
) -> std::result::Result<ProgramCacheEntry, Box<dyn std::error::Error>> {
38+
ProgramCacheEntry::new(
39+
loader_key,
40+
program_runtime_environment,
41+
deployment_slot,
42+
deployment_slot.saturating_add(DELAY_VISIBILITY_SLOT_OFFSET),
43+
programdata,
44+
account_size,
45+
#[cfg(feature = "metrics")]
46+
load_program_metrics,
47+
)
48+
}
49+
3050
pub(crate) fn load_program_accounts<CB: TransactionProcessingCallback>(
3151
callbacks: &CB,
3252
pubkey: &Pubkey,
@@ -107,27 +127,25 @@ pub fn load_program_with_pubkey<CB: TransactionProcessingCallback>(
107127
ProgramCacheEntry::new_tombstone(current_slot, owner, ProgramCacheEntryType::Closed),
108128
),
109129

110-
ProgramAccountLoadResult::ProgramOfLoaderV1(program_account) => ProgramCacheEntry::new(
111-
program_account.owner(),
112-
environments.program_runtime_v1.clone(),
113-
0,
114-
DELAY_VISIBILITY_SLOT_OFFSET,
115-
program_account.data(),
116-
program_account.data().len(),
130+
ProgramAccountLoadResult::ProgramOfLoaderV1(program_account) => load_program_from_bytes(
117131
#[cfg(feature = "metrics")]
118132
&mut load_program_metrics,
133+
program_account.data(),
134+
program_account.owner(),
135+
program_account.data().len(),
136+
0,
137+
environments.program_runtime_v1.clone(),
119138
)
120139
.map_err(|_| (0, ProgramCacheEntryOwner::LoaderV1)),
121140

122-
ProgramAccountLoadResult::ProgramOfLoaderV2(program_account) => ProgramCacheEntry::new(
123-
program_account.owner(),
124-
environments.program_runtime_v1.clone(),
125-
0,
126-
DELAY_VISIBILITY_SLOT_OFFSET,
127-
program_account.data(),
128-
program_account.data().len(),
141+
ProgramAccountLoadResult::ProgramOfLoaderV2(program_account) => load_program_from_bytes(
129142
#[cfg(feature = "metrics")]
130143
&mut load_program_metrics,
144+
program_account.data(),
145+
program_account.owner(),
146+
program_account.data().len(),
147+
0,
148+
environments.program_runtime_v1.clone(),
131149
)
132150
.map_err(|_| (0, ProgramCacheEntryOwner::LoaderV2)),
133151

@@ -140,18 +158,17 @@ pub fn load_program_with_pubkey<CB: TransactionProcessingCallback>(
140158
.get(UpgradeableLoaderState::size_of_programdata_metadata()..)
141159
.ok_or(Box::new(InstructionError::InvalidAccountData).into())
142160
.and_then(|programdata| {
143-
ProgramCacheEntry::new(
144-
program_account.owner(),
145-
environments.program_runtime_v1.clone(),
146-
deployment_slot,
147-
deployment_slot.saturating_add(DELAY_VISIBILITY_SLOT_OFFSET),
161+
load_program_from_bytes(
162+
#[cfg(feature = "metrics")]
163+
&mut load_program_metrics,
148164
programdata,
165+
program_account.owner(),
149166
program_account
150167
.data()
151168
.len()
152169
.saturating_add(programdata_account.data().len()),
153-
#[cfg(feature = "metrics")]
154-
&mut load_program_metrics,
170+
deployment_slot,
171+
environments.program_runtime_v1.clone(),
155172
)
156173
})
157174
.map_err(|_| (deployment_slot, ProgramCacheEntryOwner::LoaderV3)),
@@ -162,15 +179,14 @@ pub fn load_program_with_pubkey<CB: TransactionProcessingCallback>(
162179
.get(LoaderV4State::program_data_offset()..)
163180
.ok_or(Box::new(InstructionError::InvalidAccountData).into())
164181
.and_then(|elf_bytes| {
165-
ProgramCacheEntry::new(
166-
&loader_v4::id(),
167-
environments.program_runtime_v1.clone(),
168-
deployment_slot,
169-
deployment_slot.saturating_add(DELAY_VISIBILITY_SLOT_OFFSET),
170-
elf_bytes,
171-
program_account.data().len(),
182+
load_program_from_bytes(
172183
#[cfg(feature = "metrics")]
173184
&mut load_program_metrics,
185+
elf_bytes,
186+
&loader_v4::id(),
187+
program_account.data().len(),
188+
deployment_slot,
189+
environments.program_runtime_v1.clone(),
174190
)
175191
})
176192
.map_err(|_| (deployment_slot, ProgramCacheEntryOwner::LoaderV4))
@@ -235,9 +251,7 @@ mod tests {
235251
crate::transaction_processor::TransactionBatchProcessor,
236252
solana_account::WritableAccount,
237253
solana_program_runtime::{
238-
loaded_programs::{
239-
BlockRelation, ForkGraph, ProgramRuntimeEnvironment, ProgramRuntimeEnvironments,
240-
},
254+
loaded_programs::{BlockRelation, ForkGraph, ProgramRuntimeEnvironments},
241255
solana_sbpf::program::BuiltinProgram,
242256
},
243257
solana_sdk_ids::{bpf_loader, bpf_loader_upgradeable},
@@ -463,15 +477,14 @@ mod tests {
463477
let slot = 2;
464478
let environment = ProgramRuntimeEnvironment::new(BuiltinProgram::new_mock());
465479

466-
let result = ProgramCacheEntry::new(
467-
&loader,
468-
environment.clone(),
469-
slot,
470-
slot.saturating_add(DELAY_VISIBILITY_SLOT_OFFSET),
471-
&buffer,
472-
size,
480+
let result = load_program_from_bytes(
473481
#[cfg(feature = "metrics")]
474482
&mut metrics,
483+
&buffer,
484+
&loader,
485+
size,
486+
slot,
487+
environment.clone(),
475488
);
476489

477490
assert!(result.is_ok());
@@ -573,15 +586,14 @@ mod tests {
573586
);
574587

575588
let environments = ProgramRuntimeEnvironments::default();
576-
let expected = ProgramCacheEntry::new(
577-
account_data.owner(),
578-
environments.program_runtime_v1.clone(),
579-
0,
580-
DELAY_VISIBILITY_SLOT_OFFSET,
581-
account_data.data(),
582-
account_data.data().len(),
589+
let expected = load_program_from_bytes(
583590
#[cfg(feature = "metrics")]
584591
&mut LoadProgramMetrics::default(),
592+
account_data.data(),
593+
account_data.owner(),
594+
account_data.data().len(),
595+
0,
596+
environments.program_runtime_v1.clone(),
585597
);
586598

587599
assert_eq!(result.unwrap(), (Arc::new(expected.unwrap()), 0));
@@ -667,15 +679,14 @@ mod tests {
667679
.set_data(data[UpgradeableLoaderState::size_of_programdata_metadata()..].to_vec());
668680

669681
let environments = ProgramRuntimeEnvironments::default();
670-
let expected = ProgramCacheEntry::new(
671-
account_data.owner(),
672-
environments.program_runtime_v1.clone(),
673-
0,
674-
DELAY_VISIBILITY_SLOT_OFFSET,
675-
account_data.data(),
676-
account_data.data().len(),
682+
let expected = load_program_from_bytes(
677683
#[cfg(feature = "metrics")]
678684
&mut LoadProgramMetrics::default(),
685+
account_data.data(),
686+
account_data.owner(),
687+
account_data.data().len(),
688+
0,
689+
environments.program_runtime_v1.clone(),
679690
);
680691
assert_eq!(result.unwrap(), (Arc::new(expected.unwrap()), 0));
681692
}
@@ -752,15 +763,14 @@ mod tests {
752763
.insert(key, (account_data.clone(), 0));
753764

754765
let environments = ProgramRuntimeEnvironments::default();
755-
let expected = ProgramCacheEntry::new(
756-
account_data.owner(),
757-
environments.program_runtime_v1.clone(),
758-
0,
759-
DELAY_VISIBILITY_SLOT_OFFSET,
760-
account_data.data(),
761-
account_data.data().len(),
766+
let expected = load_program_from_bytes(
762767
#[cfg(feature = "metrics")]
763768
&mut LoadProgramMetrics::default(),
769+
account_data.data(),
770+
account_data.owner(),
771+
account_data.data().len(),
772+
0,
773+
environments.program_runtime_v1.clone(),
764774
);
765775
assert_eq!(result.unwrap(), (Arc::new(expected.unwrap()), 0));
766776
}

‎svm/src/transaction_processor.rs‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -904,11 +904,8 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
904904
} else if missing_programs.is_empty() {
905905
break;
906906
} else {
907-
// Remember: there are multiple transaction processor threads running concurrently
908-
// and those other threads may be loading this or other programs.
909-
//
910-
// So, sleep until some other thread submits a program with their
911-
// `finish_cooperative_loading_task` call. We'll then wake up and try to load the
907+
// Sleep until the next finish_cooperative_loading_task() call.
908+
// Once a task completes we'll wake up and try to load the
912909
// missing programs inside the tx batch again.
913910
let _new_cookie = task_waiter.wait(task_cookie);
914911
}

0 commit comments

Comments
 (0)