From 3886d9190e89d44a701ad5cbbda0c7457feba510 Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Fri, 12 Aug 2022 17:31:05 +0200 Subject: [PATCH] Optimize ValueStack (#404) * optimize ValueStack methods by removing unncessary bounds checks This resulted in a performance boost of approx 8-10% across the board. In debug mode we still do bounds checking which helps with testing. * optimize ValueStack::peek{_mut} slightly No longer adds and subtracts depth by 1. * add #[cold] UntypedError::invalid_len and make use of it * replace type name with Self in match * simplify and clean up impls for {i32,i64}::{div,rem} * fix compile error * apply rustfmt --- core/src/trap.rs | 18 +++---- core/src/untyped.rs | 28 +++++------ core/src/value.rs | 18 +++---- wasmi_v1/src/engine/exec_context.rs | 3 +- wasmi_v1/src/engine/value_stack.rs | 75 ++++++++++++++++++++++++----- 5 files changed, 93 insertions(+), 49 deletions(-) diff --git a/core/src/trap.rs b/core/src/trap.rs index e9955be6b8..6f3f03962f 100644 --- a/core/src/trap.rs +++ b/core/src/trap.rs @@ -159,15 +159,15 @@ impl TrapCode { /// other uses since it avoid heap memory allocation in certain cases. pub fn trap_message(&self) -> &'static str { match self { - TrapCode::Unreachable => "unreachable", - TrapCode::MemoryAccessOutOfBounds => "out of bounds memory access", - TrapCode::TableAccessOutOfBounds => "undefined element", - TrapCode::ElemUninitialized => "uninitialized element", - TrapCode::DivisionByZero => "integer divide by zero", - TrapCode::IntegerOverflow => "integer overflow", - TrapCode::InvalidConversionToInt => "invalid conversion to integer", - TrapCode::StackOverflow => "call stack exhausted", - TrapCode::UnexpectedSignature => "indirect call type mismatch", + Self::Unreachable => "unreachable", + Self::MemoryAccessOutOfBounds => "out of bounds memory access", + Self::TableAccessOutOfBounds => "undefined element", + Self::ElemUninitialized => "uninitialized element", + Self::DivisionByZero => "integer divide by zero", + Self::IntegerOverflow => "integer overflow", + Self::InvalidConversionToInt => "invalid conversion to integer", + Self::StackOverflow => "call stack exhausted", + Self::UnexpectedSignature => "indirect call type mismatch", } } } diff --git a/core/src/untyped.rs b/core/src/untyped.rs index d1ed7e953f..729c5cf3e8 100644 --- a/core/src/untyped.rs +++ b/core/src/untyped.rs @@ -882,6 +882,14 @@ pub enum UntypedError { }, } +impl UntypedError { + /// Creates a new `InvalidLen` [`UntypedError`]. + #[cold] + pub fn invalid_len(expected: usize, found: usize) -> Self { + Self::InvalidLen { expected, found } + } +} + impl Display for UntypedError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { @@ -953,10 +961,7 @@ where { fn decode_untyped_slice(results: &[UntypedValue]) -> Result { if results.len() != 1 { - return Err(UntypedError::InvalidLen { - expected: 1, - found: results.len(), - }); + return Err(UntypedError::invalid_len(1, results.len())); } Ok(>::from(results[0])) } @@ -979,10 +984,7 @@ macro_rules! impl_decode_untyped_slice { )* )), _unexpected => { - Err(UntypedError::InvalidLen { - expected: $n, - found: results.len(), - }) + Err(UntypedError::invalid_len($n, results.len())) } } } @@ -1012,10 +1014,7 @@ where { fn encode_untyped_slice(self, results: &mut [UntypedValue]) -> Result<(), UntypedError> { if results.len() != 1 { - return Err(UntypedError::InvalidLen { - expected: 1, - found: results.len(), - }); + return Err(UntypedError::invalid_len(1, results.len())); } results[0] = self.into(); Ok(()) @@ -1033,10 +1032,7 @@ macro_rules! impl_encode_untyped_slice { #[allow(non_snake_case)] fn encode_untyped_slice(self, results: &mut [UntypedValue]) -> Result<(), UntypedError> { if results.len() != $n { - return Err(UntypedError::InvalidLen { - expected: $n, - found: results.len(), - }) + return Err(UntypedError::invalid_len($n, results.len())) } let ($($tuple,)*) = self; let converted: [UntypedValue; $n] = [ diff --git a/core/src/value.rs b/core/src/value.rs index 28fa7a1ccb..e9d9b8eb43 100644 --- a/core/src/value.rs +++ b/core/src/value.rs @@ -780,14 +780,11 @@ macro_rules! impl_integer_arithmetic_ops { #[inline] fn div(self, other: $type) -> Result<$type, TrapCode> { if other == 0 { - Err(TrapCode::DivisionByZero) - } else { - let (result, overflow) = self.overflowing_div(other); - if overflow { - Err(TrapCode::IntegerOverflow) - } else { - Ok(result) - } + return Err(TrapCode::DivisionByZero); + } + match self.overflowing_div(other) { + (result, false) => Ok(result), + (_, true) => Err(TrapCode::IntegerOverflow), } } } @@ -853,10 +850,9 @@ macro_rules! impl_integer { #[inline] fn rem(self, other: $type) -> Result<$type, TrapCode> { if other == 0 { - Err(TrapCode::DivisionByZero) - } else { - Ok(self.wrapping_rem(other)) + return Err(TrapCode::DivisionByZero); } + Ok(self.wrapping_rem(other)) } } }; diff --git a/wasmi_v1/src/engine/exec_context.rs b/wasmi_v1/src/engine/exec_context.rs index 1f299bad33..2ba3907e48 100644 --- a/wasmi_v1/src/engine/exec_context.rs +++ b/wasmi_v1/src/engine/exec_context.rs @@ -344,8 +344,7 @@ where /// Returns the local depth as `usize`. fn convert_local_depth(local_depth: LocalIdx) -> usize { - // TODO: calculate the -1 offset at module compilation time. - (local_depth.into_inner() - 1) as usize + local_depth.into_inner() as usize } /// Calculates the effective address of a linear memory access. diff --git a/wasmi_v1/src/engine/value_stack.rs b/wasmi_v1/src/engine/value_stack.rs index 6bbcb41ac9..5cd70ec226 100644 --- a/wasmi_v1/src/engine/value_stack.rs +++ b/wasmi_v1/src/engine/value_stack.rs @@ -95,6 +95,54 @@ impl ValueStack { } } + /// Returns the [`UntypedValue`] at the given `index`. + /// + /// # Note + /// + /// This is an optimized convenience method that only asserts + /// that the index is within bounds in `debug` mode. + /// + /// # Safety + /// + /// This is safe since all wasmi bytecode has been validated + /// during translation and therefore cannot result in out of + /// bounds accesses. + /// + /// # Panics (Debug) + /// + /// If the `index` is out of bounds. + fn get_release_unchecked(&self, index: usize) -> UntypedValue { + debug_assert!(index < self.entries.len()); + // Safety: This is safe since all wasmi bytecode has been validated + // during translation and therefore cannot result in out of + // bounds accesses. + unsafe { *self.entries.get_unchecked(index) } + } + + /// Returns the [`UntypedValue`] at the given `index`. + /// + /// # Note + /// + /// This is an optimized convenience method that only asserts + /// that the index is within bounds in `debug` mode. + /// + /// # Safety + /// + /// This is safe since all wasmi bytecode has been validated + /// during translation and therefore cannot result in out of + /// bounds accesses. + /// + /// # Panics (Debug) + /// + /// If the `index` is out of bounds. + fn get_release_unchecked_mut(&mut self, index: usize) -> &mut UntypedValue { + debug_assert!(index < self.entries.len()); + // Safety: This is safe since all wasmi bytecode has been validated + // during translation and therefore cannot result in out of + // bounds accesses. + unsafe { self.entries.get_unchecked_mut(index) } + } + /// Extends the value stack by the `additional` amount of zeros. /// /// # Errors @@ -136,7 +184,7 @@ impl ValueStack { let src = self.stack_ptr - keep; let dst = self.stack_ptr - keep - drop; for i in 0..keep { - self.entries[dst + i] = self.entries[src + i]; + *self.get_release_unchecked_mut(dst + i) = self.get_release_unchecked(src + i); } self.stack_ptr -= drop; } @@ -147,7 +195,7 @@ impl ValueStack { /// /// This has the same effect as [`ValueStack::peek`]`(0)`. pub fn last(&self) -> UntypedValue { - self.entries[self.stack_ptr - 1] + self.get_release_unchecked(self.stack_ptr - 1) } /// Returns the last stack entry of the [`ValueStack`]. @@ -156,25 +204,29 @@ impl ValueStack { /// /// This has the same effect as [`ValueStack::peek`]`(0)`. pub fn last_mut(&mut self) -> &mut UntypedValue { - &mut self.entries[self.stack_ptr - 1] + self.get_release_unchecked_mut(self.stack_ptr - 1) } /// Peeks the entry at the given depth from the last entry. /// /// # Note /// - /// Given a `depth` of 0 has the same effect as [`ValueStack::last`]. + /// Given a `depth` of 1 has the same effect as [`ValueStack::last`]. + /// + /// A `depth` of 0 is invalid and undefined. pub fn peek(&self, depth: usize) -> UntypedValue { - self.entries[self.stack_ptr - depth - 1] + self.get_release_unchecked(self.stack_ptr - depth) } /// Peeks the `&mut` entry at the given depth from the last entry. /// /// # Note /// - /// Given a `depth` of 0 has the same effect as [`ValueStack::last_mut`]. + /// Given a `depth` of 1 has the same effect as [`ValueStack::last_mut`]. + /// + /// A `depth` of 0 is invalid and undefined. pub fn peek_mut(&mut self, depth: usize) -> &mut UntypedValue { - &mut self.entries[self.stack_ptr - depth - 1] + self.get_release_unchecked_mut(self.stack_ptr - depth) } /// Pops the last [`UntypedValue`] from the [`ValueStack`]. @@ -185,9 +237,10 @@ impl ValueStack { /// the executed WebAssembly bytecode for correctness. pub fn pop(&mut self) -> UntypedValue { self.stack_ptr -= 1; - self.entries[self.stack_ptr] + self.get_release_unchecked(self.stack_ptr) } + /// Drops the last value on the [`ValueStack`]. pub fn drop(&mut self, depth: usize) { self.stack_ptr -= depth; } @@ -211,8 +264,8 @@ impl ValueStack { pub fn pop2(&mut self) -> (UntypedValue, UntypedValue) { self.stack_ptr -= 2; ( - self.entries[self.stack_ptr], - self.entries[self.stack_ptr + 1], + self.get_release_unchecked(self.stack_ptr), + self.get_release_unchecked(self.stack_ptr + 1), ) } @@ -246,7 +299,7 @@ impl ValueStack { where T: Into, { - self.entries[self.stack_ptr] = entry.into(); + *self.get_release_unchecked_mut(self.stack_ptr) = entry.into(); self.stack_ptr += 1; }