From 3661d674aef23360705251a31b7362cf18c493b8 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Wed, 12 Jul 2023 04:06:59 +0200 Subject: [PATCH] LibJS: Add optimized GetGlobal instruction to access global variables Using a special instruction to access global variables allows skipping the environment chain traversal for them and going directly to the module/global environment. Currently, this instruction only caches the offset for bindings that belong to the global object environment. However, there is also an opportunity to cache the offset in the global declarative record. This change results in a 57% increase in speed for imaging-gaussian-blur.js in Kraken. --- .../Libraries/LibJS/Bytecode/ASTCodegen.cpp | 4 +- .../Libraries/LibJS/Bytecode/Executable.h | 5 ++ .../Libraries/LibJS/Bytecode/Generator.cpp | 4 ++ Userland/Libraries/LibJS/Bytecode/Generator.h | 3 + .../Libraries/LibJS/Bytecode/Instruction.h | 1 + Userland/Libraries/LibJS/Bytecode/Op.cpp | 60 +++++++++++++++++++ Userland/Libraries/LibJS/Bytecode/Op.h | 19 ++++++ 7 files changed, 95 insertions(+), 1 deletion(-) diff --git a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp index 22e3479346a..3e652422395 100644 --- a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp +++ b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp @@ -295,7 +295,9 @@ Bytecode::CodeGenerationErrorOr RegExpLiteral::generate_bytecode(Bytecode: Bytecode::CodeGenerationErrorOr Identifier::generate_bytecode(Bytecode::Generator& generator) const { - if (is_local()) { + if (is_global()) { + generator.emit(generator.intern_identifier(m_string), generator.next_global_variable_cache()); + } else if (is_local()) { generator.emit(local_variable_index()); } else { generator.emit(generator.intern_identifier(m_string)); diff --git a/Userland/Libraries/LibJS/Bytecode/Executable.h b/Userland/Libraries/LibJS/Bytecode/Executable.h index 5cc7268f66f..3a9b817daaa 100644 --- a/Userland/Libraries/LibJS/Bytecode/Executable.h +++ b/Userland/Libraries/LibJS/Bytecode/Executable.h @@ -21,9 +21,14 @@ struct PropertyLookupCache { u64 unique_shape_serial_number { 0 }; }; +struct GlobalVariableCache : public PropertyLookupCache { + u64 environment_serial_number { 0 }; +}; + struct Executable { DeprecatedFlyString name; Vector property_lookup_caches; + Vector global_variable_caches; Vector> basic_blocks; NonnullOwnPtr string_table; NonnullOwnPtr identifier_table; diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.cpp b/Userland/Libraries/LibJS/Bytecode/Generator.cpp index 356d543bac7..834e26e92d2 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Generator.cpp @@ -57,9 +57,13 @@ CodeGenerationErrorOr> Generator::generate(ASTNode con Vector property_lookup_caches; property_lookup_caches.resize(generator.m_next_property_lookup_cache); + Vector global_variable_caches; + global_variable_caches.resize(generator.m_next_global_variable_cache); + return adopt_own(*new Executable { .name = {}, .property_lookup_caches = move(property_lookup_caches), + .global_variable_caches = move(global_variable_caches), .basic_blocks = move(generator.m_root_basic_blocks), .string_table = move(generator.m_string_table), .identifier_table = move(generator.m_identifier_table), diff --git a/Userland/Libraries/LibJS/Bytecode/Generator.h b/Userland/Libraries/LibJS/Bytecode/Generator.h index 732d0d07036..772e81f5d50 100644 --- a/Userland/Libraries/LibJS/Bytecode/Generator.h +++ b/Userland/Libraries/LibJS/Bytecode/Generator.h @@ -202,6 +202,8 @@ public: void emit_get_by_id(IdentifierTableIndex); void emit_get_by_id_with_this(IdentifierTableIndex, Register); + [[nodiscard]] size_t next_global_variable_cache() { return m_next_global_variable_cache++; } + private: Generator(); ~Generator() = default; @@ -222,6 +224,7 @@ private: u32 m_next_register { 2 }; u32 m_next_block { 1 }; u32 m_next_property_lookup_cache { 0 }; + u32 m_next_global_variable_cache { 0 }; FunctionKind m_enclosing_function_kind { FunctionKind::Normal }; Vector m_continuable_scopes; Vector m_breakable_scopes; diff --git a/Userland/Libraries/LibJS/Bytecode/Instruction.h b/Userland/Libraries/LibJS/Bytecode/Instruction.h index be9a96a78ce..30cab2e4589 100644 --- a/Userland/Libraries/LibJS/Bytecode/Instruction.h +++ b/Userland/Libraries/LibJS/Bytecode/Instruction.h @@ -45,6 +45,7 @@ O(GetObjectPropertyIterator) \ O(GetPrivateById) \ O(GetVariable) \ + O(GetGlobal) \ O(GetLocal) \ O(GreaterThan) \ O(GreaterThanEquals) \ diff --git a/Userland/Libraries/LibJS/Bytecode/Op.cpp b/Userland/Libraries/LibJS/Bytecode/Op.cpp index 08dc405107f..f6866bc0618 100644 --- a/Userland/Libraries/LibJS/Bytecode/Op.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Op.cpp @@ -417,6 +417,61 @@ ThrowCompletionOr GetVariable::execute_impl(Bytecode::Interpreter& interpr return {}; } +ThrowCompletionOr GetGlobal::execute_impl(Bytecode::Interpreter& interpreter) const +{ + auto& vm = interpreter.vm(); + auto& realm = *vm.current_realm(); + + auto const& name = interpreter.current_executable().get_identifier(m_identifier); + + auto& cache = interpreter.current_executable().global_variable_caches[m_cache_index]; + auto& binding_object = realm.global_environment().object_record().binding_object(); + auto& declarative_record = realm.global_environment().declarative_record(); + + // OPTIMIZATION: If the shape of the object hasn't changed, we can use the cached property offset. + // NOTE: Unique shapes don't change identity, so we compare their serial numbers instead. + auto& shape = binding_object.shape(); + if (cache.environment_serial_number == declarative_record.environment_serial_number() + && &shape == cache.shape + && (!shape.is_unique() || shape.unique_shape_serial_number() == cache.unique_shape_serial_number)) { + interpreter.accumulator() = binding_object.get_direct(cache.property_offset.value()); + return {}; + } + + cache.environment_serial_number = declarative_record.environment_serial_number(); + + if (vm.running_execution_context().script_or_module.has>()) { + // NOTE: GetGlobal is used to access variables stored in the module environment and global environment. + // The module environment is checked first since it precedes the global environment in the environment chain. + auto& module_environment = *vm.running_execution_context().script_or_module.get>()->environment(); + if (TRY(module_environment.has_binding(name))) { + // TODO: Cache offset of binding value + interpreter.accumulator() = TRY(module_environment.get_binding_value(vm, name, vm.in_strict_mode())); + return {}; + } + } + + if (TRY(declarative_record.has_binding(name))) { + // TODO: Cache offset of binding value + interpreter.accumulator() = TRY(declarative_record.get_binding_value(vm, name, vm.in_strict_mode())); + return {}; + } + + if (TRY(binding_object.has_property(name))) { + CacheablePropertyMetadata cacheable_metadata; + interpreter.accumulator() = js_undefined(); + interpreter.accumulator() = TRY(binding_object.internal_get(name, interpreter.accumulator(), &cacheable_metadata)); + if (cacheable_metadata.type == CacheablePropertyMetadata::Type::OwnProperty) { + cache.shape = shape; + cache.property_offset = cacheable_metadata.property_offset.value(); + cache.unique_shape_serial_number = shape.unique_shape_serial_number(); + } + return {}; + } + + return vm.throw_completion(ErrorType::UnknownIdentifier, name); +} + ThrowCompletionOr GetLocal::execute_impl(Bytecode::Interpreter& interpreter) const { auto& vm = interpreter.vm(); @@ -1463,6 +1518,11 @@ DeprecatedString GetVariable::to_deprecated_string_impl(Bytecode::Executable con return DeprecatedString::formatted("GetVariable {} ({})", m_identifier, executable.identifier_table->get(m_identifier)); } +DeprecatedString GetGlobal::to_deprecated_string_impl(Bytecode::Executable const& executable) const +{ + return DeprecatedString::formatted("GetGlobal {} ({})", m_identifier, executable.identifier_table->get(m_identifier)); +} + DeprecatedString GetLocal::to_deprecated_string_impl(Bytecode::Executable const&) const { return DeprecatedString::formatted("GetLocal {}", m_index); diff --git a/Userland/Libraries/LibJS/Bytecode/Op.h b/Userland/Libraries/LibJS/Bytecode/Op.h index 49c2ba580d3..e419b79436d 100644 --- a/Userland/Libraries/LibJS/Bytecode/Op.h +++ b/Userland/Libraries/LibJS/Bytecode/Op.h @@ -524,6 +524,25 @@ private: Optional mutable m_cached_environment_coordinate; }; +class GetGlobal final : public Instruction { +public: + explicit GetGlobal(IdentifierTableIndex identifier, u32 cache_index) + : Instruction(Type::GetGlobal) + , m_identifier(identifier) + , m_cache_index(cache_index) + { + } + + ThrowCompletionOr execute_impl(Bytecode::Interpreter&) const; + DeprecatedString to_deprecated_string_impl(Bytecode::Executable const&) const; + void replace_references_impl(BasicBlock const&, BasicBlock const&) { } + void replace_references_impl(Register, Register) { } + +private: + IdentifierTableIndex m_identifier; + u32 m_cache_index { 0 }; +}; + class GetLocal final : public Instruction { public: explicit GetLocal(size_t index)