From 56a15dab604eaf666cc23f6df564e27318b43c4a Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Sun, 16 Jun 2024 21:47:28 +0200 Subject: [PATCH] Use `Stack` in `Executor` (#1071) * use Stack in Executor This replaces the ValueStack and CallStack references with a single Stack reference. * swap values and calls fields in Stack This has severe performance implications on the Executor. Unfortunately in Rust 1.78 this regresses performance overall and in Rust 1.79 this improves performance all in all. * apply clippy suggestion * Stack: swap field reset order * refactor Stack::merge_call_frames * use EngineResources reference in Executor * fix doc link * apply rustfmt --- crates/wasmi/src/engine/executor/instrs.rs | 69 ++++++------------- .../wasmi/src/engine/executor/instrs/call.rs | 45 ++++++------ .../src/engine/executor/instrs/memory.rs | 2 +- .../src/engine/executor/instrs/return_.rs | 23 ++++--- .../wasmi/src/engine/executor/instrs/table.rs | 2 +- crates/wasmi/src/engine/executor/mod.rs | 6 +- crates/wasmi/src/engine/executor/stack/mod.rs | 20 +++--- 7 files changed, 71 insertions(+), 96 deletions(-) diff --git a/crates/wasmi/src/engine/executor/instrs.rs b/crates/wasmi/src/engine/executor/instrs.rs index 72e7ae43b2..2173b01230 100644 --- a/crates/wasmi/src/engine/executor/instrs.rs +++ b/crates/wasmi/src/engine/executor/instrs.rs @@ -1,6 +1,9 @@ pub use self::call::{dispatch_host_func, ResumableHostError}; use self::return_::ReturnOutcome; -use super::cache::{CachedGlobal, CachedMemory}; +use super::{ + cache::{CachedGlobal, CachedMemory}, + Stack, +}; use crate::{ core::{TrapCode, UntypedVal}, engine::{ @@ -21,8 +24,7 @@ use crate::{ }, code_map::InstructionPtr, executor::stack::{CallFrame, CallStack, FrameRegisters, ValueStack}, - func_types::FuncTypeRegistry, - CodeMap, + EngineResources, }, memory::DataSegment, module::DEFAULT_MEMORY_INDEX, @@ -73,12 +75,10 @@ macro_rules! forward_return { #[inline(never)] pub fn execute_instrs<'engine, T>( store: &mut Store, - value_stack: &'engine mut ValueStack, - call_stack: &'engine mut CallStack, - code_map: &'engine CodeMap, - func_types: &'engine FuncTypeRegistry, + stack: &'engine mut Stack, + res: &'engine EngineResources, ) -> Result<(), Error> { - Executor::new(value_stack, call_stack, code_map, func_types).execute(store) + Executor::new(stack, res).execute(store) } /// An execution context for executing a Wasmi function frame. @@ -92,59 +92,34 @@ struct Executor<'engine> { memory: CachedMemory, /// The cached global variable at index 0. global: CachedGlobal, - /// The value stack. - /// - /// # Note - /// - /// This reference is mainly used to synchronize back state - /// after manipulations to the value stack via `sp`. - value_stack: &'engine mut ValueStack, - /// The call stack. - /// - /// # Note - /// - /// This is used to store the stack of nested function calls. - call_stack: &'engine mut CallStack, - /// The Wasm function code map. - /// - /// # Note - /// - /// This is used to lookup Wasm function information. - code_map: &'engine CodeMap, - /// The Wasm function type registry. - /// - /// # Note + /// The value and call stacks. + stack: &'engine mut Stack, + /// The static resources of an [`Engine`]. /// - /// This is used to lookup Wasm function information. - func_types: &'engine FuncTypeRegistry, + /// [`Engine`]: crate::Engine + res: &'engine EngineResources, } impl<'engine> Executor<'engine> { /// Creates a new [`Executor`] for executing a Wasmi function frame. #[inline(always)] - pub fn new( - value_stack: &'engine mut ValueStack, - call_stack: &'engine mut CallStack, - code_map: &'engine CodeMap, - func_types: &'engine FuncTypeRegistry, - ) -> Self { - let frame = call_stack + pub fn new(stack: &'engine mut Stack, res: &'engine EngineResources) -> Self { + let frame = stack + .calls .peek() .expect("must have call frame on the call stack"); // Safety: We are using the frame's own base offset as input because it is // guaranteed by the Wasm validation and translation phase to be // valid for all register indices used by the associated function body. - let sp = unsafe { value_stack.stack_ptr_at(frame.base_offset()) }; + let sp = unsafe { stack.values.stack_ptr_at(frame.base_offset()) }; let ip = frame.instr_ptr(); Self { sp, ip, memory: CachedMemory::default(), global: CachedGlobal::default(), - value_stack, - call_stack, - code_map, - func_types, + stack, + res, } } @@ -160,7 +135,7 @@ impl<'engine> Executor<'engine> { #[inline(always)] fn execute(mut self, store: &mut Store) -> Result<(), Error> { use Instruction as Instr; - let instance = Self::instance(self.call_stack); + let instance = Self::instance(&self.stack.calls); self.memory.update(&mut store.inner, instance); self.global.update(&mut store.inner, instance); loop { @@ -914,7 +889,7 @@ macro_rules! get_entity { )] #[inline] fn $name(&self, store: &StoreInner, index: $index_ty) -> $id_ty { - let instance = Self::instance(self.call_stack); + let instance = Self::instance(&self.stack.calls); let index = ::core::primitive::u32::from(index); store .resolve_instance(instance) @@ -1037,7 +1012,7 @@ impl<'engine> Executor<'engine> { /// /// The initialization of the [`Executor`] allows for efficient execution. fn init_call_frame(&mut self, frame: &CallFrame) { - Self::init_call_frame_impl(self.value_stack, &mut self.sp, &mut self.ip, frame) + Self::init_call_frame_impl(&mut self.stack.values, &mut self.sp, &mut self.ip, frame) } /// Initializes the [`Executor`] state for the [`CallFrame`]. diff --git a/crates/wasmi/src/engine/executor/instrs/call.rs b/crates/wasmi/src/engine/executor/instrs/call.rs index 25f482cb42..cc55266838 100644 --- a/crates/wasmi/src/engine/executor/instrs/call.rs +++ b/crates/wasmi/src/engine/executor/instrs/call.rs @@ -4,7 +4,7 @@ use crate::{ engine::{ bytecode::{FuncIdx, Instruction, Register, RegisterSpan, SignatureIdx, TableIdx}, code_map::InstructionPtr, - executor::stack::{CallFrame, FrameParams, Stack, ValueStack}, + executor::stack::{CallFrame, FrameParams, ValueStack}, func_types::FuncTypeRegistry, CompiledFunc, CompiledFuncEntity, @@ -172,7 +172,8 @@ impl<'engine> Executor<'engine> { // other parts of the code more fragile with respect to instruction ordering. self.ip.add(offset); let caller = self - .call_stack + .stack + .calls .peek_mut() .expect("caller call frame must be on the stack"); caller.update_instr_ptr(self.ip); @@ -218,10 +219,11 @@ impl<'engine> Executor<'engine> { // We have to reinstantiate the `self.sp` [`FrameRegisters`] since we just called // [`ValueStack::alloc_call_frame`] which might invalidate all live [`FrameRegisters`]. let caller = self - .call_stack + .stack + .calls .peek() .expect("need to have a caller on the call stack"); - let (mut uninit_params, offsets) = self.value_stack.alloc_call_frame(func, |this| { + let (mut uninit_params, offsets) = self.stack.values.alloc_call_frame(func, |this| { // Safety: We use the base offset of a live call frame on the call stack. self.sp = unsafe { this.stack_ptr_at(caller.base_offset()) }; })?; @@ -298,7 +300,7 @@ impl<'engine> Executor<'engine> { func: CompiledFunc, mut instance: Option, ) -> Result<(), Error> { - let func = self.code_map.get(Some(store.fuel_mut()), func)?; + let func = self.res.code_map.get(Some(store.fuel_mut()), func)?; let mut called = self.dispatch_compiled_func::(results, func)?; match ::KIND { CallKind::Nested => { @@ -316,16 +318,14 @@ impl<'engine> Executor<'engine> { // on the value stack which is what the function expects. After this operation we ensure // that `self.sp` is adjusted via a call to `init_call_frame` since it may have been // invalidated by this method. - let caller_instance = unsafe { - Stack::merge_call_frames(self.call_stack, self.value_stack, &mut called) - }; + let caller_instance = unsafe { self.stack.merge_call_frames(&mut called) }; if let Some(caller_instance) = caller_instance { instance.get_or_insert(caller_instance); } } } self.init_call_frame(&called); - self.call_stack.push(called, instance)?; + self.stack.calls.push(called, instance)?; Ok(()) } @@ -368,7 +368,8 @@ impl<'engine> Executor<'engine> { /// /// [`CallStack`]: crate::engine::executor::stack::CallStack fn caller_results(&self) -> RegisterSpan { - self.call_stack + self.stack + .calls .peek() .expect("must have caller on the stack") .results() @@ -496,20 +497,18 @@ impl<'engine> Executor<'engine> { func: &Func, host_func: HostFuncEntity, ) -> Result<(), Error> { - let (len_params, len_results) = self.func_types.len_params_results(host_func.ty_dedup()); + let (len_params, len_results) = + self.res.func_types.len_params_results(host_func.ty_dedup()); let max_inout = len_params.max(len_results); - let instance = *self - .call_stack - .instance() - .expect("need to have an instance on the call stack"); + let instance = *Self::instance(&self.stack.calls); // We have to reinstantiate the `self.sp` [`FrameRegisters`] since we just called // [`ValueStack::reserve`] which might invalidate all live [`FrameRegisters`]. let caller = match ::KIND { - CallKind::Nested => self.call_stack.peek().copied(), - CallKind::Tail => self.call_stack.pop().map(|(frame, _instance)| frame), + CallKind::Nested => self.stack.calls.peek().copied(), + CallKind::Tail => self.stack.calls.pop().map(|(frame, _instance)| frame), } .expect("need to have a caller on the call stack"); - let buffer = self.value_stack.extend_by(max_inout, |this| { + let buffer = self.stack.values.extend_by(max_inout, |this| { // Safety: we use the base offset of a live call frame on the call stack. self.sp = unsafe { this.stack_ptr_at(caller.base_offset()) }; })?; @@ -521,14 +520,14 @@ impl<'engine> Executor<'engine> { self.update_instr_ptr_at(1); } self.dispatch_host_func::(store, host_func, &instance) - .map_err(|error| match self.call_stack.is_empty() { + .map_err(|error| match self.stack.calls.is_empty() { true => error, false => ResumableHostError::new(error, *func, results).into(), })?; self.memory.update(&mut store.inner, &instance); self.global.update(&mut store.inner, &instance); let results = results.iter(len_results); - let returned = self.value_stack.drop_return(max_inout); + let returned = self.stack.values.drop_return(max_inout); for (result, value) in results.zip(returned) { // # Safety (1) // @@ -553,8 +552,8 @@ impl<'engine> Executor<'engine> { ) -> Result<(usize, usize), Error> { dispatch_host_func( store, - self.func_types, - self.value_stack, + &self.res.func_types, + &mut self.stack.values, host_func, Some(instance), ) @@ -636,7 +635,7 @@ impl<'engine> Executor<'engine> { let actual_signature = store.inner.resolve_func(func).ty_dedup(); let expected_signature = store .inner - .resolve_instance(Self::instance(self.call_stack)) + .resolve_instance(Self::instance(&self.stack.calls)) .get_signature(u32::from(func_type)) .unwrap_or_else(|| { panic!("missing signature for call_indirect at index: {func_type:?}") diff --git a/crates/wasmi/src/engine/executor/instrs/memory.rs b/crates/wasmi/src/engine/executor/instrs/memory.rs index caa6201030..8a49fac9b5 100644 --- a/crates/wasmi/src/engine/executor/instrs/memory.rs +++ b/crates/wasmi/src/engine/executor/instrs/memory.rs @@ -97,7 +97,7 @@ impl<'engine> Executor<'engine> { // The `memory.grow` operation might have invalidated the cached // linear memory so we need to reset it in order for the cache to // reload in case it is used again. - let instance = Self::instance(self.call_stack); + let instance = Self::instance(&self.stack.calls); self.memory.update(store, instance); return_value } diff --git a/crates/wasmi/src/engine/executor/instrs/return_.rs b/crates/wasmi/src/engine/executor/instrs/return_.rs index 2d4bf0f156..fb27882cef 100644 --- a/crates/wasmi/src/engine/executor/instrs/return_.rs +++ b/crates/wasmi/src/engine/executor/instrs/return_.rs @@ -26,18 +26,24 @@ impl<'engine> Executor<'engine> { #[inline(always)] fn return_impl(&mut self, store: &mut StoreInner) -> ReturnOutcome { let (returned, popped_instance) = self - .call_stack + .stack + .calls .pop() .expect("the executing call frame is always on the stack"); - self.value_stack.truncate(returned.frame_offset()); - let new_instance = popped_instance.and_then(|_| self.call_stack.instance()); + self.stack.values.truncate(returned.frame_offset()); + let new_instance = popped_instance.and_then(|_| self.stack.calls.instance()); if let Some(new_instance) = new_instance { self.global.update(store, new_instance); self.memory.update(store, new_instance); } - match self.call_stack.peek() { + match self.stack.calls.peek() { Some(caller) => { - Self::init_call_frame_impl(self.value_stack, &mut self.sp, &mut self.ip, caller); + Self::init_call_frame_impl( + &mut self.stack.values, + &mut self.sp, + &mut self.ip, + caller, + ); ReturnOutcome::Wasm } None => ReturnOutcome::Host, @@ -56,7 +62,8 @@ impl<'engine> Executor<'engine> { #[inline(always)] fn return_caller_results(&mut self) -> (FrameRegisters, RegisterSpan) { let (callee, caller) = self - .call_stack + .stack + .calls .peek_2() .expect("the callee must exist on the call stack"); match caller { @@ -68,7 +75,7 @@ impl<'engine> Executor<'engine> { // // Safety: The caller call frame is still live on the value stack // and therefore it is safe to acquire its value stack pointer. - let caller_sp = unsafe { self.value_stack.stack_ptr_at(caller.base_offset()) }; + let caller_sp = unsafe { self.stack.values.stack_ptr_at(caller.base_offset()) }; let results = callee.results(); (caller_sp, results) } @@ -77,7 +84,7 @@ impl<'engine> Executor<'engine> { // // In this case we transfer the single return `value` to the root // register span of the entire value stack which is simply its zero index. - let dst_sp = self.value_stack.root_stack_ptr(); + let dst_sp = self.stack.values.root_stack_ptr(); let results = RegisterSpan::new(Register::from_i16(0)); (dst_sp, results) } diff --git a/crates/wasmi/src/engine/executor/instrs/table.rs b/crates/wasmi/src/engine/executor/instrs/table.rs index 89e3139fef..094add9c34 100644 --- a/crates/wasmi/src/engine/executor/instrs/table.rs +++ b/crates/wasmi/src/engine/executor/instrs/table.rs @@ -416,7 +416,7 @@ impl<'engine> Executor<'engine> { let table_index = self.fetch_table_index(1); let element_index = self.fetch_element_segment_index(2); let (instance, table, element, fuel) = store.resolve_table_init_params( - Self::instance(self.call_stack), + Self::instance(&self.stack.calls), &self.get_table(store, table_index), &self.get_element_segment(store, element_index), ); diff --git a/crates/wasmi/src/engine/executor/mod.rs b/crates/wasmi/src/engine/executor/mod.rs index 9523a03924..df573fe8f0 100644 --- a/crates/wasmi/src/engine/executor/mod.rs +++ b/crates/wasmi/src/engine/executor/mod.rs @@ -293,11 +293,7 @@ impl<'engine> EngineExecutor<'engine> { /// When encountering a Wasm or host trap during execution. #[inline(always)] fn execute_func(&mut self, store: &mut Store) -> Result<(), Error> { - let value_stack = &mut self.stack.values; - let call_stack = &mut self.stack.calls; - let code_map = &self.res.code_map; - let func_types = &self.res.func_types; - execute_instrs(store, value_stack, call_stack, code_map, func_types) + execute_instrs(store, self.stack, self.res) } /// Convenience forwarder to [`dispatch_host_func`]. diff --git a/crates/wasmi/src/engine/executor/stack/mod.rs b/crates/wasmi/src/engine/executor/stack/mod.rs index 0dd48a7967..7a6c1c69c9 100644 --- a/crates/wasmi/src/engine/executor/stack/mod.rs +++ b/crates/wasmi/src/engine/executor/stack/mod.rs @@ -22,10 +22,10 @@ fn err_stack_overflow() -> TrapCode { /// Data structure that combines both value stack and call stack. #[derive(Debug, Default)] pub struct Stack { - /// The value stack. - pub values: ValueStack, /// The call stack. pub calls: CallStack, + /// The value stack. + pub values: ValueStack, } impl Stack { @@ -49,13 +49,13 @@ impl Stack { limits.initial_value_stack_height, limits.maximum_value_stack_height, ); - Self { values, calls } + Self { calls, values } } /// Resets the [`Stack`] for clean reuse. pub fn reset(&mut self) { - self.values.reset(); self.calls.reset(); + self.values.reset(); } /// Create an empty [`Stack`]. @@ -98,12 +98,8 @@ impl Stack { /// all [`FrameRegisters`] affected by this. #[inline(always)] #[must_use] - pub unsafe fn merge_call_frames( - call_stack: &mut CallStack, - value_stack: &mut ValueStack, - callee: &mut CallFrame, - ) -> Option { - let (caller, instance) = call_stack.pop().expect("caller call frame must exist"); + pub unsafe fn merge_call_frames(&mut self, callee: &mut CallFrame) -> Option { + let (caller, instance) = self.calls.pop().expect("caller call frame must exist"); debug_assert_eq!(callee.results(), caller.results()); debug_assert!(caller.base_offset() <= callee.base_offset()); // Safety: @@ -112,7 +108,9 @@ impl Stack { // Therefore only value stack offsets of the top-most call frame on the // value stack are going to be invalidated which we ensure to adjust and // reinstantiate after this operation. - let len_drained = value_stack.drain(caller.frame_offset(), callee.frame_offset()); + let len_drained = self + .values + .drain(caller.frame_offset(), callee.frame_offset()); callee.move_down(len_drained); instance }