Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor - Adds check that only loaded programs can be unloaded #35146

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
85 changes: 60 additions & 25 deletions program-runtime/src/loaded_programs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,20 @@ impl LoadedProgram {
}

pub fn to_unloaded(&self) -> Option<Self> {
match &self.program {
LoadedProgramType::LegacyV0(_)
| LoadedProgramType::LegacyV1(_)
| LoadedProgramType::Typed(_) => {}
Lichtso marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(test)]
LoadedProgramType::TestLoaded(_) => {}
LoadedProgramType::FailedVerification(_)
| LoadedProgramType::Closed
| LoadedProgramType::DelayVisibility
| LoadedProgramType::Unloaded(_)
| LoadedProgramType::Builtin(_) => {
return None;
}
}
Some(Self {
program: LoadedProgramType::Unloaded(self.program.get_environment()?.clone()),
account_size: self.account_size,
Expand Down Expand Up @@ -1071,31 +1085,6 @@ impl<FG: ForkGraph> LoadedPrograms<FG> {
}
}

fn unload_program(&mut self, id: &Pubkey) {
if let Some(second_level) = self.entries.get_mut(id) {
for entry in second_level.slot_versions.iter_mut() {
if let Some(unloaded) = entry.to_unloaded() {
*entry = Arc::new(unloaded);
self.stats
.evictions
.entry(*id)
.and_modify(|c| saturating_add_assign!(*c, 1))
.or_insert(1);
} else {
error!(
"Failed to create an unloaded cache entry for a program type {:?}",
entry.program
);
}
}
}
}

pub fn unload_all_programs(&mut self) {
let keys = self.entries.keys().copied().collect::<Vec<Pubkey>>();
keys.iter().for_each(|key| self.unload_program(key));
}

/// This function removes the given entry for the given program from the cache.
/// The function expects that the program and entry exists in the cache. Otherwise it'll panic.
fn unload_program_entry(&mut self, program: &Pubkey, remove_entry: &Arc<LoadedProgram>) {
Expand Down Expand Up @@ -2386,6 +2375,52 @@ mod tests {
assert!(match_missing(&missing, &program3, true));
}

#[test]
fn test_unloaded() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good, but we still don't have tests for the None path in
sort_and_unload and evict_using_2s_random_selection?

let mut cache = new_mock_cache::<TestForkGraph>();
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()),
] {
let entry = Arc::new(LoadedProgram {
program: loaded_program_type,
account_size: 0,
deployment_slot: 0,
effective_slot: 0,
tx_usage_counter: AtomicU64::default(),
ix_usage_counter: AtomicU64::default(),
latest_access_slot: AtomicU64::default(),
});
assert!(entry.to_unloaded().is_none());

// Check that unload_program_entry() does nothing for this entry
let program_id = Pubkey::new_unique();
cache.assign_program(program_id, entry.clone());
cache.unload_program_entry(&program_id, &entry);
assert_eq!(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check that stats.evictions is 1 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

evictions only increments on successful unloading, not for attempts.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we should check that it goes up when expected? We must check both paths of to_unloaded somehow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

cache.entries.get(&program_id).unwrap().slot_versions.len(),
1
);
assert!(cache.stats.evictions.is_empty());
}

let entry = new_test_loaded_program_with_usage(1, 2, AtomicU64::new(3));
let unloaded_entry = entry.to_unloaded().unwrap();
assert_eq!(unloaded_entry.deployment_slot, 1);
assert_eq!(unloaded_entry.effective_slot, 2);
assert_eq!(unloaded_entry.latest_access_slot.load(Ordering::Relaxed), 1);
assert_eq!(unloaded_entry.tx_usage_counter.load(Ordering::Relaxed), 3);

// Check that unload_program_entry() does its work
let program_id = Pubkey::new_unique();
cache.assign_program(program_id, entry.clone());
cache.unload_program_entry(&program_id, &entry);
assert!(cache.stats.evictions.get(&program_id).is_some());
}

#[test]
fn test_fork_prune_find_first_ancestor() {
let mut cache = new_mock_cache::<TestForkGraphSpecific>();
Expand Down
7 changes: 0 additions & 7 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4525,13 +4525,6 @@ impl Bank {
balances
}

pub fn clear_program_cache(&self) {
self.loaded_programs_cache
.write()
.unwrap()
.unload_all_programs();
}

#[allow(clippy::type_complexity)]
pub fn load_and_execute_transactions(
&self,
Expand Down
Loading