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

re-iterator version of SIMD-170 impl #3899

Closed
wants to merge 5 commits into from

Conversation

tao-stones
Copy link

Problem

SIMD-170

Summary of Changes

Fixes #

Copy link

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

This approach looks cleaner to me. It does mean that we will do the re-iteration each time we call sanitize_and_convert_to_compute_budget_limits which is not very ideal but I think it can be acceptable.

We call that function in banking stage quite a few times:

  • ImmutableDeserializedPacket::new
  • SanitizedTransactionReceiveAndBuffer::buffer_packets (called twice, one is hidden in calculate_priority_and_cost)
  • Consumer::check_fee_payer_unlocked
  • Consumer::process_and_record_transactions_with_pre_results (in QosService::select_and_accumulate_transaction_costs)
  • Consumer::execute_and_commit_transactions_locked

It's called during replay during rebatching

  • rebatch_and_execute_batches (in CostModel::calculate_cost)

And called for both banking / replay in:

  • TransactionBatchProcessor::validate_transaction_fee_payer
  • PrioritizationFeeCache::update

@@ -11,10 +11,11 @@ use {
/// If succeeded, the transaction's specific limits/requests (could be default)
/// are retrieved and returned,
pub fn process_compute_budget_instructions<'a>(
instructions: impl Iterator<Item = (&'a Pubkey, SVMInstruction<'a>)>,
instructions: impl Iterator<Item = (&'a Pubkey, SVMInstruction<'a>)> + Clone,
Copy link

Choose a reason for hiding this comment

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

we could eliminate the Clone by changing this function to something like:

pub fn process_compute_budget_instructions<'a, F, I>(
    get_instructions: F,
    feature_set: &FeatureSet,
) -> Result<ComputeBudgetLimits, TransactionError>
where
    F: Fn() -> I,
    I: Iterator<Item = (&'a Pubkey, SVMInstruction<'a>)>,
{

@tao-stones
Copy link
Author

Thank you for the list. tbh I am not confident the re-iteration per call will be OK, it kinda negates the purpose of static meta; the lookup at AhashMap when calling BUILTIN_INSTRUCTION_COSTS.contains_key(program_id) can add up quickly. This approach might be slightly better for backporting than tao-stones@86fcaeb; But I still lean to doing heavy lifting in try_from() cause it's called once per transaction, leaving sanitize_and_convert_to_compute_budget_limits() as lightweight as possible (just few if...else and some math).

@tao-stones tao-stones closed this Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants