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.

Sign up for GitHub

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

Reduce ProgramCache write lock contention #1037

Merged
merged 3 commits into from
Apr 27, 2024
Merged

Reduce ProgramCache write lock contention #1037

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc12279
Reduce ProgramCache write-lock contention
ryoqun Apr 25, 2024
d50c11c
Also consider programs modified by tx
ryoqun Apr 25, 2024
cce3075
Memoize program cache write lock
ryoqun Apr 26, 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
11 changes: 11 additions & 0 deletions program-runtime/src/loaded_programs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,8 @@ pub struct ProgramCacheForTxBatch {
/// The epoch of the last rerooting
pub latest_root_epoch: Epoch,
pub hit_max_limit: bool,
pub loaded_missing: bool,
pub merged_modified: bool,
}

impl ProgramCacheForTxBatch {
Expand All @@ -654,6 +656,8 @@ impl ProgramCacheForTxBatch {
upcoming_environments,
latest_root_epoch,
hit_max_limit: false,
loaded_missing: false,
merged_modified: false,
}
}

Expand All @@ -669,6 +673,8 @@ impl ProgramCacheForTxBatch {
upcoming_environments: cache.get_upcoming_environments_for_epoch(epoch),
latest_root_epoch: cache.latest_root_epoch,
hit_max_limit: false,
loaded_missing: false,
merged_modified: false,
}
}

Expand Down Expand Up @@ -722,9 +728,14 @@ impl ProgramCacheForTxBatch {

pub fn merge(&mut self, other: &Self) {
other.entries.iter().for_each(|(key, entry)| {
self.merged_modified = true;
self.replenish(*key, entry.clone());
})
}

pub fn is_empty(&self) -> bool {
self.entries.is_empty()
}
}

pub enum ProgramCacheMatchCriteria {
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4162,7 +4162,7 @@ impl Bank {
programs_modified_by_tx,
} = execution_result
{
if details.status.is_ok() {
if details.status.is_ok() && !programs_modified_by_tx.is_empty() {
Copy link
Collaborator Author

@ryoqun ryoqun Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hope this one is fairly uncontroversial. haha

let mut cache = self.transaction_processor.program_cache.write().unwrap();
Copy link
Collaborator Author

@ryoqun ryoqun Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

actually, i noticed that this write-lock is per-tx write-lock, if the batch contains 2 or more transactions, while writing this: #1037 (comment)

Copy link

@Lichtso Lichtso Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You are right. How about:

if execution_results.iter().any(|execution_result| matches!(execution_result, TransactionExecutionResult::Executed { details, programs_modified_by_tx } if details.status.is_ok() && !programs_modified_by_tx.is_empty())) {
    let mut cache = self.transaction_processor.program_cache.write().unwrap();
    for execution_result in &execution_results {
        if let TransactionExecutionResult::Executed { programs_modified_by_tx, .. } = execution_result {
            cache.merge(programs_modified_by_tx);
        }
    }
}

Copy link
Collaborator Author

@ryoqun ryoqun Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, that incurs 2 pass looping for the worst case (totaling, O(2*N)). also rather heavy code duplication.

Considering !programs_modified_by_tx.is_empty() should be rare (unless malice), I think a quick and dirty memoization like this will be enough (this worst case's overall cost is O(Cm*N), where Cm << 2, Cm == memoization overhead cce3075

cache.merge(programs_modified_by_tx);
}
Expand Down
25 changes: 17 additions & 8 deletions svm/src/transaction_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,14 +292,22 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {

execution_time.stop();

const SHRINK_LOADED_PROGRAMS_TO_PERCENTAGE: u8 = 90;
self.program_cache
.write()
.unwrap()
.evict_using_2s_random_selection(
Percentage::from(SHRINK_LOADED_PROGRAMS_TO_PERCENTAGE),
self.slot,
);
// Skip eviction when there's no chance this particular tx batch has increased the size of
// ProgramCache entries. Note that loaded_missing is deliberately defined, so that there's
// still at least one other batch, which will evict the program cache, even after the
// occurrences of cooperative loading.
if programs_loaded_for_tx_batch.borrow().loaded_missing
|| programs_loaded_for_tx_batch.borrow().merged_modified
{
const SHRINK_LOADED_PROGRAMS_TO_PERCENTAGE: u8 = 90;
self.program_cache
.write()
.unwrap()
.evict_using_2s_random_selection(
Percentage::from(SHRINK_LOADED_PROGRAMS_TO_PERCENTAGE),
self.slot,
);
}

debug!(
"load: {}us execute: {}us txs_len={}",
Expand Down Expand Up @@ -395,6 +403,7 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
}
// Submit our last completed loading task.
if let Some((key, program)) = program_to_store.take() {
loaded_programs_for_txs.as_mut().unwrap().loaded_missing = true;
if program_cache.finish_cooperative_loading_task(self.slot, key, program)
&& limit_to_load_programs
{
Expand Down
Loading