From d9f877dc9ac6901882b0b88aafd1a99e84fcfdb0 Mon Sep 17 00:00:00 2001 From: Lord-McSweeney Date: Thu, 16 Jan 2025 10:45:49 -0800 Subject: [PATCH 1/2] avm2: Store locals on AVM stack instead of per-Activation RegisterSets --- core/src/avm2.rs | 95 +++++++++++++-------- core/src/avm2/activation.rs | 161 +++++++++++++++++------------------- core/src/avm2/function.rs | 7 +- core/src/avm2/optimize.rs | 14 +--- 4 files changed, 145 insertions(+), 132 deletions(-) diff --git a/core/src/avm2.rs b/core/src/avm2.rs index ad35cdea5df3..77604c980917 100644 --- a/core/src/avm2.rs +++ b/core/src/avm2.rs @@ -296,44 +296,53 @@ impl<'gc> Avm2<'gc> { ) -> Result<(), Error<'gc>> { let mut init_activation = Activation::from_script(context, script)?; - let (method, scope, _domain) = script.init(); - match method { - Method::Native(method) => { - if method.resolved_signature.read().is_none() { - method.resolve_signature(&mut init_activation)?; + // Execute everything in a closure so we can run `cleanup` more easily. + let mut closure = || -> Result<(), Error<'gc>> { + let (method, scope, _domain) = script.init(); + match method { + Method::Native(method) => { + if method.resolved_signature.read().is_none() { + method.resolve_signature(&mut init_activation)?; + } + + let resolved_signature = method.resolved_signature.read(); + let resolved_signature = resolved_signature.as_ref().unwrap(); + + // This exists purely to check if the builtin is OK with being called with + // no parameters. + init_activation.resolve_parameters( + Method::Native(method), + &[], + resolved_signature, + None, + )?; + init_activation + .context + .avm2 + .push_global_init(init_activation.gc(), script); + let r = (method.method)(&mut init_activation, Value::Object(scope), &[]); + init_activation.context.avm2.pop_call(init_activation.gc()); + r?; + } + Method::Bytecode(method) => { + init_activation + .context + .avm2 + .push_global_init(init_activation.gc(), script); + let r = init_activation.run_actions(method); + init_activation.context.avm2.pop_call(init_activation.gc()); + r?; } + }; - let resolved_signature = method.resolved_signature.read(); - let resolved_signature = resolved_signature.as_ref().unwrap(); - - // This exists purely to check if the builtin is OK with being called with - // no parameters. - init_activation.resolve_parameters( - Method::Native(method), - &[], - resolved_signature, - None, - )?; - init_activation - .context - .avm2 - .push_global_init(init_activation.gc(), script); - let r = (method.method)(&mut init_activation, Value::Object(scope), &[]); - init_activation.context.avm2.pop_call(init_activation.gc()); - r?; - } - Method::Bytecode(method) => { - init_activation - .context - .avm2 - .push_global_init(init_activation.gc(), script); - let r = init_activation.run_actions(method); - init_activation.context.avm2.pop_call(init_activation.gc()); - r?; - } + Ok(()) }; - Ok(()) + let result = closure(); + + init_activation.cleanup(); + + result } fn orphan_objects_mut(&mut self) -> &mut Vec> { @@ -776,7 +785,7 @@ impl<'gc> Avm2<'gc> { /// Peek the n-th value from the end of the operand stack. #[inline(always)] - fn peek(&mut self, index: usize) -> Value<'gc> { + fn peek(&self, index: usize) -> Value<'gc> { let value = self.stack[self.stack.len() - index - 1]; avm_debug!(self, "Stack peek {}: {value:?}", self.stack.len()); @@ -784,6 +793,22 @@ impl<'gc> Avm2<'gc> { value } + #[inline(always)] + fn stack_at(&self, index: usize) -> Value<'gc> { + let value = self.stack[index]; + + avm_debug!(self, "Stack peek {}: {value:?}", index); + + value + } + + #[inline(always)] + fn set_stack_at(&mut self, index: usize, value: Value<'gc>) { + avm_debug!(self, "Stack poke {}: {value:?}", index); + + self.stack[index] = value; + } + fn pop_args(&mut self, arg_count: u32) -> Vec> { let mut args = vec![Value::Undefined; arg_count as usize]; for arg in args.iter_mut().rev() { diff --git a/core/src/avm2/activation.rs b/core/src/avm2/activation.rs index 555b48d9740b..57173d633fbe 100644 --- a/core/src/avm2/activation.rs +++ b/core/src/avm2/activation.rs @@ -24,7 +24,6 @@ use crate::context::UpdateContext; use crate::string::{AvmAtom, AvmString, StringContext}; use crate::tag_utils::SwfMovie; use gc_arena::Gc; -use smallvec::SmallVec; use std::cmp::{min, Ordering}; use std::sync::Arc; use swf::avm2::types::{ @@ -33,37 +32,6 @@ use swf::avm2::types::{ use super::error::make_mismatch_error; -/// Represents a particular register set. -/// -/// This type exists primarily because SmallVec isn't garbage-collectable. -pub struct RegisterSet<'gc>(SmallVec<[Value<'gc>; 8]>); - -unsafe impl gc_arena::Collect for RegisterSet<'_> { - #[inline] - fn trace(&self, cc: &gc_arena::Collection) { - for register in &self.0 { - register.trace(cc); - } - } -} - -impl<'gc> RegisterSet<'gc> { - /// Create a new register set with a given number of specified registers. - /// - /// The given registers will be set to `undefined`. - pub fn new(num: u32) -> Self { - Self(smallvec![Value::Undefined; num as usize]) - } - - pub fn get_unchecked(&self, num: u32) -> Value<'gc> { - self.0[num as usize] - } - - pub fn get_unchecked_mut(&mut self, num: u32) -> &mut Value<'gc> { - self.0.get_mut(num as usize).unwrap() - } -} - #[derive(Clone)] enum FrameControl<'gc> { Continue, @@ -76,13 +44,10 @@ pub struct Activation<'a, 'gc: 'a> { ip: i32, /// Amount of actions performed since the last timeout check - actions_since_timeout_check: u16, + actions_since_timeout_check: u32, - /// Local registers. - /// - /// All activations have local registers, but it is possible for multiple - /// activations (such as a rescope) to execute from the same register set. - local_registers: RegisterSet<'gc>, + /// The number of locals this method uses. + num_locals: usize, /// This represents the outer scope of the method that is executing. /// @@ -159,12 +124,10 @@ impl<'a, 'gc> Activation<'a, 'gc> { /// It is a logic error to attempt to run AVM2 code in a nothing /// `Activation`. pub fn from_nothing(context: &'a mut UpdateContext<'gc>) -> Self { - let local_registers = RegisterSet::new(0); - Self { ip: 0, actions_since_timeout_check: 0, - local_registers, + num_locals: 0, outer: ScopeChain::new(context.avm2.stage_domain), caller_domain: None, caller_movie: None, @@ -187,12 +150,10 @@ impl<'a, 'gc> Activation<'a, 'gc> { /// action you're performing. When running frame scripts, this is the /// `SwfMovie` associated with the `MovieClip` being processed. pub fn from_domain(context: &'a mut UpdateContext<'gc>, domain: Domain<'gc>) -> Self { - let local_registers = RegisterSet::new(0); - Self { ip: 0, actions_since_timeout_check: 0, - local_registers, + num_locals: 0, outer: ScopeChain::new(context.avm2.stage_domain), caller_domain: Some(domain), caller_movie: None, @@ -214,23 +175,20 @@ impl<'a, 'gc> Activation<'a, 'gc> { let (method, global_object, domain) = script.init(); let num_locals = match method { - Method::Native { .. } => 0, + Method::Native { .. } => 1, Method::Bytecode(bytecode) => { let body = bytecode .body() - .ok_or("Cannot execute non-native method (for script) without body")?; + .expect("Cannot execute non-native method (for script) without body"); - body.num_locals + body.num_locals as usize } }; - let mut local_registers = RegisterSet::new(num_locals + 1); - - *local_registers.get_unchecked_mut(0) = global_object.into(); let activation_class = if let Method::Bytecode(method) = method { let body = method .body() - .ok_or("Cannot execute non-native method (for script) without body")?; + .expect("Cannot execute non-native method (for script) without body"); BytecodeMethod::get_or_init_activation_class(method, context.gc(), || { let translation_unit = method.translation_unit(); @@ -253,7 +211,7 @@ impl<'a, 'gc> Activation<'a, 'gc> { let mut created_activation = Self { ip: 0, actions_since_timeout_check: 0, - local_registers, + num_locals, outer: ScopeChain::new(domain), caller_domain: Some(domain), caller_movie: script.translation_unit().map(|t| t.movie()), @@ -272,6 +230,12 @@ impl<'a, 'gc> Activation<'a, 'gc> { } } + // Create locals- script init methods are run with no parameters passed + created_activation.push_stack(global_object); + for _ in 0..num_locals - 1 { + created_activation.push_stack(Value::Undefined); + } + Ok(created_activation) } @@ -401,15 +365,12 @@ impl<'a, 'gc> Activation<'a, 'gc> { bound_class: Option>, callee: Value<'gc>, ) -> Result<(), Error<'gc>> { - let body: Result<_, Error<'gc>> = method + let body = method .body() - .ok_or_else(|| "Cannot execute non-native method without body".into()); - let body = body?; - let num_locals = body.num_locals; - let has_rest_or_args = method.is_variadic(); + .expect("Cannot execute non-native method without body"); - let mut local_registers = RegisterSet::new(num_locals + 1); - *local_registers.get_unchecked_mut(0) = this; + let num_locals = body.num_locals as usize; + let has_rest_or_args = method.is_variadic(); let activation_class = BytecodeMethod::get_or_init_activation_class(method, self.gc(), || { @@ -426,9 +387,13 @@ impl<'a, 'gc> Activation<'a, 'gc> { ClassObject::from_class(&mut dummy_activation, activation_class, None) })?; + if let Some(bound_class) = bound_class { + assert!(this.is_of_type(self, bound_class)); + } + self.ip = 0; self.actions_since_timeout_check = 0; - self.local_registers = local_registers; + self.num_locals = num_locals; self.outer = outer; self.caller_domain = Some(outer.domain()); self.caller_movie = Some(method.owner_movie()); @@ -463,13 +428,13 @@ impl<'a, 'gc> Activation<'a, 'gc> { bound_class, )?; - { - for (i, arg) in arguments_list[0..min(signature.len(), arguments_list.len())] - .iter() - .enumerate() - { - *self.local_registers.get_unchecked_mut(1 + i as u32) = *arg; - } + // Create locals + self.push_stack(this); + + let num_args = min(signature.len(), arguments_list.len()); + + for arg in &arguments_list[0..num_args] { + self.push_stack(*arg); } if has_rest_or_args { @@ -501,9 +466,17 @@ impl<'a, 'gc> Activation<'a, 'gc> { args_object.set_local_property_is_enumerable(self.gc(), "callee".into(), false); } - *self - .local_registers - .get_unchecked_mut(1 + signature.len() as u32) = args_object.into(); + self.push_stack(args_object); + } + + // Locals not including the `this` value or arguments. + let mut extra_locals = num_locals - num_args - 1; + if has_rest_or_args { + extra_locals -= 1; + } + + for _ in 0..extra_locals { + self.push_stack(Value::Undefined); } Ok(()) @@ -523,12 +496,10 @@ impl<'a, 'gc> Activation<'a, 'gc> { caller_domain: Option>, caller_movie: Option>, ) -> Self { - let local_registers = RegisterSet::new(0); - Self { ip: 0, actions_since_timeout_check: 0, - local_registers, + num_locals: 0, outer, caller_domain, caller_movie, @@ -557,15 +528,20 @@ impl<'a, 'gc> Activation<'a, 'gc> { } /// Retrieve a local register. - pub fn local_register(&self, id: u32) -> Value<'gc> { - // Verification guarantees that this is valid - self.local_registers.get_unchecked(id) + pub fn local_register(&mut self, id: u32) -> Value<'gc> { + let stack_depth = self.stack_depth; + + // Verification guarantees that this points to a local register + self.avm2().stack_at(stack_depth + id as usize) } /// Set a local register. pub fn set_local_register(&mut self, id: u32, value: impl Into>) { + let stack_depth = self.stack_depth; + // Verification guarantees that this is valid - *self.local_registers.get_unchecked_mut(id) = value.into(); + self.avm2() + .set_stack_at(stack_depth + id as usize, value.into()); } /// Retrieve the outer scope of this activation @@ -658,16 +634,35 @@ impl<'a, 'gc> Activation<'a, 'gc> { self.avm2().pop_scope(); } + /// Cleans up after this Activation. This removes stack and local entries, + /// and clears the scope stack. This method must be called after an Activation + /// created with `Activation::init_from_activation` or `Activation::from_script` + /// is done executing; otherwise, old values may accumulate on the stack + /// (and not get GC-ed). + #[inline] + pub fn cleanup(&mut self) { + self.clear_stack_and_locals(); + self.clear_scope(); + } + /// Clears the operand stack used by this activation. #[inline] - pub fn clear_stack(&mut self) { + fn clear_stack(&mut self) { + let stack_depth = self.stack_depth; + let num_locals = self.num_locals; + self.avm2().truncate_stack(stack_depth + num_locals); + } + + /// Clears the operand stack and locals used by this activation. + #[inline] + fn clear_stack_and_locals(&mut self) { let stack_depth = self.stack_depth; self.avm2().truncate_stack(stack_depth); } /// Clears the scope stack used by this activation. #[inline] - pub fn clear_scope(&mut self) { + fn clear_scope(&mut self) { let scope_depth = self.scope_depth; self.avm2().scope_stack.truncate(scope_depth); } @@ -734,8 +729,6 @@ impl<'a, 'gc> Activation<'a, 'gc> { } }; - self.clear_stack(); - self.clear_scope(); val } @@ -1093,7 +1086,9 @@ impl<'a, 'gc> Activation<'a, 'gc> { } fn op_get_local(&mut self, register_index: u32) -> Result, Error<'gc>> { - self.push_stack(self.local_register(register_index)); + let value = self.local_register(register_index); + self.push_stack(value); + Ok(FrameControl::Continue) } @@ -3108,7 +3103,7 @@ impl<'a, 'gc> Activation<'a, 'gc> { register: u8, ) -> Result, Error<'gc>> { if is_local_register { - if (register as usize) < self.local_registers.0.len() { + if (register as usize) < self.num_locals { let value = self.local_register(register as u32); avm_debug!(self.avm2(), "Debug: {register_name} = {value:?}"); diff --git a/core/src/avm2/function.rs b/core/src/avm2/function.rs index 2d7c4f32700c..2d1b0687b0ab 100644 --- a/core/src/avm2/function.rs +++ b/core/src/avm2/function.rs @@ -248,7 +248,12 @@ pub fn exec<'gc>( .context .avm2 .push_call(activation.gc(), method, bound_class); - activation.run_actions(bm) + + let result = activation.run_actions(bm); + + activation.cleanup(); + + result } }; activation.context.avm2.pop_call(activation.gc()); diff --git a/core/src/avm2/optimize.rs b/core/src/avm2/optimize.rs index 013229842dfd..9bead2ff2417 100644 --- a/core/src/avm2/optimize.rs +++ b/core/src/avm2/optimize.rs @@ -502,19 +502,7 @@ pub fn optimize<'gc>( .body() .expect("Cannot verify non-native method without body!"); - // This can probably be done better by recording the receiver in `Activation`, - // but this works since it's guaranteed to be set in `Activation::from_method`. - let this_value = activation.local_register(0); - - let this_class = if let Some(this_class) = activation.bound_class() { - if this_value.is_of_type(activation, this_class) { - Some(this_class) - } else { - None - } - } else { - None - }; + let this_class = activation.bound_class(); let this_value = OptValue { class: this_class, From d780308b2ad7422ec94e156dc800242bb8cf9bd6 Mon Sep 17 00:00:00 2001 From: Lord-McSweeney Date: Sat, 18 Jan 2025 12:27:39 -0800 Subject: [PATCH 2/2] avm2: Increase actions before timeout check --- core/src/avm2/activation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/avm2/activation.rs b/core/src/avm2/activation.rs index 57173d633fbe..7d02a06643e4 100644 --- a/core/src/avm2/activation.rs +++ b/core/src/avm2/activation.rs @@ -784,7 +784,7 @@ impl<'a, 'gc> Activation<'a, 'gc> { opcodes: &[Op<'gc>], ) -> Result, Error<'gc>> { self.actions_since_timeout_check += 1; - if self.actions_since_timeout_check >= 64000 { + if self.actions_since_timeout_check >= 200000 { self.actions_since_timeout_check = 0; if self.context.update_start.elapsed() >= self.context.max_execution_duration { return Err(