Refactor: RuntimeTransaction Instruction Processing #3490
Conversation
| .program_instructions_iter() | ||
| .map(|(program_id, ix)| (program_id, SVMInstruction::from(ix))), | ||
| ); | ||
| let (precompile_signature_details, compute_budget_instruction_details) = |
There was a problem hiding this comment.
@tao-stones this is top-level processing function. Single loop over the instructions that we can use to get all the details we want. very easy to add more to this!
There was a problem hiding this comment.
First impression is neat but perhaps overengineered. Would you give Visitor design pattern another thought? To me Visitor fits the needs nicely: we have a sequence of data, each need to be processed independently by different parties (CB, Sig, Builtin ....), optionally to update some shared dataspace/cache sequentially.
| impl InstructionProcessor<ComputeBudgetFlag> for ComputeBudgetInstructionDetails { | ||
| type OUTPUT = Self; | ||
|
|
||
| #[inline] |
There was a problem hiding this comment.
is this function too large for inlining?
| ) -> Result<(), TransactionError>; | ||
|
|
||
| /// Finalize the processor and return the output. | ||
| fn finalize(self) -> Self::OUTPUT; |
There was a problem hiding this comment.
anyway to avoid this additional step?
There was a problem hiding this comment.
Yeah i don't think this is necessary actually
| type OUTPUT; | ||
|
|
||
| /// Process instructions individually given a `flag` that is derived from | ||
| /// the program id. |
There was a problem hiding this comment.
Is Flag necessary? Can we use Visitor pattern that each "type" implements process_instruction?
| T: for<'k> From<&'k ProgramIdFlag>, | ||
| P: InstructionProcessor<T>, | ||
| { | ||
| #[inline] |
There was a problem hiding this comment.
this inline-ness is also unconventional
There was a problem hiding this comment.
it can be significantly faster, particularly for the individual processors instead of the "all together" one.
| P: InstructionProcessor<T>, | ||
| { | ||
| #[inline] | ||
| pub fn process_instructions<'b>( |
There was a problem hiding this comment.
clever but a bit over-engineering to me. The function fits nicely with Visitor design pattern, I personally prefer Visitor if possible, for its readability and maintainability
| @@ -17,6 +22,36 @@ pub fn process_compute_budget_instructions<'a>( | |||
| .sanitize_and_convert_to_compute_budget_limits() | |||
| } | |||
|
|
|||
| type RuntimeTransactionInstructionsProcessor = | |||
| (PrecompileSignatureDetails, ComputeBudgetInstructionDetails); | |||
There was a problem hiding this comment.
as it is, instructionS_processor isn't very scaleable
| compute_budget_details.process_instruction(&flag.into(), instruction_index, instruction) | ||
| } | ||
|
|
||
| fn finalize(self) -> Self::OUTPUT { |
There was a problem hiding this comment.
it doesn't really do anything tangible, maybe we can remove this extra step in processing
| } | ||
|
|
||
| /// Self translation for [ProgramIdFlag]. | ||
| impl From<&ProgramIdFlag> for ProgramIdFlag { |
There was a problem hiding this comment.
conforming with my generic code. May be able to do some sort of better generic in the code, but copying a u8 for now seems fine to me.
| /// Program flag for determining how to process instructions for finding the | ||
| /// signature details. | ||
| pub enum SignatureDetailsFlag { | ||
| NoMatch, |
There was a problem hiding this comment.
Having to specify NoMatch at both levels is a redundancy not exists w Visitor
There was a problem hiding this comment.
I plan to write some benchmarks in this, but visitor is inefficient because we need to re-check the key for each visitor.
If we add more stuff here, like builtins (which is really my goal rn fro cost-model stuff), then we end up doing more work w/ the visitor pattern.
Let's consider when we add BUILTINs.
I already know from my compute_budget and precompile_details if an instruction is one of those or not.
If it is one of them, then I don't need to do a hash-table lookup - I already know the cost, because I've already identified the key.
Same even as is. If I see that something is compute-budget key, I don't need to check if it's secp or ed25519, it's not because it's compute-budget.
Appreciate the feedback, but I will have to disagree with one of your descriptions here: "processed independently". |
|
closing this - decided to take more incremental approach. add in individual processors we need, then can show improvement with something like this later on. |
Problem
RuntimeTransactiondoes multiple loops over instructions to do minor processing for caching meta dataSummary of Changes
ProgramIdFlagto assign labels to program keysFixes #