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

Find and load missing programs in LoadedPrograms cache #30275

Merged
merged 6 commits into from
Feb 21, 2023

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented Feb 12, 2023

Problem

LoadedProgram cache is currently unused.

Summary of Changes

Find and load missing programs in LoadedPrograms cache

  • filter program accounts in a transaction batch
  • filter the accounts that are missing in LoadedPrograms cache
  • load the programs before processing the transactions

Fixes #

@pgarg66 pgarg66 force-pushed the use-program-cache branch 5 times, most recently from b706e0a to f43f41f Compare February 13, 2023 03:22
@pgarg66 pgarg66 requested a review from Lichtso February 13, 2023 15:44
@pgarg66 pgarg66 force-pushed the use-program-cache branch 3 times, most recently from 2a89875 to 37cc11f Compare February 15, 2023 01:19
@pgarg66 pgarg66 marked this pull request as ready for review February 17, 2023 15:30
@@ -43,7 +43,7 @@ impl TransactionExecutorCache {

pub fn set_tombstone(&mut self, key: Pubkey) {
self.visible
.insert(key, Arc::new(LoadedProgram::new_tombstone()));
.insert(key, Arc::new(LoadedProgram::new_tombstone(0)));
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be clock.slot not 0 because it is used for re-/un-deployment.

/// This behaves differently than `insert_entry()` in case there's currently a program at the
/// `deploytment_slot`. It'll replace the current entry with a tombstone, whereas `insert_entry()`
/// would retain and return the current entry.
pub fn set_tombstone(&mut self, key: Pubkey, deployment_slot: Slot) -> Arc<LoadedProgram> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not feel like the right abstraction, because all forms of un/re/deployment should use the same insert method, including tombstones.

However, the current insert method covers both: Normal loading for execution and program-management-instructions. So maybe that should be split into two insert methods. One for program-management-instructions which can only add entries and never overwrite. And one for normal execution which can unify duplicate loads.

- filter program accounts in a transaction batch
- filter the accounts that are missing in LoadedPrograms cache
- load the programs before processing the transactions
- unit tests
@pgarg66 pgarg66 requested a review from Lichtso February 21, 2023 18:22
@pgarg66 pgarg66 merged commit b1f5b0d into solana-labs:master Feb 21, 2023
@pgarg66 pgarg66 deleted the use-program-cache branch February 21, 2023 20:53
nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request Mar 12, 2023
…0275)

* Find and load missing programs in LoadedPrograms cache

- filter program accounts in a transaction batch
- filter the accounts that are missing in LoadedPrograms cache
- load the programs before processing the transactions
- unit tests

* address review comments

* fix clippy

* address review comments

* fix test

* fix more tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants