From 961d7b4d823cecac55556a32f4b81aa7a3be377f Mon Sep 17 00:00:00 2001 From: raskad <32105367+raskad@users.noreply.github.com> Date: Wed, 3 Jul 2024 15:32:24 +0100 Subject: [PATCH] Refactor call frame access to avoid panic checks (#3888) --- core/engine/src/builtins/generator/mod.rs | 5 +- core/engine/src/context/mod.rs | 10 +- core/engine/src/module/source.rs | 4 +- core/engine/src/object/operations.rs | 12 +- core/engine/src/vm/mod.rs | 127 ++++++++++-------- core/engine/src/vm/opcode/arguments.rs | 9 +- core/engine/src/vm/opcode/await/mod.rs | 4 +- .../src/vm/opcode/control_flow/return.rs | 4 +- core/engine/src/vm/opcode/define/mod.rs | 10 +- core/engine/src/vm/opcode/delete/mod.rs | 11 +- .../src/vm/opcode/iteration/loop_ops.rs | 7 +- core/engine/src/vm/opcode/push/literal.rs | 9 +- .../src/vm/opcode/rest_parameter/mod.rs | 5 +- core/engine/src/vm/opcode/set/name.rs | 22 ++- 14 files changed, 125 insertions(+), 114 deletions(-) diff --git a/core/engine/src/builtins/generator/mod.rs b/core/engine/src/builtins/generator/mod.rs index da8e38d181..5304ae5645 100644 --- a/core/engine/src/builtins/generator/mod.rs +++ b/core/engine/src/builtins/generator/mod.rs @@ -96,8 +96,9 @@ impl GeneratorContext { let rp = frame.rp; context.vm.push_frame(frame); - context.vm.frame_mut().rp = rp; - context.vm.frame_mut().set_exit_early(true); + let frame = context.vm.frame_mut(); + frame.rp = rp; + frame.set_exit_early(true); if let Some(value) = value { context.vm.push(value); diff --git a/core/engine/src/context/mod.rs b/core/engine/src/context/mod.rs index db2fa8b0fa..ff8c391a1a 100644 --- a/core/engine/src/context/mod.rs +++ b/core/engine/src/context/mod.rs @@ -828,6 +828,10 @@ impl Context { // 1. If the execution context stack is empty, return null. // 2. Let ec be the topmost execution context on the execution context stack whose ScriptOrModule component is not null. // 3. If no such execution context exists, return null. Otherwise, return ec's ScriptOrModule. + if let Some(active_runnable) = &self.vm.frame.active_runnable { + return Some(active_runnable.clone()); + } + self.vm .frames .iter() @@ -846,11 +850,7 @@ impl Context { return self.vm.native_active_function.clone(); } - if let Some(frame) = self.vm.frames.last() { - return frame.function(&self.vm); - } - - None + self.vm.frame.function(&self.vm) } } diff --git a/core/engine/src/module/source.rs b/core/engine/src/module/source.rs index 81c204ec07..ceadec0c03 100644 --- a/core/engine/src/module/source.rs +++ b/core/engine/src/module/source.rs @@ -1772,9 +1772,7 @@ impl SourceTextModule { context .vm - .frames - .last() - .expect("there should be a frame") + .frame .set_promise_capability(&mut context.vm.stack, capability); // 9. If module.[[HasTLA]] is false, then diff --git a/core/engine/src/object/operations.rs b/core/engine/src/object/operations.rs index 79e7a26d99..5d4e7a7f57 100644 --- a/core/engine/src/object/operations.rs +++ b/core/engine/src/object/operations.rs @@ -411,7 +411,11 @@ impl JsObject { return Ok(context.vm.pop()); } - context.vm.frames[frame_index].set_exit_early(true); + if frame_index + 1 == context.vm.frames.len() { + context.vm.frame.set_exit_early(true); + } else { + context.vm.frames[frame_index + 1].set_exit_early(true); + } let result = context.run().consume(); @@ -461,7 +465,11 @@ impl JsObject { .clone()); } - context.vm.frames[frame_index].set_exit_early(true); + if frame_index + 1 == context.vm.frames.len() { + context.vm.frame.set_exit_early(true); + } else { + context.vm.frames[frame_index + 1].set_exit_early(true); + } let result = context.run().consume(); diff --git a/core/engine/src/vm/mod.rs b/core/engine/src/vm/mod.rs index 46fcdfc297..cf32142d4a 100644 --- a/core/engine/src/vm/mod.rs +++ b/core/engine/src/vm/mod.rs @@ -9,8 +9,9 @@ use crate::{ Context, JsError, JsNativeError, JsObject, JsResult, JsValue, Module, }; -use boa_gc::{custom_trace, Finalize, Trace}; +use boa_gc::{custom_trace, Finalize, Gc, Trace}; use boa_profiler::Profiler; +use boa_string::JsString; use std::{future::Future, mem::size_of, ops::ControlFlow, pin::Pin, task}; #[cfg(feature = "trace")] @@ -52,7 +53,18 @@ mod tests; /// Virtual Machine. #[derive(Debug)] pub struct Vm { + /// The current call frame. + /// + /// Whenever a new frame is pushed, it will be swaped into this field. + /// Then the old frame will get pushed to the [`Self::frames`] stack. + /// Whenever the current frame gets poped, the last frame on the [`Self::frames`] stack will be swaped into this field. + /// + /// By default this is a dummy frame that gets pushed to [`Self::frames`] when the first real frame is pushed. + pub(crate) frame: CallFrame, + + /// The stack for call frames. pub(crate) frames: Vec, + pub(crate) stack: Vec, pub(crate) return_value: JsValue, @@ -100,6 +112,12 @@ impl Vm { pub(crate) fn new(realm: Realm) -> Self { Self { frames: Vec::with_capacity(16), + frame: CallFrame::new( + Gc::new(CodeBlock::new(JsString::default(), 0, true)), + None, + EnvironmentStack::new(realm.environment().clone()), + realm.clone(), + ), stack: Vec::with_capacity(1024), return_value: JsValue::undefined(), environments: EnvironmentStack::new(realm.environment().clone()), @@ -132,29 +150,22 @@ impl Vm { #[track_caller] pub(crate) fn read(&mut self) -> T { - let value = self.frame().code_block.read::(self.frame().pc as usize); - self.frame_mut().pc += size_of::() as u32; + let frame = self.frame_mut(); + let value = frame.code_block.read::(frame.pc as usize); + frame.pc += size_of::() as u32; value } - /// Retrieves the VM frame - /// - /// # Panics - /// - /// If there is no frame, then this will panic. + /// Retrieves the VM frame. #[track_caller] pub(crate) fn frame(&self) -> &CallFrame { - self.frames.last().expect("no frame found") + &self.frame } - /// Retrieves the VM frame mutably - /// - /// # Panics - /// - /// If there is no frame, then this will panic. + /// Retrieves the VM frame mutably. #[track_caller] pub(crate) fn frame_mut(&mut self) -> &mut CallFrame { - self.frames.last_mut().expect("no frame found") + &mut self.frame } pub(crate) fn push_frame(&mut self, mut frame: CallFrame) { @@ -177,9 +188,12 @@ impl Vm { // Keep carrying the last active runnable in case the current callframe // yields. if frame.active_runnable.is_none() { - frame.active_runnable = self.frames.last().and_then(|fr| fr.active_runnable.clone()); + frame + .active_runnable + .clone_from(&self.frame.active_runnable); } + std::mem::swap(&mut self.frame, &mut frame); self.frames.push(frame); } @@ -196,13 +210,14 @@ impl Vm { } pub(crate) fn pop_frame(&mut self) -> Option { - let mut frame = self.frames.pop(); - if let Some(frame) = &mut frame { + if let Some(mut frame) = self.frames.pop() { + std::mem::swap(&mut self.frame, &mut frame); std::mem::swap(&mut self.environments, &mut frame.environments); std::mem::swap(&mut self.realm, &mut frame.realm); + Some(frame) + } else { + None } - - frame } /// Handles an exception thrown at position `pc`. @@ -271,16 +286,17 @@ impl Context { const NUMBER_OF_COLUMNS: usize = 4; pub(crate) fn trace_call_frame(&self) { - let msg = if self.vm.frames.last().is_some() { + let frame = self.vm.frame(); + let msg = if self.vm.frames.is_empty() { + " VM Start ".to_string() + } else { format!( " Call Frame -- {} ", - self.vm.frame().code_block().name().to_std_string_escaped() + frame.code_block().name().to_std_string_escaped() ) - } else { - " VM Start ".to_string() }; - println!("{}", self.vm.frame().code_block); + println!("{}", frame.code_block); println!( "{msg:-^width$}", width = Self::COLUMN_WIDTH * Self::NUMBER_OF_COLUMNS - 10 @@ -300,16 +316,13 @@ impl Context { where F: FnOnce(Opcode, &mut Context) -> JsResult, { - let bytecodes = &self.vm.frame().code_block.bytecode; - let pc = self.vm.frame().pc as usize; + let frame = self.vm.frame(); + let bytecodes = &frame.code_block.bytecode; + let pc = frame.pc as usize; let (_, varying_operand_kind, instruction) = InstructionIterator::with_pc(bytecodes, pc) .next() .expect("There should be an instruction left"); - let operands = self - .vm - .frame() - .code_block - .instruction_operands(&instruction); + let operands = frame.code_block.instruction_operands(&instruction); let opcode = instruction.opcode(); match opcode { @@ -332,12 +345,11 @@ impl Context { let result = self.execute_instruction(f); let duration = instant.elapsed(); - let fp = self - .vm - .frames - .last() - .map(CallFrame::fp) - .map(|fp| fp as usize); + let fp = if self.vm.frames.is_empty() { + None + } else { + Some(self.vm.frame.fp() as usize) + }; let stack = { let mut stack = String::from("[ "); @@ -434,14 +446,16 @@ impl Context { if !err.is_catchable() { let mut fp = self.vm.stack.len(); let mut env_fp = self.vm.environments.len(); - while let Some(frame) = self.vm.frames.last() { - if frame.exit_early() { + loop { + if self.vm.frame.exit_early() { break; } - fp = frame.fp() as usize; - env_fp = frame.env_fp as usize; - self.vm.pop_frame(); + fp = self.vm.frame.fp() as usize; + env_fp = self.vm.frame.env_fp as usize; + if self.vm.pop_frame().is_none() { + break; + } } self.vm.environments.truncate(env_fp); self.vm.stack.truncate(fp); @@ -466,11 +480,13 @@ impl Context { match result { CompletionType::Normal => {} CompletionType::Return => { - let fp = self.vm.frame().fp() as usize; + let frame = self.vm.frame(); + let fp = frame.fp() as usize; + let exit_early = frame.exit_early(); self.vm.stack.truncate(fp); let result = self.vm.take_return_value(); - if self.vm.frame().exit_early() { + if exit_early { return ControlFlow::Break(CompletionRecord::Normal(result)); } @@ -478,9 +494,10 @@ impl Context { self.vm.pop_frame(); } CompletionType::Throw => { - let mut fp = self.vm.frame().fp(); - let mut env_fp = self.vm.frame().env_fp; - if self.vm.frame().exit_early() { + let frame = self.vm.frame(); + let mut fp = frame.fp(); + let mut env_fp = frame.env_fp; + if frame.exit_early() { self.vm.environments.truncate(env_fp as usize); self.vm.stack.truncate(fp as usize); return ControlFlow::Break(CompletionRecord::Throw( @@ -493,11 +510,11 @@ impl Context { self.vm.pop_frame(); - while let Some(frame) = self.vm.frames.last_mut() { - fp = frame.fp(); - env_fp = frame.env_fp; - let pc = frame.pc; - let exit_early = frame.exit_early(); + loop { + fp = self.vm.frame.fp(); + env_fp = self.vm.frame.env_fp; + let pc = self.vm.frame.pc; + let exit_early = self.vm.frame.exit_early(); if self.vm.handle_exception_at(pc) { return ControlFlow::Continue(()); @@ -512,7 +529,9 @@ impl Context { )); } - self.vm.pop_frame(); + if self.vm.pop_frame().is_none() { + break; + } } self.vm.environments.truncate(env_fp as usize); self.vm.stack.truncate(fp as usize); diff --git a/core/engine/src/vm/opcode/arguments.rs b/core/engine/src/vm/opcode/arguments.rs index 8149c84e1d..fb4fe7fb62 100644 --- a/core/engine/src/vm/opcode/arguments.rs +++ b/core/engine/src/vm/opcode/arguments.rs @@ -19,14 +19,13 @@ impl Operation for CreateMappedArgumentsObject { const COST: u8 = 8; fn execute(context: &mut Context) -> JsResult { - let function_object = context - .vm - .frame() + let frame = context.vm.frame(); + let function_object = frame .function(&context.vm) .clone() .expect("there should be a function object"); - let code = context.vm.frame().code_block().clone(); - let args = context.vm.frame().arguments(&context.vm).to_vec(); + let code = frame.code_block().clone(); + let args = frame.arguments(&context.vm).to_vec(); let env = context.vm.environments.current_ref(); let arguments = MappedArguments::new( diff --git a/core/engine/src/vm/opcode/await/mod.rs b/core/engine/src/vm/opcode/await/mod.rs index d957166978..1211b35581 100644 --- a/core/engine/src/vm/opcode/await/mod.rs +++ b/core/engine/src/vm/opcode/await/mod.rs @@ -183,9 +183,7 @@ impl Operation for CreatePromiseCapability { context .vm - .frames - .last() - .expect("there should be a frame") + .frame .set_promise_capability(&mut context.vm.stack, Some(&promise_capability)); Ok(CompletionType::Normal) } diff --git a/core/engine/src/vm/opcode/control_flow/return.rs b/core/engine/src/vm/opcode/control_flow/return.rs index 31e4294e0f..93e502b832 100644 --- a/core/engine/src/vm/opcode/control_flow/return.rs +++ b/core/engine/src/vm/opcode/control_flow/return.rs @@ -33,10 +33,10 @@ impl Operation for CheckReturn { const COST: u8 = 3; fn execute(context: &mut Context) -> JsResult { - if !context.vm.frame().construct() { + let frame = context.vm.frame(); + if !frame.construct() { return Ok(CompletionType::Normal); } - let frame = context.vm.frame(); let this = frame.this(&context.vm); let result = context.vm.take_return_value(); diff --git a/core/engine/src/vm/opcode/define/mod.rs b/core/engine/src/vm/opcode/define/mod.rs index 5c21d2b5ce..4af7365af4 100644 --- a/core/engine/src/vm/opcode/define/mod.rs +++ b/core/engine/src/vm/opcode/define/mod.rs @@ -62,13 +62,11 @@ pub(crate) struct DefInitVar; impl DefInitVar { fn operation(context: &mut Context, index: usize) -> JsResult { let value = context.vm.pop(); - let mut binding_locator = context.vm.frame().code_block.bindings[index].clone(); + let frame = context.vm.frame(); + let strict = frame.code_block.strict(); + let mut binding_locator = frame.code_block.bindings[index].clone(); context.find_runtime_binding(&mut binding_locator)?; - context.set_binding( - &binding_locator, - value, - context.vm.frame().code_block.strict(), - )?; + context.set_binding(&binding_locator, value, strict)?; Ok(CompletionType::Normal) } diff --git a/core/engine/src/vm/opcode/delete/mod.rs b/core/engine/src/vm/opcode/delete/mod.rs index 6150e61073..6940c861d1 100644 --- a/core/engine/src/vm/opcode/delete/mod.rs +++ b/core/engine/src/vm/opcode/delete/mod.rs @@ -16,15 +16,12 @@ impl DeletePropertyByName { fn operation(context: &mut Context, index: usize) -> JsResult { let value = context.vm.pop(); let object = value.to_object(context)?; - let key = context - .vm - .frame() - .code_block() - .constant_string(index) - .into(); + let code_block = context.vm.frame().code_block(); + let key = code_block.constant_string(index).into(); + let strict = code_block.strict(); let result = object.__delete__(&key, &mut InternalMethodContext::new(context))?; - if !result && context.vm.frame().code_block().strict() { + if !result && strict { return Err(JsNativeError::typ() .with_message("Cannot delete property") .into()); diff --git a/core/engine/src/vm/opcode/iteration/loop_ops.rs b/core/engine/src/vm/opcode/iteration/loop_ops.rs index 91d43c18e2..dc22333bef 100644 --- a/core/engine/src/vm/opcode/iteration/loop_ops.rs +++ b/core/engine/src/vm/opcode/iteration/loop_ops.rs @@ -17,16 +17,17 @@ impl Operation for IncrementLoopIteration { const COST: u8 = 3; fn execute(context: &mut Context) -> JsResult { - let previous_iteration_count = context.vm.frame_mut().loop_iteration_count; - let max = context.vm.runtime_limits.loop_iteration_limit(); + let frame = context.vm.frame_mut(); + let previous_iteration_count = frame.loop_iteration_count; + if previous_iteration_count > max { return Err(JsNativeError::runtime_limit() .with_message(format!("Maximum loop iteration limit {max} exceeded")) .into()); } - context.vm.frame_mut().loop_iteration_count = previous_iteration_count.wrapping_add(1); + frame.loop_iteration_count = previous_iteration_count.wrapping_add(1); Ok(CompletionType::Normal) } } diff --git a/core/engine/src/vm/opcode/push/literal.rs b/core/engine/src/vm/opcode/push/literal.rs index 697e626c29..e7b66c25bd 100644 --- a/core/engine/src/vm/opcode/push/literal.rs +++ b/core/engine/src/vm/opcode/push/literal.rs @@ -59,12 +59,9 @@ impl PushRegExp { pattern_index: usize, flags_index: usize, ) -> JsResult { - let pattern = context - .vm - .frame() - .code_block() - .constant_string(pattern_index); - let flags = context.vm.frame().code_block().constant_string(flags_index); + let code_block = context.vm.frame().code_block(); + let pattern = code_block.constant_string(pattern_index); + let flags = code_block.constant_string(flags_index); let regexp = JsRegExp::new(pattern, flags, context)?; context.vm.push(regexp); diff --git a/core/engine/src/vm/opcode/rest_parameter/mod.rs b/core/engine/src/vm/opcode/rest_parameter/mod.rs index daabd434f5..59db486fb4 100644 --- a/core/engine/src/vm/opcode/rest_parameter/mod.rs +++ b/core/engine/src/vm/opcode/rest_parameter/mod.rs @@ -17,8 +17,9 @@ impl Operation for RestParameterInit { const COST: u8 = 6; fn execute(context: &mut Context) -> JsResult { - let argument_count = context.vm.frame().argument_count; - let param_count = context.vm.frame().code_block().parameter_length; + let frame = context.vm.frame(); + let argument_count = frame.argument_count; + let param_count = frame.code_block().parameter_length; let array = if argument_count >= param_count { let rest_count = argument_count - param_count + 1; diff --git a/core/engine/src/vm/opcode/set/name.rs b/core/engine/src/vm/opcode/set/name.rs index a9646b53c0..421412bbc6 100644 --- a/core/engine/src/vm/opcode/set/name.rs +++ b/core/engine/src/vm/opcode/set/name.rs @@ -54,18 +54,16 @@ pub(crate) struct SetName; impl SetName { fn operation(context: &mut Context, index: usize) -> JsResult { - let mut binding_locator = context.vm.frame().code_block.bindings[index].clone(); + let code_block = context.vm.frame().code_block(); + let mut binding_locator = code_block.bindings[index].clone(); + let strict = code_block.strict(); let value = context.vm.pop(); context.find_runtime_binding(&mut binding_locator)?; verify_initialized(&binding_locator, context)?; - context.set_binding( - &binding_locator, - value, - context.vm.frame().code_block.strict(), - )?; + context.set_binding(&binding_locator, value, strict)?; Ok(CompletionType::Normal) } @@ -105,9 +103,9 @@ impl Operation for SetNameByLocator { const COST: u8 = 4; fn execute(context: &mut Context) -> JsResult { - let binding_locator = context - .vm - .frame_mut() + let frame = context.vm.frame_mut(); + let strict = frame.code_block.strict(); + let binding_locator = frame .binding_stack .pop() .expect("locator should have been popped before"); @@ -115,11 +113,7 @@ impl Operation for SetNameByLocator { verify_initialized(&binding_locator, context)?; - context.set_binding( - &binding_locator, - value, - context.vm.frame().code_block.strict(), - )?; + context.set_binding(&binding_locator, value, strict)?; Ok(CompletionType::Normal) }