From 4142f42319aea64d767a17e5796bed395f81cc81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Thu, 10 Nov 2022 21:11:00 +0100 Subject: [PATCH] Refactor - Move `Executor` in program-runtime crate (#28782) * Moves CreateMetrics into the program-runtime crate. * Moves the Executor trait into executor.rs * Removes the first_instruction_account parameter from Executor::execute(). --- program-runtime/src/executor.rs | 39 +++++++++++++++++ program-runtime/src/executor_cache.rs | 18 +------- program-runtime/src/lib.rs | 1 + programs/bpf_loader/src/lib.rs | 62 +++++---------------------- runtime/src/bank.rs | 5 +-- 5 files changed, 55 insertions(+), 70 deletions(-) create mode 100644 program-runtime/src/executor.rs diff --git a/program-runtime/src/executor.rs b/program-runtime/src/executor.rs new file mode 100644 index 00000000000000..8d0894b180b945 --- /dev/null +++ b/program-runtime/src/executor.rs @@ -0,0 +1,39 @@ +use { + crate::{invoke_context::InvokeContext, timings::ExecuteDetailsTimings}, + solana_sdk::{instruction::InstructionError, saturating_add_assign}, +}; + +/// Program executor +pub trait Executor: std::fmt::Debug + Send + Sync { + /// Execute the program + fn execute(&self, invoke_context: &mut InvokeContext) -> Result<(), InstructionError>; +} + +#[derive(Debug, Default)] +pub struct CreateMetrics { + pub program_id: String, + pub register_syscalls_us: u64, + pub load_elf_us: u64, + pub verify_code_us: u64, + pub jit_compile_us: u64, +} + +impl CreateMetrics { + pub fn submit_datapoint(&self, timings: &mut ExecuteDetailsTimings) { + saturating_add_assign!( + timings.create_executor_register_syscalls_us, + self.register_syscalls_us + ); + saturating_add_assign!(timings.create_executor_load_elf_us, self.load_elf_us); + saturating_add_assign!(timings.create_executor_verify_code_us, self.verify_code_us); + saturating_add_assign!(timings.create_executor_jit_compile_us, self.jit_compile_us); + datapoint_trace!( + "create_executor_trace", + ("program_id", self.program_id, String), + ("register_syscalls_us", self.register_syscalls_us, i64), + ("load_elf_us", self.load_elf_us, i64), + ("verify_code_us", self.verify_code_us, i64), + ("jit_compile_us", self.jit_compile_us, i64), + ); + } +} diff --git a/program-runtime/src/executor_cache.rs b/program-runtime/src/executor_cache.rs index 1cfc1c432dc99f..fced218b06c00f 100644 --- a/program-runtime/src/executor_cache.rs +++ b/program-runtime/src/executor_cache.rs @@ -1,11 +1,8 @@ use { - crate::invoke_context::InvokeContext, + crate::executor::Executor, log::*, rand::Rng, - solana_sdk::{ - instruction::InstructionError, pubkey::Pubkey, saturating_add_assign, slot_history::Slot, - stake_history::Epoch, transaction_context::IndexOfAccount, - }, + solana_sdk::{pubkey::Pubkey, saturating_add_assign, slot_history::Slot, stake_history::Epoch}, std::{ collections::HashMap, fmt::Debug, @@ -17,16 +14,6 @@ use { }, }; -/// Program executor -pub trait Executor: Debug + Send + Sync { - /// Execute the program - fn execute( - &self, - first_instruction_account: IndexOfAccount, - invoke_context: &mut InvokeContext, - ) -> Result<(), InstructionError>; -} - /// Relation between a TransactionExecutorCacheEntry and its matching BankExecutorCacheEntry #[repr(u8)] #[derive(Clone, Copy, PartialEq, Eq, Debug)] @@ -386,7 +373,6 @@ mod tests { impl Executor for TestExecutor { fn execute( &self, - _first_instruction_account: IndexOfAccount, _invoke_context: &mut InvokeContext, ) -> std::result::Result<(), InstructionError> { Ok(()) diff --git a/program-runtime/src/lib.rs b/program-runtime/src/lib.rs index 88bfe95834a30e..315f2b6e9d0e8e 100644 --- a/program-runtime/src/lib.rs +++ b/program-runtime/src/lib.rs @@ -11,6 +11,7 @@ extern crate solana_metrics; pub mod accounts_data_meter; pub mod compute_budget; +pub mod executor; pub mod executor_cache; pub mod invoke_context; pub mod log_collector; diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index de727cdbc17b54..75da03f64b5ec8 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -9,9 +9,6 @@ pub mod upgradeable; pub mod upgradeable_with_jit; pub mod with_jit; -#[macro_use] -extern crate solana_metrics; - use { crate::{ allocator_bump::BpfAllocator, @@ -22,13 +19,13 @@ use { solana_measure::measure::Measure, solana_program_runtime::{ compute_budget::ComputeBudget, - executor_cache::{Executor, TransactionExecutorCache}, + executor::{CreateMetrics, Executor}, + executor_cache::TransactionExecutorCache, ic_logger_msg, ic_msg, invoke_context::{ComputeMeter, InvokeContext}, log_collector::LogCollector, stable_log, sysvar_cache::get_sysvar_with_account_check, - timings::ExecuteDetailsTimings, }, solana_rbpf::{ aligned_memory::AlignedMemory, @@ -91,39 +88,6 @@ pub enum BpfError { } impl UserDefinedError for BpfError {} -mod executor_metrics { - use super::*; - - #[derive(Debug, Default)] - pub struct CreateMetrics { - pub program_id: String, - pub register_syscalls_us: u64, - pub load_elf_us: u64, - pub verify_code_us: u64, - pub jit_compile_us: u64, - } - - impl CreateMetrics { - pub fn submit_datapoint(&self, timings: &mut ExecuteDetailsTimings) { - saturating_add_assign!( - timings.create_executor_register_syscalls_us, - self.register_syscalls_us - ); - saturating_add_assign!(timings.create_executor_load_elf_us, self.load_elf_us); - saturating_add_assign!(timings.create_executor_verify_code_us, self.verify_code_us); - saturating_add_assign!(timings.create_executor_jit_compile_us, self.jit_compile_us); - datapoint_trace!( - "create_executor_trace", - ("program_id", self.program_id, String), - ("register_syscalls_us", self.register_syscalls_us, i64), - ("load_elf_us", self.load_elf_us, i64), - ("verify_code_us", self.verify_code_us, i64), - ("jit_compile_us", self.jit_compile_us, i64), - ); - } - } -} - // The BPF loader is special in that it is the only place in the runtime and its built-in programs, // where data comes not only from instruction account but also program accounts. // Thus, these two helper methods have to distinguish the mixed sources via index_in_instruction. @@ -162,7 +126,7 @@ fn create_executor_from_bytes( feature_set: &FeatureSet, compute_budget: &ComputeBudget, log_collector: Option>>, - create_executor_metrics: &mut executor_metrics::CreateMetrics, + create_executor_metrics: &mut CreateMetrics, programdata: &[u8], use_jit: bool, reject_deployment_of_broken_elfs: bool, @@ -246,7 +210,7 @@ pub fn create_executor_from_account( program: &BorrowedAccount, programdata: &BorrowedAccount, use_jit: bool, -) -> Result<(Arc, Option), InstructionError> { +) -> Result<(Arc, Option), InstructionError> { if !check_loader_id(program.get_owner()) { ic_logger_msg!( log_collector, @@ -292,9 +256,9 @@ pub fn create_executor_from_account( } } - let mut create_executor_metrics = executor_metrics::CreateMetrics { + let mut create_executor_metrics = CreateMetrics { program_id: program.get_key().to_string(), - ..executor_metrics::CreateMetrics::default() + ..CreateMetrics::default() }; let executor = create_executor_from_bytes( feature_set, @@ -489,7 +453,7 @@ fn process_instruction_common( create_executor_metrics.submit_datapoint(&mut invoke_context.timings); } - executor.execute(program_account_index, invoke_context) + executor.execute(invoke_context) } else { drop(program); debug_assert_eq!(first_instruction_account, 1); @@ -702,7 +666,7 @@ fn process_loader_upgradeable_instruction( let instruction_context = transaction_context.get_current_instruction_context()?; let buffer = instruction_context.try_borrow_instruction_account(transaction_context, 3)?; - let mut create_executor_metrics = executor_metrics::CreateMetrics::default(); + let mut create_executor_metrics = CreateMetrics::default(); let executor = create_executor_from_bytes( &invoke_context.feature_set, invoke_context.get_compute_budget(), @@ -881,7 +845,7 @@ fn process_loader_upgradeable_instruction( // Load and verify the program bits let buffer = instruction_context.try_borrow_instruction_account(transaction_context, 2)?; - let mut create_executor_metrics = executor_metrics::CreateMetrics::default(); + let mut create_executor_metrics = CreateMetrics::default(); let executor = create_executor_from_bytes( &invoke_context.feature_set, invoke_context.get_compute_budget(), @@ -1378,7 +1342,7 @@ fn process_loader_instruction( ic_msg!(invoke_context, "key[0] did not sign the transaction"); return Err(InstructionError::MissingRequiredSignature); } - let mut create_executor_metrics = executor_metrics::CreateMetrics::default(); + let mut create_executor_metrics = CreateMetrics::default(); let executor = create_executor_from_bytes( &invoke_context.feature_set, invoke_context.get_compute_budget(), @@ -1436,11 +1400,7 @@ impl Debug for BpfExecutor { } impl Executor for BpfExecutor { - fn execute( - &self, - _first_instruction_account: IndexOfAccount, - invoke_context: &mut InvokeContext, - ) -> Result<(), InstructionError> { + fn execute(&self, invoke_context: &mut InvokeContext) -> Result<(), InstructionError> { let log_collector = invoke_context.get_log_collector(); let compute_meter = invoke_context.get_compute_meter(); let stack_height = invoke_context.get_stack_height(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 3bbed125433dce..68f55a88ea46da 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -89,8 +89,9 @@ use { solana_program_runtime::{ accounts_data_meter::MAX_ACCOUNTS_DATA_LEN, compute_budget::{self, ComputeBudget}, + executor::Executor, executor_cache::{ - BankExecutorCache, Executor, TransactionExecutorCache, TxBankExecutorCacheDiff, + BankExecutorCache, TransactionExecutorCache, TxBankExecutorCacheDiff, MAX_CACHED_EXECUTORS, }, invoke_context::{BuiltinProgram, ProcessInstructionWithContext}, @@ -7950,7 +7951,6 @@ pub(crate) mod tests { rand::Rng, solana_program_runtime::{ compute_budget::MAX_COMPUTE_UNIT_LIMIT, - executor_cache::Executor, invoke_context::{mock_process_instruction, InvokeContext}, prioritization_fee::{PrioritizationFeeDetails, PrioritizationFeeType}, }, @@ -15129,7 +15129,6 @@ pub(crate) mod tests { impl Executor for TestExecutor { fn execute( &self, - _first_instruction_account: IndexOfAccount, _invoke_context: &mut InvokeContext, ) -> std::result::Result<(), InstructionError> { Ok(())