Move arguments object creation to opcode (#3432)

boa_engine/src/bytecompiler/declarations.rs

@@ -3,6 +3,7 @@ use std::rc::Rc;
 use crate::{
     bytecompiler::{ByteCompiler, FunctionCompiler, FunctionSpec, NodeKind},
     environments::CompileTimeEnvironment,
+    js_string,
     vm::{create_function_object_fast, BindingOpcode, CodeBlockFlags, Opcode},
     JsNativeError, JsResult,
 };
@@ -285,6 +286,7 @@ impl ByteCompiler<'_, '_> {
             let function = create_function_object_fast(code, self.context);
 
             // c. Perform ? env.CreateGlobalFunctionBinding(fn, fo, false).
+            let name = js_string!(self.interner().resolve_expect(name.sym()).utf16());
             self.context
                 .create_global_function_binding(name, function, false)?;
         }
@@ -726,14 +728,17 @@ impl ByteCompiler<'_, '_> {
             // c. If varEnv is a Global Environment Record, then
             if var_env.is_global() {
                 // Ensures global functions are printed when generating the global flowgraph.
-                let _ = self.push_function_to_constants(code.clone());
+                let index = self.push_function_to_constants(code.clone());
 
                 // b. Let fo be InstantiateFunctionObject of f with arguments lexEnv and privateEnv.
-                let function = create_function_object_fast(code, self.context);
+                self.emit_with_varying_operand(Opcode::GetFunction, index);
 
                 // i. Perform ? varEnv.CreateGlobalFunctionBinding(fn, fo, true).
-                self.context
-                    .create_global_function_binding(name, function, true)?;
+                let name_index = self.get_or_insert_name(name);
+                self.emit(
+                    Opcode::CreateGlobalFunctionBinding,
+                    &[Operand::Bool(true), Operand::Varying(name_index)],
+                );
             }
             // d. Else,
             else {
@@ -915,14 +920,19 @@ impl ByteCompiler<'_, '_> {
         // NOTE(HalidOdat): Has been moved up, so "arguments" gets registed as
         //     the first binding in the environment with index 0.
         if arguments_object_needed {
-            // Note: This happens at runtime.
             // a. If strict is true or simpleParameterList is false, then
-            //     i. Let ao be CreateUnmappedArgumentsObject(argumentsList).
+            if strict || !formals.is_simple() {
+                // i. Let ao be CreateUnmappedArgumentsObject(argumentsList).
+                self.emit_opcode(Opcode::CreateUnmappedArgumentsObject);
+            }
             // b. Else,
-            //     i. NOTE: A mapped argument object is only provided for non-strict functions
-            //              that don't have a rest parameter, any parameter
-            //              default value initializers, or any destructured parameters.
-            //     ii. Let ao be CreateMappedArgumentsObject(func, formals, argumentsList, env).
+            else {
+                // i. NOTE: A mapped argument object is only provided for non-strict functions
+                //          that don't have a rest parameter, any parameter
+                //          default value initializers, or any destructured parameters.
+                // ii. Let ao be CreateMappedArgumentsObject(func, formals, argumentsList, env).
+                self.emit_opcode(Opcode::CreateMappedArgumentsObject);
+            }
 
             // c. If strict is true, then
             if strict {
@@ -937,7 +947,8 @@ impl ByteCompiler<'_, '_> {
                 env.create_mutable_binding(arguments, false);
             }
 
-            self.code_block_flags |= CodeBlockFlags::NEEDS_ARGUMENTS_OBJECT;
+            // e. Perform ! env.InitializeBinding("arguments", ao).
+            self.emit_binding(BindingOpcode::InitLexical, arguments);
         }
 
         // 21. For each String paramName of parameterNames, do
@@ -961,13 +972,10 @@ impl ByteCompiler<'_, '_> {
 
         // 22. If argumentsObjectNeeded is true, then
         if arguments_object_needed {
-            // MOVED: a-d.
+            // MOVED: a-e.
             //
             // NOTE(HalidOdat): Has been moved up, see comment above.
 
-            // Note: This happens at runtime.
-            // e. Perform ! env.InitializeBinding("arguments", ao).
-
             // f. Let parameterBindings be the list-concatenation of parameterNames and « "arguments" ».
             parameter_names.push(arguments);
         }

boa_engine/src/bytecompiler/mod.rs

@@ -275,7 +275,7 @@ pub struct ByteCompiler<'ctx, 'host> {
     json_parse: bool,
 
     // TODO: remove when we separate scripts from the context
-    context: &'ctx mut Context<'host>,
+    pub(crate) context: &'ctx mut Context<'host>,
 
     #[cfg(feature = "annex-b")]
     annex_b_function_names: Vec<Identifier>,

boa_engine/src/context/mod.rs

@@ -718,7 +718,7 @@ impl Context<'_> {
     /// [spec]: https://tc39.es/ecma262/#sec-createglobalfunctionbinding
     pub(crate) fn create_global_function_binding(
         &mut self,
-        name: Identifier,
+        name: JsString,
         function: JsObject,
         configurable: bool,
     ) -> JsResult<()> {
@@ -727,8 +727,7 @@ impl Context<'_> {
         let global_object = self.realm().global_object().clone();
 
         // 3. Let existingProp be ? globalObject.[[GetOwnProperty]](N).
-        let name = PropertyKey::from(self.interner().resolve_expect(name.sym()).utf16());
-        let existing_prop = global_object.__get_own_property__(&name, self)?;
+        let existing_prop = global_object.__get_own_property__(&name.clone().into(), self)?;
 
         // 4. If existingProp is undefined or existingProp.[[Configurable]] is true, then
         let desc = if existing_prop.is_none()

boa_engine/src/object/internal_methods/function.rs

@@ -1,5 +1,5 @@
 use crate::{
-    builtins::function::{arguments::Arguments, ThisMode},
+    builtins::function::ThisMode,
     context::intrinsics::StandardConstructors,
     environments::{FunctionSlots, ThisBindingStatus},
     native_function::NativeFunctionObject,
@@ -121,39 +121,6 @@ pub(crate) fn function_call(
             .push_lexical(code.constant_compile_time_environment(last_env));
     }
 
-    // Taken from: `FunctionDeclarationInstantiation` abstract function.
-    //
-    // Spec: https://tc39.es/ecma262/#sec-functiondeclarationinstantiation
-    //
-    // 22. If argumentsObjectNeeded is true, then
-    if code.needs_arguments_object() {
-        // a. If strict is true or simpleParameterList is false, then
-        //     i. Let ao be CreateUnmappedArgumentsObject(argumentsList).
-        // b. Else,
-        //     i. NOTE: A mapped argument object is only provided for non-strict functions
-        //              that don't have a rest parameter, any parameter
-        //              default value initializers, or any destructured parameters.
-        //     ii. Let ao be CreateMappedArgumentsObject(func, formals, argumentsList, env).
-        let args = context.vm.stack[(fp + CallFrame::FIRST_ARGUMENT_POSITION)..].to_vec();
-        let arguments_obj = if code.strict() || !code.params.is_simple() {
-            Arguments::create_unmapped_arguments_object(&args, context)
-        } else {
-            let env = context.vm.environments.current();
-            Arguments::create_mapped_arguments_object(
-                function_object,
-                &code.params,
-                &args,
-                env.declarative_expect(),
-                context,
-            )
-        };
-        let env_index = context.vm.environments.len() as u32 - 1;
-        context
-            .vm
-            .environments
-            .put_lexical_value(env_index, 0, arguments_obj.into());
-    }
-
     Ok(CallValue::Ready)
 }
 
@@ -257,39 +224,6 @@ fn function_construct(
             .push_lexical(code.constant_compile_time_environment(last_env));
     }
 
-    // Taken from: `FunctionDeclarationInstantiation` abstract function.
-    //
-    // Spec: https://tc39.es/ecma262/#sec-functiondeclarationinstantiation
-    //
-    // 22. If argumentsObjectNeeded is true, then
-    if code.needs_arguments_object() {
-        // a. If strict is true or simpleParameterList is false, then
-        //     i. Let ao be CreateUnmappedArgumentsObject(argumentsList).
-        // b. Else,
-        //     i. NOTE: A mapped argument object is only provided for non-strict functions
-        //              that don't have a rest parameter, any parameter
-        //              default value initializers, or any destructured parameters.
-        //     ii. Let ao be CreateMappedArgumentsObject(func, formals, argumentsList, env).
-        let args = context.vm.stack[at..].to_vec();
-        let arguments_obj = if code.strict() || !code.params.is_simple() {
-            Arguments::create_unmapped_arguments_object(&args, context)
-        } else {
-            let env = context.vm.environments.current();
-            Arguments::create_mapped_arguments_object(
-                this_function_object,
-                &code.params,
-                &args,
-                env.declarative_expect(),
-                context,
-            )
-        };
-        let env_index = context.vm.environments.len() as u32 - 1;
-        context
-            .vm
-            .environments
-            .put_lexical_value(env_index, 0, arguments_obj.into());
-    }
-
     // Insert `this` value
     context
         .vm

boa_engine/src/vm/code_block.rs

@@ -59,22 +59,17 @@ bitflags! {
         /// Does this function have a parameters environment.
         const PARAMETERS_ENV_BINDINGS = 0b0000_1000;
 
-        /// Does this function need a `"arguments"` object.
-        ///
-        /// The `"arguments"` binding is the first binding.
-        const NEEDS_ARGUMENTS_OBJECT = 0b0001_0000;
-
         /// The `[[ClassFieldInitializerName]]` internal slot.
-        const IN_CLASS_FIELD_INITIALIZER = 0b0010_0000;
+        const IN_CLASS_FIELD_INITIALIZER = 0b0001_0000;
 
         /// `[[ConstructorKind]]`
-        const IS_DERIVED_CONSTRUCTOR = 0b0100_0000;
+        const IS_DERIVED_CONSTRUCTOR = 0b0010_0000;
 
-        const IS_ASYNC = 0b1000_0000;
-        const IS_GENERATOR = 0b0001_0000_0000;
+        const IS_ASYNC = 0b0100_0000;
+        const IS_GENERATOR = 0b0000_1000_0000;
 
         /// Arrow and method functions don't have `"prototype"` property.
-        const HAS_PROTOTYPE_PROPERTY = 0b0010_0000_0000;
+        const HAS_PROTOTYPE_PROPERTY = 0b0001_0000_0000;
 
         /// Trace instruction execution to `stdout`.
         #[cfg(feature = "trace")]
@@ -233,13 +228,6 @@ impl CodeBlock {
             .contains(CodeBlockFlags::PARAMETERS_ENV_BINDINGS)
     }
 
-    /// Does this function need a `"arguments"` object.
-    pub(crate) fn needs_arguments_object(&self) -> bool {
-        self.flags
-            .get()
-            .contains(CodeBlockFlags::NEEDS_ARGUMENTS_OBJECT)
-    }
-
     /// Does this function have the `[[ClassFieldInitializerName]]` internal slot set to non-empty value.
     pub(crate) fn in_class_field_initializer(&self) -> bool {
         self.flags
@@ -526,6 +514,15 @@ impl CodeBlock {
             Instruction::CreateIteratorResult { done } => {
                 format!("done: {done}")
             }
+            Instruction::CreateGlobalFunctionBinding {
+                name_index,
+                configurable,
+            } => {
+                let name = self
+                    .constant_string(name_index.value() as usize)
+                    .to_std_string_escaped();
+                format!("name: {name}, configurable: {configurable}")
+            }
             Instruction::Pop
             | Instruction::Dup
             | Instruction::Swap
@@ -646,6 +643,8 @@ impl CodeBlock {
             | Instruction::GetReturnValue
             | Instruction::SetReturnValue
             | Instruction::BindThisValue
+            | Instruction::CreateMappedArgumentsObject
+            | Instruction::CreateUnmappedArgumentsObject
             | Instruction::Nop => String::new(),
 
             Instruction::U16Operands
@@ -707,10 +706,7 @@ impl CodeBlock {
             | Instruction::Reserved55
             | Instruction::Reserved56
             | Instruction::Reserved57
-            | Instruction::Reserved58
-            | Instruction::Reserved59
-            | Instruction::Reserved60
-            | Instruction::Reserved61 => unreachable!("Reserved opcodes are unrechable"),
+            | Instruction::Reserved58 => unreachable!("Reserved opcodes are unrechable"),
         }
     }
 }

boa_engine/src/vm/flowgraph/mod.rs

@@ -451,6 +451,9 @@ impl CodeBlock {
                 | Instruction::MaybeException
                 | Instruction::CheckReturn
                 | Instruction::BindThisValue
+                | Instruction::CreateMappedArgumentsObject
+                | Instruction::CreateUnmappedArgumentsObject
+                | Instruction::CreateGlobalFunctionBinding { .. }
                 | Instruction::Nop => {
                     graph.add_node(previous_pc, NodeShape::None, label.into(), Color::None);
                     graph.add_edge(previous_pc, pc, None, Color::None, EdgeStyle::Line);
@@ -517,10 +520,7 @@ impl CodeBlock {
                 | Instruction::Reserved55
                 | Instruction::Reserved56
                 | Instruction::Reserved57
-                | Instruction::Reserved58
-                | Instruction::Reserved59
-                | Instruction::Reserved60
-                | Instruction::Reserved61 => unreachable!("Reserved opcodes are unrechable"),
+                | Instruction::Reserved58 => unreachable!("Reserved opcodes are unrechable"),
             }
         }
 

boa_engine/src/vm/opcode/arguments.rs

@@ -0,0 +1,64 @@
+use crate::{
+    builtins::function::arguments::Arguments,
+    vm::{CallFrame, CompletionType},
+    Context, JsResult,
+};
+
+use super::Operation;
+
+/// `CreateMappedArgumentsObject` implements the Opcode Operation for `Opcode::CreateMappedArgumentsObject`
+///
+/// Operation:
+///  - TODO: doc
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct CreateMappedArgumentsObject;
+
+impl Operation for CreateMappedArgumentsObject {
+    const NAME: &'static str = "CreateMappedArgumentsObject";
+    const INSTRUCTION: &'static str = "INST - CreateMappedArgumentsObject";
+    const COST: u8 = 8;
+
+    fn execute(context: &mut Context<'_>) -> JsResult<CompletionType> {
+        let arguments_start = context.vm.frame().fp as usize + CallFrame::FIRST_ARGUMENT_POSITION;
+        let function_object = context
+            .vm
+            .frame()
+            .function(&context.vm)
+            .clone()
+            .expect("there should be a function object");
+        let code = context.vm.frame().code_block().clone();
+        let args = context.vm.stack[arguments_start..].to_vec();
+
+        let env = context.vm.environments.current();
+        let arguments = Arguments::create_mapped_arguments_object(
+            &function_object,
+            &code.params,
+            &args,
+            env.declarative_expect(),
+            context,
+        );
+        context.vm.push(arguments);
+        Ok(CompletionType::Normal)
+    }
+}
+
+/// `CreateUnmappedArgumentsObject` implements the Opcode Operation for `Opcode::CreateUnmappedArgumentsObject`
+///
+/// Operation:
+///  - TODO: doc
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct CreateUnmappedArgumentsObject;
+
+impl Operation for CreateUnmappedArgumentsObject {
+    const NAME: &'static str = "CreateUnmappedArgumentsObject";
+    const INSTRUCTION: &'static str = "INST - CreateUnmappedArgumentsObject";
+    const COST: u8 = 4;
+
+    fn execute(context: &mut Context<'_>) -> JsResult<CompletionType> {
+        let arguments_start = context.vm.frame().fp as usize + CallFrame::FIRST_ARGUMENT_POSITION;
+        let args = context.vm.stack[arguments_start..].to_vec();
+        let arguments = Arguments::create_unmapped_arguments_object(&args, context);
+        context.vm.push(arguments);
+        Ok(CompletionType::Normal)
+    }
+}