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

Refactor - LoadedPrograms::assign_program() #35233

Merged
Lichtso merged 3 commits into from
Feb 28, 2024
Merged

Refactor - LoadedPrograms::assign_program() #35233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f74c692
Forbids all program replacements except for reloads and builtins.
Lichtso Feb 14, 2024
ffab724
Adds test_assign_program_failure() and test_assign_program_success().
Lichtso Feb 23, 2024
89b434d
Explicitly disallows LoadedProgramType::DelayVisibility to be inserte…
Lichtso Feb 23, 2024
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions program-runtime/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ assert_matches = { workspace = true }
libsecp256k1 = { workspace = true }
solana-logger = { workspace = true }
solana-sdk = { workspace = true, features = ["dev-context-only-utils"] }
test-case = { workspace = true }

[lib]
crate-type = ["lib"]
Expand Down
204 changes: 131 additions & 73 deletions program-runtime/src/loaded_programs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,10 @@ impl<FG: ForkGraph> LoadedPrograms<FG> {
/// Insert a single entry. It's typically called during transaction loading,
/// when the cache doesn't contain the entry corresponding to program `key`.
pub fn assign_program(&mut self, key: Pubkey, entry: Arc<LoadedProgram>) -> bool {
debug_assert!(!matches!(
&entry.program,
LoadedProgramType::DelayVisibility
));
let slot_versions = &mut self.entries.entry(key).or_default().slot_versions;
match slot_versions.binary_search_by(|at| {
at.effective_slot
Expand All @@ -721,33 +725,39 @@ impl<FG: ForkGraph> LoadedPrograms<FG> {
}) {
Ok(index) => {
let existing = slot_versions.get_mut(index).unwrap();
if std::mem::discriminant(&existing.program)
!= std::mem::discriminant(&entry.program)
{
// Copy over the usage counter to the new entry
entry.tx_usage_counter.fetch_add(
existing.tx_usage_counter.load(Ordering::Relaxed),
Ordering::Relaxed,
);
entry.ix_usage_counter.fetch_add(
existing.ix_usage_counter.load(Ordering::Relaxed),
Ordering::Relaxed,
);
*existing = entry.clone();
self.stats.reloads.fetch_add(1, Ordering::Relaxed);
false
} else {
// Something is wrong, I can feel it ...
self.stats.replacements.fetch_add(1, Ordering::Relaxed);
true
match (&existing.program, &entry.program) {
(LoadedProgramType::Builtin(_), LoadedProgramType::Builtin(_))
Comment thread
alessandrod marked this conversation as resolved.
| (LoadedProgramType::Unloaded(_), LoadedProgramType::LegacyV0(_))
| (LoadedProgramType::Unloaded(_), LoadedProgramType::LegacyV1(_))
| (LoadedProgramType::Unloaded(_), LoadedProgramType::Typed(_)) => {}
#[cfg(test)]
(LoadedProgramType::Unloaded(_), LoadedProgramType::TestLoaded(_)) => {}
Comment thread
pgarg66 marked this conversation as resolved.
_ => {
// Something is wrong, I can feel it ...
error!("LoadedPrograms::assign_program() failed key={:?} existing={:?} entry={:?}", key, slot_versions, entry);
debug_assert!(false, "Unexpected replacement of an entry");
self.stats.replacements.fetch_add(1, Ordering::Relaxed);
return true;
}
}
// Copy over the usage counter to the new entry
entry.tx_usage_counter.fetch_add(
existing.tx_usage_counter.load(Ordering::Relaxed),
Ordering::Relaxed,
);
entry.ix_usage_counter.fetch_add(
existing.ix_usage_counter.load(Ordering::Relaxed),
Ordering::Relaxed,
);
*existing = Arc::clone(&entry);
self.stats.reloads.fetch_add(1, Ordering::Relaxed);
}
Err(index) => {
self.stats.insertions.fetch_add(1, Ordering::Relaxed);
slot_versions.insert(index, entry.clone());
false
slot_versions.insert(index, Arc::clone(&entry));
}
}
false
}

pub fn prune_by_deployment_slot(&mut self, slot: Slot) {
Expand Down Expand Up @@ -1153,6 +1163,7 @@ mod tests {
Arc, RwLock,
},
},
test_case::{test_case, test_matrix},
};

static MOCK_ENVIRONMENT: std::sync::OnceLock<ProgramRuntimeEnvironment> =
Expand Down Expand Up @@ -1342,15 +1353,6 @@ mod tests {
programs.push((program2, *deployment_slot, usage_counter));
});

for slot in 21..31 {
set_tombstone(
&mut cache,
program2,
slot,
LoadedProgramType::DelayVisibility,
);
}

for slot in 31..41 {
insert_unloaded_program(&mut cache, program2, slot);
}
Expand Down Expand Up @@ -1402,12 +1404,12 @@ mod tests {
// Test that the cache is constructed with the expected number of entries.
assert_eq!(num_loaded, 8);
assert_eq!(num_unloaded, 30);
assert_eq!(num_tombstones, 30);
assert_eq!(num_tombstones, 20);

// Evicting to 2% should update cache with
// * 5 active entries
// * 33 unloaded entries (3 active programs will get unloaded)
// * 30 tombstones (tombstones are not evicted)
// * 20 tombstones (tombstones are not evicted)
cache.evict_using_2s_random_selection(Percentage::from(2), 21);

let num_loaded = num_matching_entries(&cache, |program_type| {
Expand All @@ -1428,7 +1430,7 @@ mod tests {
// Test that expected number of loaded entries get evicted/unloaded.
assert_eq!(num_loaded, 5);
assert_eq!(num_unloaded, 33);
assert_eq!(num_tombstones, 30);
assert_eq!(num_tombstones, 20);
}

#[test]
Expand Down Expand Up @@ -1489,15 +1491,6 @@ mod tests {
programs.push((program2, *deployment_slot, usage_counter));
});

for slot in 21..31 {
set_tombstone(
&mut cache,
program2,
slot,
LoadedProgramType::DelayVisibility,
);
}

for slot in 31..41 {
insert_unloaded_program(&mut cache, program2, slot);
}
Expand Down Expand Up @@ -1548,12 +1541,12 @@ mod tests {

assert_eq!(num_loaded, 8);
assert_eq!(num_unloaded, 30);
assert_eq!(num_tombstones, 30);
assert_eq!(num_tombstones, 20);

// Evicting to 2% should update cache with
// * 5 active entries
// * 33 unloaded entries (3 active programs will get unloaded)
// * 30 tombstones (tombstones are not evicted)
// * 20 tombstones (tombstones are not evicted)
cache.sort_and_unload(Percentage::from(2));
// Check that every program is still in the cache.
programs.iter().for_each(|entry| {
Expand Down Expand Up @@ -1593,7 +1586,7 @@ mod tests {

assert_eq!(num_loaded, 5);
assert_eq!(num_unloaded, 33);
assert_eq!(num_tombstones, 30);
assert_eq!(num_tombstones, 20);
}

#[test]
Expand Down Expand Up @@ -1675,36 +1668,102 @@ mod tests {
}
}

#[test]
fn test_assign_program_tombstones() {
#[test_matrix(
(
LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())),
LoadedProgramType::Closed,
LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock())),
),
(
LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())),
LoadedProgramType::Closed,
LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),
LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock())),
LoadedProgramType::Builtin(BuiltinProgram::new_mock()),
)
)]
#[test_matrix(
(LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),),
(
LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())),
LoadedProgramType::Closed,
LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),
LoadedProgramType::Builtin(BuiltinProgram::new_mock()),
)
)]
#[test_matrix(
(LoadedProgramType::Builtin(BuiltinProgram::new_mock()),),
(
LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())),
LoadedProgramType::Closed,
LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),
LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock())),
)
)]
#[should_panic(expected = "Unexpected replacement of an entry")]
fn test_assign_program_failure(old: LoadedProgramType, new: LoadedProgramType) {
let mut cache = new_mock_cache::<TestForkGraph>();
let program1 = Pubkey::new_unique();
let env = cache.environments.program_runtime_v1.clone();

set_tombstone(
&mut cache,
program1,
10,
LoadedProgramType::FailedVerification(env.clone()),
);
assert_eq!(cache.entries.get(&program1).unwrap().slot_versions.len(), 1);
set_tombstone(&mut cache, program1, 10, LoadedProgramType::Closed);
assert_eq!(cache.entries.get(&program1).unwrap().slot_versions.len(), 1);
set_tombstone(
&mut cache,
program1,
10,
LoadedProgramType::FailedVerification(env.clone()),
let program_id = Pubkey::new_unique();
assert!(!cache.assign_program(
program_id,
Arc::new(LoadedProgram {
program: old,
account_size: 0,
deployment_slot: 10,
effective_slot: 11,
tx_usage_counter: AtomicU64::default(),
ix_usage_counter: AtomicU64::default(),
latest_access_slot: AtomicU64::default(),
}),
));
cache.assign_program(
program_id,
Arc::new(LoadedProgram {
program: new,
account_size: 0,
deployment_slot: 10,
effective_slot: 11,
tx_usage_counter: AtomicU64::default(),
ix_usage_counter: AtomicU64::default(),
latest_access_slot: AtomicU64::default(),
}),
);
assert_eq!(cache.entries.get(&program1).unwrap().slot_versions.len(), 1);
}

// Fail on exact replacement
assert!(cache.assign_program(
program1,
Arc::new(LoadedProgram::new_tombstone(
10,
LoadedProgramType::FailedVerification(env)
))
#[test_case(
LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),
LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock()))
)]
#[test_case(
LoadedProgramType::Builtin(BuiltinProgram::new_mock()),
LoadedProgramType::Builtin(BuiltinProgram::new_mock())
)]
fn test_assign_program_success(old: LoadedProgramType, new: LoadedProgramType) {
Copy link
Copy Markdown
Contributor

@alessandrod alessandrod Feb 23, 2024

Choose a reason for hiding this comment

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

What about LegacyV*? We have some elf files in the monorepo can't we test those?

Copy link
Copy Markdown
Contributor Author

@Lichtso Lichtso Feb 23, 2024

Choose a reason for hiding this comment

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

If we do that I want to do it for the entire file in a separate PR and get rid of TestLoaded entirely.

let mut cache = new_mock_cache::<TestForkGraph>();
let program_id = Pubkey::new_unique();
assert!(!cache.assign_program(
program_id,
Arc::new(LoadedProgram {
program: old,
account_size: 0,
deployment_slot: 10,
effective_slot: 11,
tx_usage_counter: AtomicU64::default(),
ix_usage_counter: AtomicU64::default(),
latest_access_slot: AtomicU64::default(),
}),
));
assert!(!cache.assign_program(
program_id,
Arc::new(LoadedProgram {
program: new,
account_size: 0,
deployment_slot: 10,
effective_slot: 11,
tx_usage_counter: AtomicU64::default(),
ix_usage_counter: AtomicU64::default(),
latest_access_slot: AtomicU64::default(),
}),
));
}

Expand Down Expand Up @@ -2385,7 +2444,6 @@ mod tests {
for loaded_program_type in [
LoadedProgramType::FailedVerification(cache.environments.program_runtime_v1.clone()),
LoadedProgramType::Closed,
LoadedProgramType::DelayVisibility, // Never inserted in the global cache
LoadedProgramType::Unloaded(cache.environments.program_runtime_v1.clone()),
LoadedProgramType::Builtin(BuiltinProgram::new_mock()),
] {
Expand Down