From 4fa57480932965874146b771e5078460ee071655 Mon Sep 17 00:00:00 2001 From: Linus Groh Date: Tue, 5 Oct 2021 08:44:58 +0100 Subject: [PATCH] LibJS: Add an optimization to avoid needless arguments object creation This gives FunctionNode a "might need arguments object" boolean flag and sets it based on the simplest possible heuristic for this: if we encounter an identifier called "arguments" or "eval" up to the next (nested) function declaration or expression, we won't need an arguments object. Otherwise, we *might* need one - the final decision is made in the FunctionDeclarationInstantiation AO. Now, this is obviously not perfect. Even if you avoid eval, something like `foo.arguments` will still trigger a false positive - but it's a start and already massively cuts down on needlessly allocated objects, especially in real-world code that is often minified, and so a full "arguments" identifier will be an actual arguments object more often than not. To illustrate the actual impact of this change, here's the number of allocated arguments objects during a full test-js run: Before: - Unmapped arguments objects: 78765 - Mapped arguments objects: 2455 After: - Unmapped arguments objects: 18 - Mapped arguments objects: 37 This results in a ~5% speedup of test-js on my Linux host machine, and about 3.5% on i686 Serenity in QEMU (warm runs, average of 5). The following microbenchmark (calling an empty function 1M times) runs 25% faster on Linux and 45% on Serenity: function foo() {} for (var i = 0; i < 1_000_000; ++i) foo(); test262 reports no changes in either direction, apart from a speedup :^) --- Userland/Libraries/LibJS/AST.cpp | 8 +++---- Userland/Libraries/LibJS/AST.h | 17 +++++++++------ Userland/Libraries/LibJS/Bytecode/Op.cpp | 2 +- Userland/Libraries/LibJS/Parser.cpp | 21 +++++++++++++------ Userland/Libraries/LibJS/Parser.h | 1 + .../LibJS/Runtime/AbstractOperations.cpp | 2 +- .../Runtime/ECMAScriptFunctionObject.cpp | 15 ++++++++----- .../LibJS/Runtime/ECMAScriptFunctionObject.h | 5 +++-- .../Runtime/GeneratorFunctionConstructor.cpp | 2 +- 9 files changed, 47 insertions(+), 26 deletions(-) diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp index 883ba0259f..5cd3a2d7c9 100644 --- a/Userland/Libraries/LibJS/AST.cpp +++ b/Userland/Libraries/LibJS/AST.cpp @@ -187,7 +187,7 @@ Value FunctionExpression::instantiate_ordinary_function_expression(Interpreter& scope->create_immutable_binding(global_object, name(), false); } - auto closure = ECMAScriptFunctionObject::create(global_object, used_name, body(), parameters(), function_length(), scope, kind(), is_strict_mode(), is_arrow_function()); + auto closure = ECMAScriptFunctionObject::create(global_object, used_name, body(), parameters(), function_length(), scope, kind(), is_strict_mode(), might_need_arguments_object(), is_arrow_function()); // FIXME: 6. Perform SetFunctionName(closure, name). // FIXME: 7. Perform MakeConstructor(closure). @@ -1289,7 +1289,7 @@ ThrowCompletionOr ClassExpression::class_definition_evaluation(Interprete auto copy_initializer = field.initializer(); auto body = create_ast_node(field.initializer()->source_range(), copy_initializer.release_nonnull()); // FIXME: A potential optimization is not creating the functions here since these are never directly accessible. - initializer = ECMAScriptFunctionObject::create(interpreter.global_object(), property_key.to_display_string(), *body, {}, 0, interpreter.lexical_environment(), FunctionKind::Regular, false); + initializer = ECMAScriptFunctionObject::create(interpreter.global_object(), property_key.to_display_string(), *body, {}, 0, interpreter.lexical_environment(), FunctionKind::Regular, false, false); initializer->set_home_object(field.is_static() ? class_constructor : &class_prototype.as_object()); } @@ -3169,7 +3169,7 @@ void ScopeNode::block_declaration_instantiation(GlobalObject& global_object, Env if (is(declaration)) { auto& function_declaration = static_cast(declaration); - auto* function = ECMAScriptFunctionObject::create(global_object, function_declaration.name(), function_declaration.body(), function_declaration.parameters(), function_declaration.function_length(), environment, function_declaration.kind(), function_declaration.is_strict_mode()); + auto* function = ECMAScriptFunctionObject::create(global_object, function_declaration.name(), function_declaration.body(), function_declaration.parameters(), function_declaration.function_length(), environment, function_declaration.kind(), function_declaration.is_strict_mode(), function_declaration.might_need_arguments_object()); VERIFY(is(*environment)); static_cast(*environment).initialize_or_set_mutable_binding({}, global_object, function_declaration.name(), function); } @@ -3313,7 +3313,7 @@ ThrowCompletionOr Program::global_declaration_instantiation(Interpreter& i }); for (auto& declaration : functions_to_initialize) { - auto* function = ECMAScriptFunctionObject::create(global_object, declaration.name(), declaration.body(), declaration.parameters(), declaration.function_length(), &global_environment, declaration.kind(), declaration.is_strict_mode()); + auto* function = ECMAScriptFunctionObject::create(global_object, declaration.name(), declaration.body(), declaration.parameters(), declaration.function_length(), &global_environment, declaration.kind(), declaration.is_strict_mode(), declaration.might_need_arguments_object()); global_environment.create_global_function_binding(declaration.name(), function, false); if (auto* exception = interpreter.exception()) return throw_completion(exception->value()); diff --git a/Userland/Libraries/LibJS/AST.h b/Userland/Libraries/LibJS/AST.h index def672b0bb..f44c44a991 100644 --- a/Userland/Libraries/LibJS/AST.h +++ b/Userland/Libraries/LibJS/AST.h @@ -432,19 +432,23 @@ public: Vector const& parameters() const { return m_parameters; }; i32 function_length() const { return m_function_length; } bool is_strict_mode() const { return m_is_strict_mode; } + bool might_need_arguments_object() const { return m_might_need_arguments_object; } bool is_arrow_function() const { return m_is_arrow_function; } FunctionKind kind() const { return m_kind; } protected: - FunctionNode(FlyString name, NonnullRefPtr body, Vector parameters, i32 function_length, FunctionKind kind, bool is_strict_mode, bool is_arrow_function) + FunctionNode(FlyString name, NonnullRefPtr body, Vector parameters, i32 function_length, FunctionKind kind, bool is_strict_mode, bool might_need_arguments_object, bool is_arrow_function) : m_name(move(name)) , m_body(move(body)) , m_parameters(move(parameters)) , m_function_length(function_length) , m_kind(kind) , m_is_strict_mode(is_strict_mode) + , m_might_need_arguments_object(might_need_arguments_object) , m_is_arrow_function(is_arrow_function) { + if (m_is_arrow_function) + VERIFY(!m_might_need_arguments_object); } void dump(int indent, String const& class_name) const; @@ -462,7 +466,8 @@ private: Vector const m_parameters; const i32 m_function_length; FunctionKind m_kind; - bool m_is_strict_mode; + bool m_is_strict_mode { false }; + bool m_might_need_arguments_object { false }; bool m_is_arrow_function { false }; }; @@ -472,9 +477,9 @@ class FunctionDeclaration final public: static bool must_have_name() { return true; } - FunctionDeclaration(SourceRange source_range, FlyString const& name, NonnullRefPtr body, Vector parameters, i32 function_length, FunctionKind kind, bool is_strict_mode = false) + FunctionDeclaration(SourceRange source_range, FlyString const& name, NonnullRefPtr body, Vector parameters, i32 function_length, FunctionKind kind, bool is_strict_mode, bool might_need_arguments_object) : Declaration(source_range) - , FunctionNode(name, move(body), move(parameters), function_length, kind, is_strict_mode, false) + , FunctionNode(name, move(body), move(parameters), function_length, kind, is_strict_mode, might_need_arguments_object, false) { } @@ -498,9 +503,9 @@ class FunctionExpression final public: static bool must_have_name() { return false; } - FunctionExpression(SourceRange source_range, FlyString const& name, NonnullRefPtr body, Vector parameters, i32 function_length, FunctionKind kind, bool is_strict_mode, bool is_arrow_function = false) + FunctionExpression(SourceRange source_range, FlyString const& name, NonnullRefPtr body, Vector parameters, i32 function_length, FunctionKind kind, bool is_strict_mode, bool might_need_arguments_object, bool is_arrow_function = false) : Expression(source_range) - , FunctionNode(name, move(body), move(parameters), function_length, kind, is_strict_mode, is_arrow_function) + , FunctionNode(name, move(body), move(parameters), function_length, kind, is_strict_mode, might_need_arguments_object, is_arrow_function) { } diff --git a/Userland/Libraries/LibJS/Bytecode/Op.cpp b/Userland/Libraries/LibJS/Bytecode/Op.cpp index 99f9c718ac..ce4ca46364 100644 --- a/Userland/Libraries/LibJS/Bytecode/Op.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Op.cpp @@ -334,7 +334,7 @@ void Call::execute_impl(Bytecode::Interpreter& interpreter) const void NewFunction::execute_impl(Bytecode::Interpreter& interpreter) const { auto& vm = interpreter.vm(); - interpreter.accumulator() = ECMAScriptFunctionObject::create(interpreter.global_object(), m_function_node.name(), m_function_node.body(), m_function_node.parameters(), m_function_node.function_length(), vm.lexical_environment(), m_function_node.kind(), m_function_node.is_strict_mode(), m_function_node.is_arrow_function()); + interpreter.accumulator() = ECMAScriptFunctionObject::create(interpreter.global_object(), m_function_node.name(), m_function_node.body(), m_function_node.parameters(), m_function_node.function_length(), vm.lexical_environment(), m_function_node.kind(), m_function_node.is_strict_mode(), m_function_node.might_need_arguments_object(), m_function_node.is_arrow_function()); } void Return::execute_impl(Bytecode::Interpreter& interpreter) const diff --git a/Userland/Libraries/LibJS/Parser.cpp b/Userland/Libraries/LibJS/Parser.cpp index b70b652add..94f803d0f9 100644 --- a/Userland/Libraries/LibJS/Parser.cpp +++ b/Userland/Libraries/LibJS/Parser.cpp @@ -657,7 +657,8 @@ RefPtr Parser::try_parse_arrow_function_expression(bool expe return create_ast_node( { m_state.current_token.filename(), rule_start.position(), position() }, "", move(body), - move(parameters), function_length, FunctionKind::Regular, body->in_strict_mode(), true); + move(parameters), function_length, FunctionKind::Regular, body->in_strict_mode(), + /* might_need_arguments_object */ false, /* is_arrow_function */ true); } RefPtr Parser::try_parse_labelled_statement(AllowLabelledFunction allow_function) @@ -878,7 +879,7 @@ NonnullRefPtr Parser::parse_class_expression(bool expect_class_ break; } - //https://tc39.es/ecma262/#sec-class-definitions-static-semantics-early-errors + // https://tc39.es/ecma262/#sec-class-definitions-static-semantics-early-errors // ClassElement : static MethodDefinition // It is a Syntax Error if PropName of MethodDefinition is "prototype". if (is_static && name == "prototype"sv) @@ -975,11 +976,13 @@ NonnullRefPtr Parser::parse_class_expression(bool expect_class_ constructor = create_ast_node( { m_state.current_token.filename(), rule_start.position(), position() }, class_name, move(constructor_body), - Vector { FunctionNode::Parameter { FlyString { "args" }, nullptr, true } }, 0, FunctionKind::Regular, true); + Vector { FunctionNode::Parameter { FlyString { "args" }, nullptr, true } }, 0, FunctionKind::Regular, + /* is_strict_mode */ true, /* might_need_arguments_object */ false); } else { constructor = create_ast_node( { m_state.current_token.filename(), rule_start.position(), position() }, class_name, move(constructor_body), - Vector {}, 0, FunctionKind::Regular, true); + Vector {}, 0, FunctionKind::Regular, + /* is_strict_mode */ true, /* might_need_arguments_object */ false); } } @@ -1958,6 +1961,7 @@ NonnullRefPtr Parser::parse_function_node(u8 parse_options) TemporaryChange break_context_rollback(m_state.in_break_context, false); TemporaryChange continue_context_rollback(m_state.in_continue_context, false); TemporaryChange class_field_initializer_rollback(m_state.in_class_field_initializer, false); + TemporaryChange might_need_arguments_object_rollback(m_state.function_might_need_arguments_object, false); constexpr auto is_function_expression = IsSame; auto function_kind = (parse_options & FunctionNodeParseOptions::IsGeneratorFunction) != 0 ? FunctionKind::Generator : FunctionKind::Regular; @@ -1989,7 +1993,7 @@ NonnullRefPtr Parser::parse_function_node(u8 parse_options) if (function_length == -1) function_length = parameters.size(); - TemporaryChange change(m_state.in_function_context, true); + TemporaryChange function_context_rollback(m_state.in_function_context, true); auto old_labels_in_scope = move(m_state.labels_in_scope); ScopeGuard guard([&]() { @@ -2006,7 +2010,7 @@ NonnullRefPtr Parser::parse_function_node(u8 parse_options) return create_ast_node( { m_state.current_token.filename(), rule_start.position(), position() }, name, move(body), move(parameters), function_length, - function_kind, has_strict_directive); + function_kind, has_strict_directive, m_state.function_might_need_arguments_object); } Vector Parser::parse_formal_parameters(int& function_length, u8 parse_options) @@ -3129,6 +3133,11 @@ Token Parser::consume() { auto old_token = m_state.current_token; m_state.current_token = m_state.lexer.next(); + // NOTE: This is the bare minimum needed to decide whether we might need an arguments object + // in a function expression or declaration. ("might" because the AST implements some further + // conditions from the spec that rule out the need for allocating one) + if (old_token.type() == TokenType::Identifier && old_token.value().is_one_of("arguments"sv, "eval"sv)) + m_state.function_might_need_arguments_object = true; return old_token; } diff --git a/Userland/Libraries/LibJS/Parser.h b/Userland/Libraries/LibJS/Parser.h index 752bb14b78..7a95f4f19c 100644 --- a/Userland/Libraries/LibJS/Parser.h +++ b/Userland/Libraries/LibJS/Parser.h @@ -253,6 +253,7 @@ private: bool in_continue_context { false }; bool string_legacy_octal_escape_sequence_in_scope { false }; bool in_class_field_initializer { false }; + bool function_might_need_arguments_object { false }; ParserState(Lexer, Program::Type); }; diff --git a/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp b/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp index b6a7e86383..6df25398f3 100644 --- a/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp +++ b/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp @@ -627,7 +627,7 @@ ThrowCompletionOr eval_declaration_instantiation(VM& vm, GlobalObject& glo return throw_completion(exception->value()); for (auto& declaration : functions_to_initialize) { - auto* function = ECMAScriptFunctionObject::create(global_object, declaration.name(), declaration.body(), declaration.parameters(), declaration.function_length(), lexical_environment, declaration.kind(), declaration.is_strict_mode()); + auto* function = ECMAScriptFunctionObject::create(global_object, declaration.name(), declaration.body(), declaration.parameters(), declaration.function_length(), lexical_environment, declaration.kind(), declaration.is_strict_mode(), declaration.might_need_arguments_object()); if (global_var_environment) { global_var_environment->create_global_function_binding(declaration.name(), function, true); if (auto* exception = vm.exception()) diff --git a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp index d0ef282fee..05c605618b 100644 --- a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp +++ b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp @@ -24,7 +24,7 @@ namespace JS { -ECMAScriptFunctionObject* ECMAScriptFunctionObject::create(GlobalObject& global_object, FlyString name, Statement const& ecmascript_code, Vector parameters, i32 m_function_length, Environment* parent_scope, FunctionKind kind, bool is_strict, bool is_arrow_function) +ECMAScriptFunctionObject* ECMAScriptFunctionObject::create(GlobalObject& global_object, FlyString name, Statement const& ecmascript_code, Vector parameters, i32 m_function_length, Environment* parent_scope, FunctionKind kind, bool is_strict, bool might_need_arguments_object, bool is_arrow_function) { Object* prototype = nullptr; switch (kind) { @@ -35,10 +35,10 @@ ECMAScriptFunctionObject* ECMAScriptFunctionObject::create(GlobalObject& global_ prototype = global_object.generator_function_prototype(); break; } - return global_object.heap().allocate(global_object, move(name), ecmascript_code, move(parameters), m_function_length, parent_scope, *prototype, kind, is_strict, is_arrow_function); + return global_object.heap().allocate(global_object, move(name), ecmascript_code, move(parameters), m_function_length, parent_scope, *prototype, kind, is_strict, might_need_arguments_object, is_arrow_function); } -ECMAScriptFunctionObject::ECMAScriptFunctionObject(FlyString name, Statement const& ecmascript_code, Vector formal_parameters, i32 function_length, Environment* parent_scope, Object& prototype, FunctionKind kind, bool strict, bool is_arrow_function) +ECMAScriptFunctionObject::ECMAScriptFunctionObject(FlyString name, Statement const& ecmascript_code, Vector formal_parameters, i32 function_length, Environment* parent_scope, Object& prototype, FunctionKind kind, bool strict, bool might_need_arguments_object, bool is_arrow_function) : FunctionObject(prototype) , m_environment(parent_scope) , m_formal_parameters(move(formal_parameters)) @@ -48,6 +48,7 @@ ECMAScriptFunctionObject::ECMAScriptFunctionObject(FlyString name, Statement con , m_name(move(name)) , m_function_length(function_length) , m_kind(kind) + , m_might_need_arguments_object(might_need_arguments_object) , m_is_arrow_function(is_arrow_function) { // NOTE: This logic is from OrdinaryFunctionCreate, https://tc39.es/ecma262/#sec-ordinaryfunctioncreate @@ -159,7 +160,11 @@ ThrowCompletionOr ECMAScriptFunctionObject::function_declaration_instantia }); } - auto arguments_object_needed = this_mode() != ThisMode::Lexical; + auto arguments_object_needed = m_might_need_arguments_object; + + if (this_mode() == ThisMode::Lexical) + arguments_object_needed = false; + if (parameter_names.contains(vm.names.arguments.as_string())) arguments_object_needed = false; @@ -360,7 +365,7 @@ ThrowCompletionOr ECMAScriptFunctionObject::function_declaration_instantia VERIFY(!vm.exception()); for (auto& declaration : functions_to_initialize) { - auto* function = ECMAScriptFunctionObject::create(global_object(), declaration.name(), declaration.body(), declaration.parameters(), declaration.function_length(), lex_environment, declaration.kind(), declaration.is_strict_mode()); + auto* function = ECMAScriptFunctionObject::create(global_object(), declaration.name(), declaration.body(), declaration.parameters(), declaration.function_length(), lex_environment, declaration.kind(), declaration.is_strict_mode(), declaration.might_need_arguments_object()); var_environment->set_mutable_binding(global_object(), declaration.name(), function, false); } diff --git a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.h b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.h index 9e5f5eebcf..b936997988 100644 --- a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.h +++ b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.h @@ -28,9 +28,9 @@ public: Global, }; - static ECMAScriptFunctionObject* create(GlobalObject&, FlyString name, Statement const& ecmascript_code, Vector parameters, i32 m_function_length, Environment* parent_scope, FunctionKind, bool is_strict, bool is_arrow_function = false); + static ECMAScriptFunctionObject* create(GlobalObject&, FlyString name, Statement const& ecmascript_code, Vector parameters, i32 m_function_length, Environment* parent_scope, FunctionKind, bool is_strict, bool might_need_arguments_object = true, bool is_arrow_function = false); - ECMAScriptFunctionObject(FlyString name, Statement const& ecmascript_code, Vector parameters, i32 m_function_length, Environment* parent_scope, Object& prototype, FunctionKind, bool is_strict, bool is_arrow_function = false); + ECMAScriptFunctionObject(FlyString name, Statement const& ecmascript_code, Vector parameters, i32 m_function_length, Environment* parent_scope, Object& prototype, FunctionKind, bool is_strict, bool might_need_arguments_object, bool is_arrow_function); virtual void initialize(GlobalObject&) override; virtual ~ECMAScriptFunctionObject(); @@ -100,6 +100,7 @@ private: Optional m_bytecode_executable; i32 m_function_length { 0 }; FunctionKind m_kind { FunctionKind::Regular }; + bool m_might_need_arguments_object { true }; bool m_is_arrow_function { false }; bool m_has_simple_parameter_list { false }; }; diff --git a/Userland/Libraries/LibJS/Runtime/GeneratorFunctionConstructor.cpp b/Userland/Libraries/LibJS/Runtime/GeneratorFunctionConstructor.cpp index 721762bd86..34851e55b7 100644 --- a/Userland/Libraries/LibJS/Runtime/GeneratorFunctionConstructor.cpp +++ b/Userland/Libraries/LibJS/Runtime/GeneratorFunctionConstructor.cpp @@ -62,7 +62,7 @@ Value GeneratorFunctionConstructor::construct(FunctionObject& new_target) block.dump(executable); } - return ECMAScriptFunctionObject::create(global_object(), function->name(), function->body(), function->parameters(), function->function_length(), vm().lexical_environment(), FunctionKind::Generator, function->is_strict_mode(), false); + return ECMAScriptFunctionObject::create(global_object(), function->name(), function->body(), function->parameters(), function->function_length(), vm().lexical_environment(), FunctionKind::Generator, function->is_strict_mode(), function->might_need_arguments_object()); } }