From 350e6c54d7da67fb75756b2d03769d262af1e56a Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Thu, 7 Dec 2023 16:12:12 +0100 Subject: [PATCH] LibJS: Remove dedicated iterator result instructions in favor of GetById When iterating over an iterable, we get back a JS object with the fields "value" and "done". Before this change, we've had two dedicated instructions for retrieving the two fields: IteratorResultValue and IteratorResultDone. These had no fast path whatsoever and just did a generic [[Get]] access to fetch the corresponding property values. By replacing the instructions with GetById("value") and GetById("done"), they instantly get caching and JIT fast paths for free, making iterating over iterables much faster. :^) 26% speed-up on this microbenchmark: function go(a) { for (const p of a) { } } const a = []; a.length = 1_000_000; go(a); --- .../Libraries/LibJS/Bytecode/ASTCodegen.cpp | 26 ++++++++--------- .../Libraries/LibJS/Bytecode/Generator.cpp | 10 +++++++ Userland/Libraries/LibJS/Bytecode/Generator.h | 3 ++ .../Libraries/LibJS/Bytecode/Instruction.h | 2 -- .../Libraries/LibJS/Bytecode/Interpreter.cpp | 29 ------------------- Userland/Libraries/LibJS/Bytecode/Op.h | 22 -------------- Userland/Libraries/LibJS/JIT/Compiler.cpp | 28 ------------------ Userland/Libraries/LibJS/JIT/Compiler.h | 2 -- 8 files changed, 26 insertions(+), 96 deletions(-) diff --git a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp index dc164743d1..3e4de3e081 100644 --- a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp +++ b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp @@ -1344,7 +1344,7 @@ static Bytecode::CodeGenerationErrorOr generate_array_binding_pattern_byte generator.emit(); generator.emit(temp_iterator_result_reg); - generator.emit(); + generator.emit_iterator_complete(); generator.emit(is_iterator_exhausted_register); // We still have to check for exhaustion here. If the iterator is exhausted, @@ -1358,7 +1358,7 @@ static Bytecode::CodeGenerationErrorOr generate_array_binding_pattern_byte // Get the next value in the iterator generator.emit(temp_iterator_result_reg); - generator.emit(); + generator.emit_iterator_value(); auto& create_binding_block = generator.make_block(); generator.emit(Bytecode::Label { create_binding_block }); @@ -1798,7 +1798,7 @@ Bytecode::CodeGenerationErrorOr YieldExpression::generate_bytecode(Bytecod generator.emit(inner_result_register); // iv. Let done be ? IteratorComplete(innerResult). - generator.emit(); + generator.emit_iterator_complete(); // v. If done is true, then auto& type_is_normal_done_block = generator.make_block(); @@ -1811,7 +1811,7 @@ Bytecode::CodeGenerationErrorOr YieldExpression::generate_bytecode(Bytecod // 1. Return ? IteratorValue(innerResult). generator.emit(inner_result_register); - generator.emit(); + generator.emit_iterator_value(); generator.emit(Bytecode::Label { loop_end_block }); generator.switch_to_basic_block(type_is_normal_not_done_block); @@ -1822,7 +1822,7 @@ Bytecode::CodeGenerationErrorOr YieldExpression::generate_bytecode(Bytecod // FIXME: Yield currently only accepts a Value, not an object conforming to the IteratorResult interface, so we have to do an observable lookup of `value` here. // This only matters for non-async generators. - generator.emit(); + generator.emit_iterator_value(); generate_yield(generator, Bytecode::Label { continuation_block }, received_completion_register, received_completion_type_register, received_completion_value_register, type_identifier, value_identifier, AwaitBeforeYield::No); @@ -1871,7 +1871,7 @@ Bytecode::CodeGenerationErrorOr YieldExpression::generate_bytecode(Bytecod generator.emit(inner_result_register); // 5. Let done be ? IteratorComplete(innerResult). - generator.emit(); + generator.emit_iterator_complete(); // 6. If done is true, then auto& type_is_throw_done_block = generator.make_block(); @@ -1884,7 +1884,7 @@ Bytecode::CodeGenerationErrorOr YieldExpression::generate_bytecode(Bytecod // a. Return ? IteratorValue(innerResult). generator.emit(inner_result_register); - generator.emit(); + generator.emit_iterator_value(); generator.emit(Bytecode::Label { loop_end_block }); generator.switch_to_basic_block(type_is_throw_not_done_block); @@ -1895,7 +1895,7 @@ Bytecode::CodeGenerationErrorOr YieldExpression::generate_bytecode(Bytecod // FIXME: Yield currently only accepts a Value, not an object conforming to the IteratorResult interface, so we have to do an observable lookup of `value` here. // This only matters for non-async generators. - generator.emit(); + generator.emit_iterator_value(); generate_yield(generator, Bytecode::Label { continuation_block }, received_completion_register, received_completion_type_register, received_completion_value_register, type_identifier, value_identifier, AwaitBeforeYield::No); @@ -1970,7 +1970,7 @@ Bytecode::CodeGenerationErrorOr YieldExpression::generate_bytecode(Bytecod generator.emit(inner_return_result_register); // vii. Let done be ? IteratorComplete(innerReturnResult). - generator.emit(); + generator.emit_iterator_complete(); // viii. If done is true, then auto& type_is_return_done_block = generator.make_block(); @@ -1983,7 +1983,7 @@ Bytecode::CodeGenerationErrorOr YieldExpression::generate_bytecode(Bytecod // 1. Let value be ? IteratorValue(innerReturnResult). generator.emit(inner_result_register); - generator.emit(); + generator.emit_iterator_value(); // 2. Return Completion Record { [[Type]]: return, [[Value]]: value, [[Target]]: empty }. generator.perform_needed_unwinds(); @@ -1997,7 +1997,7 @@ Bytecode::CodeGenerationErrorOr YieldExpression::generate_bytecode(Bytecod // FIXME: Yield currently only accepts a Value, not an object conforming to the IteratorResult interface, so we have to do an observable lookup of `value` here. // This only matters for non-async generators. - generator.emit(); + generator.emit_iterator_value(); generate_yield(generator, Bytecode::Label { continuation_block }, received_completion_register, received_completion_type_register, received_completion_value_register, type_identifier, value_identifier, AwaitBeforeYield::No); @@ -2825,8 +2825,8 @@ static Bytecode::CodeGenerationErrorOr for_in_of_body_evaluation(Bytecode: // d. Let done be ? IteratorComplete(nextResult). auto iterator_result_register = generator.allocate_register(); generator.emit(iterator_result_register); + generator.emit_iterator_complete(); - generator.emit(); // e. If done is true, return V. auto& loop_continue = generator.make_block(); generator.emit( @@ -2836,7 +2836,7 @@ static Bytecode::CodeGenerationErrorOr for_in_of_body_evaluation(Bytecode: // f. Let nextValue be ? IteratorValue(nextResult). generator.emit(iterator_result_register); - generator.emit(); + generator.emit_iterator_value(); // g. If lhsKind is either assignment or varBinding, then if (head_result.lhs_kind != LHSKind::LexicalBinding) { diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.cpp b/Userland/Libraries/LibJS/Bytecode/Generator.cpp index fc98bc04ae..91f97d567c 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Generator.cpp @@ -584,4 +584,14 @@ void Generator::emit_get_by_id_with_this(IdentifierTableIndex id, Register this_ emit(id, this_reg, m_next_property_lookup_cache++); } +void Generator::emit_iterator_value() +{ + emit_get_by_id(intern_identifier("value"sv)); +} + +void Generator::emit_iterator_complete() +{ + emit_get_by_id(intern_identifier("done"sv)); +} + } diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.h b/Userland/Libraries/LibJS/Bytecode/Generator.h index 56032c35c2..3dbd7a9cad 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.h +++ b/Userland/Libraries/LibJS/Bytecode/Generator.h @@ -234,6 +234,9 @@ public: void emit_get_by_id(IdentifierTableIndex); void emit_get_by_id_with_this(IdentifierTableIndex, Register); + void emit_iterator_value(); + void emit_iterator_complete(); + [[nodiscard]] size_t next_global_variable_cache() { return m_next_global_variable_cache++; } [[nodiscard]] size_t next_environment_variable_cache() { return m_next_environment_variable_cache++; } [[nodiscard]] size_t next_property_lookup_cache() { return m_next_property_lookup_cache++; } diff --git a/Userland/Libraries/LibJS/Bytecode/Instruction.h b/Userland/Libraries/LibJS/Bytecode/Instruction.h index 2721fbd9fa..80dd933951 100644 --- a/Userland/Libraries/LibJS/Bytecode/Instruction.h +++ b/Userland/Libraries/LibJS/Bytecode/Instruction.h @@ -65,8 +65,6 @@ O(InstanceOf) \ O(IteratorClose) \ O(IteratorNext) \ - O(IteratorResultDone) \ - O(IteratorResultValue) \ O(IteratorToArray) \ O(Jump) \ O(JumpConditional) \ diff --git a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp index 9922c3d2be..048298968c 100644 --- a/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Interpreter.cpp @@ -1287,25 +1287,6 @@ ThrowCompletionOr IteratorNext::execute_impl(Bytecode::Interpreter& interp return {}; } -ThrowCompletionOr IteratorResultDone::execute_impl(Bytecode::Interpreter& interpreter) const -{ - auto& vm = interpreter.vm(); - auto iterator_result = TRY(interpreter.accumulator().to_object(vm)); - - auto complete = TRY(iterator_complete(vm, iterator_result)); - interpreter.accumulator() = Value(complete); - return {}; -} - -ThrowCompletionOr IteratorResultValue::execute_impl(Bytecode::Interpreter& interpreter) const -{ - auto& vm = interpreter.vm(); - auto iterator_result = TRY(interpreter.accumulator().to_object(vm)); - - interpreter.accumulator() = TRY(iterator_value(vm, iterator_result)); - return {}; -} - ThrowCompletionOr NewClass::execute_impl(Bytecode::Interpreter& interpreter) const { interpreter.accumulator() = TRY(new_class(interpreter.vm(), interpreter.accumulator(), m_class_expression, m_lhs_name)); @@ -1768,16 +1749,6 @@ DeprecatedString IteratorNext::to_deprecated_string_impl(Executable const&) cons return "IteratorNext"; } -DeprecatedString IteratorResultDone::to_deprecated_string_impl(Executable const&) const -{ - return "IteratorResultDone"; -} - -DeprecatedString IteratorResultValue::to_deprecated_string_impl(Executable const&) const -{ - return "IteratorResultValue"; -} - DeprecatedString ResolveThisBinding::to_deprecated_string_impl(Bytecode::Executable const&) const { return "ResolveThisBinding"sv; diff --git a/Userland/Libraries/LibJS/Bytecode/Op.h b/Userland/Libraries/LibJS/Bytecode/Op.h index 6079d5dbdc..5d8867b3ed 100644 --- a/Userland/Libraries/LibJS/Bytecode/Op.h +++ b/Userland/Libraries/LibJS/Bytecode/Op.h @@ -1492,28 +1492,6 @@ public: DeprecatedString to_deprecated_string_impl(Bytecode::Executable const&) const; }; -class IteratorResultDone final : public Instruction { -public: - IteratorResultDone() - : Instruction(Type::IteratorResultDone, sizeof(*this)) - { - } - - ThrowCompletionOr execute_impl(Bytecode::Interpreter&) const; - DeprecatedString to_deprecated_string_impl(Bytecode::Executable const&) const; -}; - -class IteratorResultValue final : public Instruction { -public: - IteratorResultValue() - : Instruction(Type::IteratorResultValue, sizeof(*this)) - { - } - - ThrowCompletionOr execute_impl(Bytecode::Interpreter&) const; - DeprecatedString to_deprecated_string_impl(Bytecode::Executable const&) const; -}; - class ResolveThisBinding final : public Instruction { public: explicit ResolveThisBinding() diff --git a/Userland/Libraries/LibJS/JIT/Compiler.cpp b/Userland/Libraries/LibJS/JIT/Compiler.cpp index ee85a2e0d3..2c27fb3918 100644 --- a/Userland/Libraries/LibJS/JIT/Compiler.cpp +++ b/Userland/Libraries/LibJS/JIT/Compiler.cpp @@ -3111,20 +3111,6 @@ void Compiler::compile_iterator_next(Bytecode::Op::IteratorNext const&) check_exception(); } -static Value cxx_iterator_result_done(VM& vm, Value iterator) -{ - auto iterator_result = TRY_OR_SET_EXCEPTION(iterator.to_object(vm)); - return Value(TRY_OR_SET_EXCEPTION(iterator_complete(vm, iterator_result))); -} - -void Compiler::compile_iterator_result_done(Bytecode::Op::IteratorResultDone const&) -{ - load_accumulator(ARG1); - native_call((void*)cxx_iterator_result_done); - store_accumulator(RET); - check_exception(); -} - static Value cxx_throw_if_not_object(VM& vm, Value value) { if (!value.is_object()) @@ -3153,20 +3139,6 @@ void Compiler::compile_throw_if_nullish(Bytecode::Op::ThrowIfNullish const&) check_exception(); } -static Value cxx_iterator_result_value(VM& vm, Value iterator) -{ - auto iterator_result = TRY_OR_SET_EXCEPTION(iterator.to_object(vm)); - return TRY_OR_SET_EXCEPTION(iterator_value(vm, iterator_result)); -} - -void Compiler::compile_iterator_result_value(Bytecode::Op::IteratorResultValue const&) -{ - load_accumulator(ARG1); - native_call((void*)cxx_iterator_result_value); - store_accumulator(RET); - check_exception(); -} - static Value cxx_iterator_close(VM& vm, Value iterator, Completion::Type completion_type, Optional const& completion_value) { auto& iterator_record = verify_cast(iterator.as_object()); diff --git a/Userland/Libraries/LibJS/JIT/Compiler.h b/Userland/Libraries/LibJS/JIT/Compiler.h index ac88fd2769..c836dd0ad3 100644 --- a/Userland/Libraries/LibJS/JIT/Compiler.h +++ b/Userland/Libraries/LibJS/JIT/Compiler.h @@ -117,10 +117,8 @@ private: O(GetObjectFromIteratorRecord, get_object_from_iterator_record) \ O(GetNextMethodFromIteratorRecord, get_next_method_from_iterator_record) \ O(IteratorNext, iterator_next) \ - O(IteratorResultDone, iterator_result_done) \ O(ThrowIfNotObject, throw_if_not_object) \ O(ThrowIfNullish, throw_if_nullish) \ - O(IteratorResultValue, iterator_result_value) \ O(IteratorClose, iterator_close) \ O(IteratorToArray, iterator_to_array) \ O(Append, append) \